Unity-Technologies / ROS-TCP-Connector

Apache License 2.0
296 stars 93 forks source link

ROSConnection.GetOrCreateInstance().RegisterPublisher(...) seems like registering the same publisher twice. #299

Open ynyBonfennil opened 2 years ago

ynyBonfennil commented 2 years ago

Detail of this bug is already described here: https://github.com/Unity-Technologies/Unity-Robotics-Hub/issues/377.

When I register a publisher by ROSConnection.GetOrCreateInstance().RegisterPublisher(...) and the connection with ROS TCP Endpoint is established, it registers the same publisher twice and ROS TCP Endpoint outputs the following warning:

[WARN] [1665548483.104343200] [rcl.logging_rosout]: Publisher already registered for provided node name. If this is due to multiple nodes with the same name then all logs for that logger name will go out over the existing publisher. As soon as any node with that name is destructed it will unregister the publisher, preventing any further logs for that name from being published on the rosout topic.

My environment is as follow:

As I wrote on the issue above, I found that public RosTopicState RegisterPublisher(...) in ROSConnection.cs calls topicState.RegisterPublisher(resolvedQueueSize, resolvedLatch) at line 352 and it stores a command of registering a publisher in the queue, although public RosTopicState RegisterPublisher(...) calls GetOrCreateTopic(rosTopicName, rosMessageName) at line 348 and it also stores a command of registering a publisher in the same queue when a connection with ROS TCP Endpoint is established.

The commands stored in the queue will be sent after the connection with ROS TCP Endpoint is ready, and that means the current implementation seems to register the same publisher twice.


I don't think this is a good idea but commenting out either line 207 or 260 of RosTopicState.cs can make the warning in question disappear.

(commenting out line 207)

        public void RegisterPublisher(int queueSize, bool latch)
        {
            if (IsPublisher)
            {
                Debug.LogWarning($"Publisher for topic {m_Topic} registered twice!");
                return;
            }
            IsPublisher = true;
            IsPublisherLatched = latch;
            // m_ConnectionInternal.SendPublisherRegistration(m_Topic, m_RosMessageName, queueSize, latch);  // comment out
            CreateMessageSender(queueSize);
        }

(or commenting out line 260)

        internal void OnConnectionEstablished(NetworkStream stream)
        {
            if (m_SubscriberCallbacks.Count > 0 && !SentSubscriberRegistration)
            {
                m_ConnectionInternal.SendSubscriberRegistration(m_Topic, m_RosMessageName, stream);
                SentSubscriberRegistration = true;
            }

            if (IsUnityService)
            {
                m_ConnectionInternal.SendUnityServiceRegistration(m_Topic, m_RosMessageName, stream);
            }

            if (IsPublisher)
            {
                //Register the publisher before sending anything.
                //m_ConnectionInternal.SendPublisherRegistration(m_Topic, m_RosMessageName, m_MessageSender.QueueSize, IsPublisherLatched, stream);  // comment out
                if (IsPublisherLatched)
                {
                    m_MessageSender.PrepareLatchMessage();
                    m_ConnectionInternal.AddSenderToQueue(m_MessageSender);
                }
            }

            if (m_IsRosService)
            {
                m_ConnectionInternal.SendRosServiceRegistration(m_Topic, m_RosMessageName, stream);
            }
        }