-
Notifications
You must be signed in to change notification settings - Fork 41
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
JSON support to achieve full integration with non-ROS devices for arbitrary ROS message types #51
base: main
Are you sure you want to change the base?
Conversation
…ROS message types
Hi, sorry for the delayed response! Awesome addition to the mqtt_client, thank you very very much! I will try to test it out soon. Meanwhile, I have seen that |
Hi again! Actually the I recently started working on porting to I still didn't have time to test ROS2 support, so if you can confirm tests working for both ROS and ROS2 it would be great. You can check my PR on |
status_update: ROS2 support is now fixed and tested working! The problem was (in addition to coding errors) that ROS2 message serialization procedure is different from ROS1 |
Hi, Keep up the good work! |
Hello Thank you for your nice comments. I would double check that you are specifically pulling my fork of |
Thank you for your support. I could get everything up and running, I did not realize that I mixed up your fork with the original one... sight |
Hi, again thank you very much for the great effort! I have tried to get the ROS2 version running for some quick tests, but encountered a few issues. Here's what I tried:
Perhaps you can help out? Could you provide a working container-based setup? |
Hello, I just would like to make sure about something that wasn't explicitly mentioned in your steps.
Does it not work even after doing As for |
Thank you, first building I didn't manage though to quickly test in ROS 1. Looks like I'm not sure how we should proceed at this point. While I think it is a great feature, I'm leaning towards not conducting a through review and not merging right now. This implementation depends on changes to This doesn't stop anyone from using the feature with your forks though! :) |
@lreiher @ahmad-ra I do agree that in an ideal world the rosx_introspection should be easier to be installed using the simple apt install method. However, besides the FastCDR library I believe, with a little bit of explanation is not too difficult to install. In the end, the extra steps are not too many. As for FastCDR, it is a bit more tricky, if possible it should be included as a 3rd party (perhaps it is and I am too new to ROS to understand) and make it easier to install. Nevertheless as a newbie to ROS and Linux I was able to get everything running within a bit of time. I believe some more guidance in the README.md will be sufficient. @lreiher I ended up installing FastCDR globally which requires a cmake update to 3.20 (or later) for ROS1 (noetic). I do agree that things could be easier to get running but this is a first step towards it. At this point I am just happy to have it running. And, oh boy, it is stable:) |
…kage, rather than a static lib built with cmake
I totally agree with @fate4gle that this is the first step towards the goal, since the changes done are not small, and it has to start somewhere, then gradually get better in terms of easier usage, and that the Readme should be updated with better instructions. I also agree with @lreiher that streamlining the process is important to release it, so I think we can move forward some steps towards having an easier installation, to not leave the work half-done, and to see the fruitful result of the efforts where the feature will be useful to many others. In this regard, my latest commit removed the need to manually build Regarding Regarding the changes to |
Hello, First of all, thank you all for your effort in this utility. I want to report that, when using this PR, an issue arises. Issue: [mqtt_client-1] terminate called after throwing an instance of 'eprosima::fastcdr::exception::NotEnoughMemoryException' You can reproduce this error easily launching the mqtt_client with custom params and doing in another terminal:
|
Hi there! |
The main problem that I see right now is the official release process to the ROS package indices. For sure, people could clone As long as the changes to This is also the main reason that I have been reluctant with conducting a thorough code review right now. I would postpone that until a release becomes feasible. What I could offer is:
|
Hello there, Could you give me hints ? |
Hello, I’m facing the same issue with the rosx_introspection package. I get the error about missing rosx_introspectionConfig.cmake rosx_introspection-config.cmake after running catkin_make. Any clues for this issues ? |
@LuCarpentier92 no luck, i switch to mqtt_bridge and wait for this PR to be developed in futureand hope the auther will give us some update. |
This PR integrates functionality for a full communication with other (non-mqtt_client) MQTT clients. Unlike the current feature of exchanging primitive messages, this feature allows ROS-based devices to exchange arbitrary messages with devices not based on ROS. It works in both directions, where a non-ROS-based device can send a JSON string that will be casted into a ROS message in runtime.
The PR is based on my updates to ros_msg_parser package that uses shapeshifter to provide type introspection in ROS, my contribution was to update the parser class to convert a JSON string into a serialized ROS message, achieving bi-directional parsing capabilities between JSON <-> ROS-msg.
The README is updated to explain the new functionality, and its demo and technical details are added in the corresponding sections.
This PR should close #9 and set the ground ready for working on #18.