Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GZ->ROS communication doesn't work when using the bridge and gz_server in the same composable container #555

Closed
azeey opened this issue May 30, 2024 · 8 comments · Fixed by #559
Assignees
Labels
bug Something isn't working

Comments

@azeey
Copy link
Contributor

azeey commented May 30, 2024

Part of #544

When the bridge and gz_server nodes are in the same process, GZ->ROS communication doesn't work because of

// Ignore messages that are published from this bridge.
if (!_info.IntraProcess()) {
this->gz_callback(_msg, ros_pub);
}

which ignores all gz-transport messages published from the same process. This was done to enable bidirectional bridges, so I don't think we can simply remove it.

@azeey azeey added the bug Something isn't working label May 30, 2024
@caguero
Copy link
Contributor

caguero commented Jun 4, 2024

I thought about this and have a few ideas:

1. Solve it a Transport level: Similar to ROS, add a new IgnoreLocalMessages function to the SubscribeOptions class. This will ignore messages coming from the same node. Note that this will still allow to receive messages from other nodes within the same process. For that, we might need to extend the PublishMsgDetails struct to store the UUID of the sender node. Then, when processing the local handlers before running a local callback, we'll compare the sender UUID with the UUID of the subscriber node (this UUID is already in the SubscriptionHandler class). If they match and the IgnoreLocalMessages option is set, we'll discard this message.

2. Solve it at the bridge with extra support from Transport.

I need to review the bridge code but another idea could be to modify Transport to include the Node UUID of the sender in the message sent. That way, it could be included in the MessageInfo instance when passed to each callback (on the receiver side).

Then, in the bridge, when receiving a message from a callback we could check the Node UUID of the sender. If that matches the UUID of the bridge's publisher we know we should ignore the message to avoid loops.

I'm leaning towards (1) because if doesn't add any extra overhead to the network protocol.

@azeey
Copy link
Contributor Author

azeey commented Jun 4, 2024

  1. Solve it a Transport level: Similar to ROS, add a new IgnoreLocalMessages function to the SubscribeOptions class. This will ignore messages coming from the same node. Note that this will still allow to receive messages from other nodes

I would also be in favor of (1), but I think we would only need the IgnoreLocalMessages option because it's okay to receive messages from other nodes in the same process. From

std::shared_ptr<gz::transport::Node> gz_node_;
, each "bridge" has a single gz-transport Node, so the double message issue would only occur if we receive a message from the same node.

@caguero
Copy link
Contributor

caguero commented Jun 4, 2024

I think we need a solution where "real" publishers share the process with the bridge, which should be the case where we use composition.

That's the case where the current bridge implementation doesn't work as we're relying only on the isIntraprocess() option. That will discard all messages from "real" publishers. And if we comment it out, we'll generate a loop of messages (or at least a duplication).

That's why I think proposal (1) has a finer control because we'll discriminate the case where the bridge subscriber receives the data publisher from the bridge. I think that's what we want (we should set that ignorelocalMessages to true in the bridge.

Do you agree?

@azeey
Copy link
Contributor Author

azeey commented Jun 4, 2024

Yes, I agree. I'm saying we don't need the part where you said

For that, we might need to extend the PublishMsgDetails struct to store the UUID of the sender node.

We just need to implement the IgnoreLocalMessages option from (1).

@azeey
Copy link
Contributor Author

azeey commented Jun 4, 2024

Maybe I'm misunderstanding. I thought in (1) you were stating two solutions. (a) IgnoreLocalMessages that still allows messages from other nodes in the same process. (b) A further modification to disable messages from nodes in the same process. Is that not what you're saying?

@caguero
Copy link
Contributor

caguero commented Jun 4, 2024

Sorry if this was a bit confusing. My proposal in (1) is a single solution and I was describing some implementation details to do it. The ignoreLocalMessages is essentially a flag that you could use on subscription to disable messages published from the same node. Independently of how you set ignoreLocalMessages, messages from other nodes should always be received.

@azeey
Copy link
Contributor Author

azeey commented Jun 4, 2024

I see. Well, let's go ahead with (1) then since we're both on the same page.

@caguero
Copy link
Contributor

caguero commented Jun 4, 2024

See gazebosim/gz-transport#506 and #559

@github-project-automation github-project-automation bot moved this from Inbox to Done in Core development Jun 7, 2024
@azeey azeey moved this from Done to Highlights in Core development Jun 10, 2024
@azeey azeey moved this from Highlights to Done in Core development Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants