foxglove / ros-foxglove-bridge

Foxglove WebSocket bridge for ROS 1 and ROS 2
MIT License
162 stars 71 forks source link

add "rosx_introspection" to support JSON encoding #307

Closed facontidavide closed 3 months ago

facontidavide commented 4 months ago

Description

This is an alternative to #288 using rosx_introspection instead.

Note that this requires the latest changes in the main branch, more specifically: https://github.com/facontidavide/rosx_introspection/commit/b4c5a0db17a494a7de5d2c9469d83e2787fe7768

rosx_introspection will be release soon on all the ROS2 distros and, potentially, Noetic too. This will allow us to add this functionality to the ROS1 bridge too, in a follow-up PR.

IMPORTANT: the JSON parser support missing fields (in that case, default values will be used).

CLAassistant commented 4 months ago

CLA assistant check
All committers have signed the CLA.

facontidavide commented 4 months ago

I know that CI is failing. I started the official process to release rosx_introspection, as you can see here, and it may take a while.

facontidavide commented 4 months ago

Well, that was a feature 😅 Sure, I can make it more strict

jhurliman commented 4 months ago

which is somewhat unexpected.

Could you explain more about your use case where this is unexpected?

achim-k commented 4 months ago

Well, that was a feature 😅 Sure, I can make it more strict

I think it's great, but it would be nice if one could enable/disable it via a ROS parameter if that is easily doable. If it's too much work then I think it's fine to keep it.

Alternatively, another solution would be to raise an error if the JSON object contains fields that do not appear in the ROS message. This would then still work for a empty JSON object but not for a {data: "garbage"} object.

facontidavide commented 3 months ago

Conflict fixed and Linter happy. I believe you can test it on your side and consider merging

facontidavide commented 3 months ago

As a side note, there was a typo in your JSOn message definition (field "nanosec", not "nsec").

I found the problem and it has been fixed in rosx_introspection 1.0.2

I also added a workaround in my latest commit, plus some other minor changes.

About your suggested changes in the mutex, I believe the current one is correct.

achim-k commented 3 months ago

I just pushed a few more commits to add unit tests and to initialize JSOn parsers only when a client channel is advertised. Before this was done for all published topics on the system. This was also the reason for the mutex confusion.

Changes looks good now and can be merged. Not sure why the license/cla check is still pending, I saw that you signed it before :thinking:

As a side note, there was a typo in your JSOn message definition (field "nanosec", not "nsec").

Indeed. This is some peculiarity with how Foxglove handles header stamps. We will have to fix this on Foxglove side.