Added the ability to explicitly set type names and some QoS settings #61
Conversation
- Updated the docs - Added in "system_default" qos values - Fixed type publishers are now made right away rather than waiting for an mqtt message to come in
|
There were a lot of combinations of explicit/deduced types and explicit/auto QoS in each direction tested. Please let me know what kind of evidence you would like me to provide. Its gonna end up being a lot of screenshots if you want all tested combinations. |
|
I should also be forward that there is a ninja nerf in here that is kinda ugly. Whenever we are dealing with fixed_types I changed the encodings of bools in the primitive mqtt messages to be "0" or "1" instead of "true" or "false". In my particular use case this is because this is how a device I am communicating with encodes those things and I couldn't figure out a clean way to make either or supported without a bunch of typing. |
|
i do wonder if a future pass through this code would do better to refactor the code into 2 separate nodes, perhaps utilizing a common library. One seems to be focused more on using mqtt as some sort of intermediary transport between two ros systems (eg, ros <-> mqtt <-> ros) vs the other which acting as an adapter between mqtt and ros. Not, sure exactly but when I look at #51 I am thinking that some sort of biggish refactor would be desirable sometime soon |
There was a problem hiding this comment.
Thank you very much, your changes are very welcome! Unfortunately it took me a while to find time to review. I have left a few comments that you could address.
Regarding testing: you said that you had basically tested a lot of QoS combinations? I don't require proof for all possible combinations, but it would be great if you could reaffirm that you are confident with the changes. :)
If a particular setup is not working, someone will eventually run into that problem and can then file a bug report.
Co-authored-by: Lennart Reiher <lreiher@me.com>
This change now allows non-primitive encoding to work with fixed types on either side of the bridge. Note that if the ros type specified mqtt2ros does not match the one received from the mqtt type topic then the config file specified version will win (eg, the type will not be allowed to change). Note that this is necessary since ROS doesn't really allow us to change the type of topics on the fly and since we used a fixed type, the publisher was created on startup.
If you'd like me to provide some sort of OQE (objective quality evidence), I can capture some screenshots or termincal captures of what I have done. But the basic methodology for testing all these combinations has been to modify one of the example configurations (mainly params.ros2.primitive-fixed-type-and-qos.yaml) and then run something like:
to test the auto discovery, and generally adding in Auto QoS was verified using the command line tool to very the subcription qos matched my expectation. Eg: I did actually have a bug in there with the explicit QoS on the ros subscription side. I did rerun those tests with a couple of QoS variations and virified those using the cli. |
There was a problem hiding this comment.
Looks good from my side and I believe that you have performed thorough testing! I checked a couple minimal setups myself.
I'm ready to merge, please confirm that you are finished with the PR as well. :)
Again, thank you very much! Merging this in now would allow me to directly release it for the new ROS 2 Jazzy.
I'm happy if you're happy. The only comment not covered was that one place you asked for an error message, but that is relatively minor. Merge away. |
Resolves: #48