ros2 - Adaptaions to Eloquent #493

mmatthe · 2020-04-28T09:07:18Z

During usage of the ROS2 branch of the rosbridge_suite, several problems occured in the Python Code. These were fixed such that it works for our use case. The overview of the changes:

allow an increased subscribing frequency of 1000Hz (it was 10Hz, meaning only 10 ROS2 messages were received per second)
Speed up receiving CompressedImage from the websocket by creating a shortcut implementation with some optimizations
allow both nsecs and nanosec fields for time (ROS1/2 compatiblity).
allow received int values to be interpreted as float values, when needed.

- adapt rosbridge_suite to ROS2 Eloquent - increase spin period to 1000Hz to allow 1000 messages per second into the websocket - increase parsing speed of sensor_msgs/CompressedImage - add some debug messages - allow interpreting int as float when needed - better handling array.array and numpy arrays - allow bytes and str websocket messages

@joshwapohlmann

- adapt rosbridge_suite to ROS2 Eloquent - increase spin period to 1000Hz to allow 1000 messages per second into the websocket - increase parsing speed of sensor_msgs/CompressedImage - add some debug messages - allow interpreting int as float when needed - better handling array.array and numpy arrays - allow bytes and str websocket messages Contributors: @joshwapohlmann @mmatthe

…ge_suite into ros2

mvollrath · 2020-05-13T20:52:48Z

I'm happy to merge this with a review by a ROS2 user.

flynneva · 2020-05-13T21:15:25Z

Tested and confirmed on Ubuntu 18.04 with ROS2 Eloquent.
Compiled, no issues.

NOTE: to run, I had to pip install pymongo before using rosbridge_websocket.

tested with webviz.io resulted in a service call error. I think the service that webviz was looking for just was missing.

tested on a less-complex website and it worked great.

flynneva · 2020-05-13T21:25:00Z

just tested it with dashing too...works great

mvollrath · 2020-05-13T21:49:57Z

Please try removing the pip-installed pymongo package and test with apt package python3-bson, because this is what will be installed for BSON library.

It looks like the webviz error would go away if the rosapi node (part of rosbridge) were running.

flynneva · 2020-05-13T23:01:12Z

followed your recommendation - uninstalled the pip pymongo and installed the apt package python3-bson.

same as before, compiled and worked great.

I also tried to launch the rosapi node in another terminal, however it looks like the service /rosapi/topics_and_raw_types that webviz is looking for isnt available. it probably wasn't carried over to the ros2 version of rosapi yet.

mvollrath · 2020-05-13T23:04:12Z

Is there something new in ROS2 where having a requirements.txt is either supported or desired?

mvollrath · 2020-05-14T02:43:30Z

rosbridge_library/src/rosbridge_library/internal/message_conversion.py


 def populate_instance(msg, inst):
    """ Returns an instance of the provided class, with its fields populated
    according to the values in msg """
    inst_type = msg_instance_type_repr(inst)
+    if inst_type == "sensor_msgs/CompressedImage":


I'm not convinced yet that hardcoding special handling for specific message types in this module in this way is a good idea.

If the data is coming in as anything but a base64 string, it shouldn't be coming anywhere near this code path. I don't see any place where base64 decoding was added, so I don't understand how this decoder could possibly work. Can you explain how this works?

I agree that this is kind of a hacky solution to shortcut decoding of CompressedImage. At this point in the data flow, msg is a dict containing all the fields. In particular, arrays/lists have already been decoded into python/numpy arrays. From this point in the data flow on, the only remaining thing is to translate the fields from the dict into the attributes of inst (inst is already the target object of the correct type). in our debugging sessions it happened that decoding of CompressedImages was extremely slow. This is due to an known problem within the python-idl mapping. Particular, inst.data = msg["data"] would be the slow thing. Instead, it should be inst.data = array.array("B", msg["data"]) which omits the runtime check if all values are within bounds.

Since the generic decoding within populate_instance can only do inst.data = msg["data"], I have included the shortcut here.

Please let me know, if I should delete it in this fork or keep it.

It seems like this could be made into a general solution by moving it into _to_inst with the rest of the decoding. Otherwise I would accept this hack in this PR if there is a comment linking to your comment explaining why it exists.

I have created an extra file that collects shortcut decoders. Currently, it only contains a decoder for sensor_msgs/CompressedImage. I needed that file to keep the message_conversion.py clean. I have moved the call to these function into the _to_object_inst

Please see, if this solution is fine with you.

requirements.txt

rosbridge_library/src/rosbridge_library/internal/message_conversion.py

rosbridge_library/src/rosbridge_library/internal/subscribers.py

rosbridge_library/src/rosbridge_library/protocol.py

- remove extraneous print statements - remove unused packages - remove whitespace - move message conversion to str into websocket_handler

Adding a dict `shortcut_object_decoders` that contains functions that handles decoding of specific objects quicker than the generic method. remove requirements.txt Co-authored-by: joshwapohlmann <joshwa.pohlmann@barkhauseninstitut.org>

Co-Authored-by: joshwapohlmann <joshwa.pohlmann@barkhauseninstitut.org>

flynneva · 2020-05-18T23:40:40Z

just tested this for foxy on ubuntu 20.04 - works there too!

maxlein · 2020-05-20T10:32:06Z

I am confused on which ros2 bridge I shall use now!?
This one on ros2 branch or ros2-web-bridge?
Seems both are missing topics_and_raw_types service for usage with webviz.
Am I missing something? Are they dependent on each other somehow?

flynneva · 2020-05-20T18:27:50Z

either one! this ones based on python and the ros2-web-bridge one is based on js. they provide similar functionality

the maintainer provides an answer to this question here RobotWebTools/ros2-web-bridge#117

mvollrath · 2020-05-21T01:25:05Z

Seems both are missing topics_and_raw_types service for usage with webviz.

IIRC this service is provided by rosapi which is a separate script under rosbridge_suite. I don't know if rosapi has been ported for ROS2 yet.

mvollrath · 2020-05-21T01:31:03Z

rosbridge_library/src/rosbridge_library/internal/object_decoders.py

+    # explicitely cast to array.array to overcome a costly runtime
+    # check within inst.data instance.
+    # See also https://github.com/RobotWebTools/rosbridge_suite/pull/493#discussion_r425151919
+    inst.data = array.array("B", msg["data"])


Is it not true that if you did this for all uint8[] in message_conversion.py _to_binary_inst you wouldn't need special cases for specific message types?

Well, you can, in the deeply buried code where the instance is set (it'd be in _to_object_inst and _to_list_inst), capture if a uint8[] is set and then apply the optimization. However, this is not very clear to understand and it might be too general, as sometimes you might want to have the additional error check that is done by ROS2 idl_typesupport. Therefore, I would not recommend using this optimization generally.

I don't see how maintaining unique decoder logic for specific message types is better. Everything about rosbridge is general and should remain general. If the usual decoding pathway is too slow for CompressedImage it's probably too slow for every other message that uses uint8[]. If the additional error check can be bypassed for CompressedImage it can probably be bypassed for every other message type.

If there is an open issue about ROS2 message conversion performance somewhere else please link.

travipross · 2020-08-05T01:50:45Z

Also compiled and tested on Ubuntu 18.04.3 with ROS Eloquent. Seems to work 👍

AviKenz · 2020-08-06T23:24:49Z

Tested and confirmed on Ubuntu 18.04 with ROS2 Eloquent.
Compiled, no issues.

NOTE: to run, I had to pip install pymongo before using rosbridge_websocket.

tested with webviz.io resulted in a service call error. I think the service that webviz was looking for just was missing.

tested on a less-complex website and it worked great.

Hello !
i am getting similar output and i CANT publish anything from any Client library.
i already tried roslibjs and jrosbridge. same errors...

Here is the issue i opened: rctoris/jrosbridge#29

I need help pleaaaaase....

im running ros-dashing.

Thanks in advance !

AviKenz · 2020-08-06T23:26:34Z

just tested it with dashing too...works great

im using dashing too. but nothing works.

my issue: rctoris/jrosbridge#29

need help !

flynneva · 2020-08-10T04:00:16Z

@mmatthe if you make one more small commit on your PR branch it should trigger the new ROS2 CI for this PR. might be helpful in diagnosing if some of these fixes help solve some of the compilation issues.

travipross · 2020-08-27T17:15:24Z

@mmatthe I've opened an additional PR in your fork for a simple addition to this PR regarding boolean types.

@mvollrath Is the only outstanding issue holding up the approval of this PR related to the addition of the shortcut decoders in rosbridge_library/src/rosbridge_library/internal/object_decoders.py?

If so, is there any way we can move that portion to a separate PR for further discussion? There are some other fixes in this branch that address some currently broken functionality in the base branch, and it'd be nice to see this merged.

mvollrath · 2020-09-08T16:20:59Z

@mvollrath Is the only outstanding issue holding up the approval of this PR related to the addition of the shortcut decoders in rosbridge_library/src/rosbridge_library/internal/object_decoders.py?

Yes.

travipross · 2020-09-08T21:15:32Z

Is there any way we can just comment/remove the two offending lines relating to checking for / returning a shortcut decoder, and potentially discuss their reintroduction in a different PR if desired?

It'd be nice to see the majority of this moved into the ros2 branch, as in its current state, many features are still broken and this contains some fixes.

I don't want to clutter things and open a new PR containing most of these same changes with this PR still open, but it seems @mmatthe might not be actively monitoring this PR right now.

AviKenz · 2020-09-18T12:32:55Z

@mvollrath its seems that the change request was already proceeded. Please check object_decoders.py.
let me know if its enough or not. i will proceed the changes on my PR :)

travipross · 2020-09-19T00:53:26Z

@mvollrath I've removed the offending shortcut_object_decoders in #533 as well as made a number of other small, but missing adaptations. I suggest we close this PR as @mmatthe seems to have abandoned it for now.

travipross · 2021-06-13T21:02:17Z

@mvollrath It looks as though this has sat abandoned by the original author for almost a year and can be closed thanks to #533

mmatthe added 3 commits April 28, 2020 11:03

Merge branch 'ros2' of https://github.com/Barkhausen-Institut/rosbrid…

219d31c

…ge_suite into ros2

mvollrath requested changes May 14, 2020

View reviewed changes

mmatthe and others added 3 commits May 14, 2020 16:17

code review changes

b4cf2bc

- remove extraneous print statements - remove unused packages - remove whitespace - move message conversion to str into websocket_handler

Refactor shortcut decoding into an extra file.

3874e2f

Adding a dict `shortcut_object_decoders` that contains functions that handles decoding of specific objects quicker than the generic method. remove requirements.txt Co-authored-by: joshwapohlmann <joshwa.pohlmann@barkhauseninstitut.org>

Add explanation on faster decoding of compressed Image

ad0008d

Co-Authored-by: joshwapohlmann <joshwa.pohlmann@barkhauseninstitut.org>

mvollrath reviewed May 21, 2020

View reviewed changes

travipross mentioned this pull request Aug 27, 2020

Add boolean type Barkhausen-Institut/rosbridge_suite#1

Closed

travipross mentioned this pull request Aug 28, 2020

Subsriber fix RessonAerospace/rosbridge_suite#1

Merged

mvollrath mentioned this pull request Sep 14, 2020

FieldTypeMismatchException with "float32" fields #530

Closed

AviKenz mentioned this pull request Sep 18, 2020

[NEW] ros2 - Adaptaions to Eloquent #531

Closed

travipross mentioned this pull request Sep 19, 2020

Fixes for Eloquent #532

Closed

travipross mentioned this pull request Sep 19, 2020

Adaptations to Eloquent [Again] #533

Merged

jihoonl closed this Jun 14, 2021

kenji-miyake mentioned this pull request Feb 12, 2022

Fix time behavior #691

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ros2 - Adaptaions to Eloquent #493

ros2 - Adaptaions to Eloquent #493

mmatthe commented Apr 28, 2020

mvollrath commented May 13, 2020

flynneva commented May 13, 2020 •

edited

Loading

flynneva commented May 13, 2020

mvollrath commented May 13, 2020

flynneva commented May 13, 2020

mvollrath commented May 13, 2020

mvollrath May 14, 2020

mmatthe May 14, 2020

mvollrath May 15, 2020

mmatthe May 18, 2020

flynneva commented May 18, 2020

maxlein commented May 20, 2020

flynneva commented May 20, 2020 •

edited

Loading

mvollrath commented May 21, 2020

mvollrath May 21, 2020

mmatthe May 24, 2020 •

edited

Loading

mvollrath May 26, 2020

travipross commented Aug 5, 2020

AviKenz commented Aug 6, 2020

AviKenz commented Aug 6, 2020

flynneva commented Aug 10, 2020

travipross commented Aug 27, 2020

mvollrath commented Sep 8, 2020

travipross commented Sep 8, 2020

AviKenz commented Sep 18, 2020

travipross commented Sep 19, 2020 •

edited

Loading

travipross commented Jun 13, 2021

ros2 - Adaptaions to Eloquent #493

ros2 - Adaptaions to Eloquent #493

Conversation

mmatthe commented Apr 28, 2020

mvollrath commented May 13, 2020

flynneva commented May 13, 2020 • edited Loading

flynneva commented May 13, 2020

mvollrath commented May 13, 2020

flynneva commented May 13, 2020

mvollrath commented May 13, 2020

mvollrath May 14, 2020

Choose a reason for hiding this comment

mmatthe May 14, 2020

Choose a reason for hiding this comment

mvollrath May 15, 2020

Choose a reason for hiding this comment

mmatthe May 18, 2020

Choose a reason for hiding this comment

flynneva commented May 18, 2020

maxlein commented May 20, 2020

flynneva commented May 20, 2020 • edited Loading

mvollrath commented May 21, 2020

mvollrath May 21, 2020

Choose a reason for hiding this comment

mmatthe May 24, 2020 • edited Loading

Choose a reason for hiding this comment

mvollrath May 26, 2020

Choose a reason for hiding this comment

travipross commented Aug 5, 2020

AviKenz commented Aug 6, 2020

AviKenz commented Aug 6, 2020

flynneva commented Aug 10, 2020

travipross commented Aug 27, 2020

mvollrath commented Sep 8, 2020

travipross commented Sep 8, 2020

AviKenz commented Sep 18, 2020

travipross commented Sep 19, 2020 • edited Loading

travipross commented Jun 13, 2021

flynneva commented May 13, 2020 •

edited

Loading

flynneva commented May 20, 2020 •

edited

Loading

mmatthe May 24, 2020 •

edited

Loading

travipross commented Sep 19, 2020 •

edited

Loading