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

Dynamic registration of topics #35

Merged
merged 19 commits into from
Nov 29, 2023
Merged

Dynamic registration of topics #35

merged 19 commits into from
Nov 29, 2023

Conversation

mvccogo
Copy link
Contributor

@mvccogo mvccogo commented Oct 2, 2023

Runtime registration of new ROS2 -> MQTT or MQTT -> ROS2 mappings, as mentioned in #25 .
Services do not return anything for now, but I guess they could return the same string that is printed for the mqtt_client node.

@mvccogo
Copy link
Contributor Author

mvccogo commented Oct 2, 2023

I've also separated it into two services as the config files also let the user decide to not have the other side bridged.

Copy link
Member

@lreiher lreiher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super nice addition, thank you very much for implementing this feature! I have made a few minor suggestion remarks.

mqtt_client_interfaces/srv/NewRos2ToMqtt.srv Outdated Show resolved Hide resolved
mqtt_client_interfaces/srv/NewMqttToRos2.srv Outdated Show resolved Hide resolved
mqtt_client_interfaces/srv/NewRos2ToMqtt.srv Outdated Show resolved Hide resolved
mqtt_client_interfaces/srv/NewMqttToRos2.srv Outdated Show resolved Hide resolved
mqtt_client/include/mqtt_client/MqttClient.ros2.hpp Outdated Show resolved Hide resolved
mqtt_client/src/MqttClient.ros2.cpp Outdated Show resolved Hide resolved
mqtt_client/src/MqttClient.ros2.cpp Outdated Show resolved Hide resolved
mqtt_client_interfaces/srv/NewMqttToRos2.srv Outdated Show resolved Hide resolved
mqtt_client_interfaces/srv/NewRos2ToMqtt.srv Outdated Show resolved Hide resolved
mqtt_client_interfaces/srv/NewMqttToRos2.srv Outdated Show resolved Hide resolved
mvccogo and others added 11 commits October 5, 2023 13:47
Co-authored-by: Lennart Reiher <lreiher@me.com>
Co-authored-by: Lennart Reiher <lreiher@me.com>
Co-authored-by: Lennart Reiher <lreiher@me.com>
Co-authored-by: Lennart Reiher <lreiher@me.com>
Co-authored-by: Lennart Reiher <lreiher@me.com>
Co-authored-by: Lennart Reiher <lreiher@me.com>
Co-authored-by: Lennart Reiher <lreiher@me.com>
@lreiher
Copy link
Member

lreiher commented Oct 23, 2023

Hi @mvccogo, there's not much missing to finalizing this PR, right? Let me know if you need anything from me, otherwise I'd be happy to release it soon. :)

@mvccogo
Copy link
Contributor Author

mvccogo commented Oct 24, 2023

Been busy the past weeks - will try finishing it up by the end of this week. Thanks for the reminder 😅

@mvccogo
Copy link
Contributor Author

mvccogo commented Oct 24, 2023

Actually, @lreiher I've replied to one of your reviews regarding the [] operator (if it should overwrite of not the mapping). If you could give me your opinion on the expected behavior, I can wrap it up.

@lreiher
Copy link
Member

lreiher commented Oct 25, 2023

I don't see any response from you, only my question:

What happens if the ROS topic is already being bridged? All settings are overridden and a new mapping is established?

@mvccogo
Copy link
Contributor Author

mvccogo commented Oct 26, 2023

I don't see any response from you, only my question:

What happens if the ROS topic is already being bridged? All settings are overridden and a new mapping is established?

Ah, no problem, I will replicate my answer to that question here then:

The std::map [] operator would return a Ros2MqttInterface& previously set and the code following this snippet would just overwrite the settings. I thought this was the expected behavior, but now that I think about it this code could silently change previously set mappings without warning the user. So I assume it could either be:

  1. Overwrite mapping settings with new values
  2. Block existing mapping from being changed if the key is found

Let me know what you think, please. I think the first option would be better, giving more freedom for the user to change mappings on the fly.

@lreiher
Copy link
Member

lreiher commented Oct 26, 2023

I have added my answer to the thread above.

@mvccogo
Copy link
Contributor Author

mvccogo commented Nov 13, 2023

I've started a review regarding your observation... I think a few more changes are needed in order to correctly update an existing mapping.

@lreiher
Copy link
Member

lreiher commented Nov 14, 2023

Okay, thanks for looking into this again! I'm ready to review once you have added more changes!

@mvccogo
Copy link
Contributor Author

mvccogo commented Nov 21, 2023

@lreiher I've added a commit that recreates the Mqtt2Ros mapping if it is marked as stale, please give it a look when you have the time. I have not tested recreating an existing mapping yet (this might be the last thing required to merge this PR, I just didn't have the time to set up a quick test), but the changes are not that remarkable (the only way to set a mapping as stale is by calling the service).

@lreiher
Copy link
Member

lreiher commented Nov 28, 2023

I have just pushed a few more small changes, adding also the is_stale logic to the ros2mqtt direction.

I have also conducted a few simple tests with adding new mqtt2ros or ros2mqtt bridges.

As soon as you give the go-ahead, I am going to merge this, thanks a lot! :)

@mvccogo
Copy link
Contributor Author

mvccogo commented Nov 29, 2023

@lreiher Thank you for tidying up the remaining formatting issues! Feel free to merge it 👍

@lreiher lreiher merged commit c5d5405 into ika-rwth-aachen:main Nov 29, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants