[WIP] ROS 2 JSON publishing support #288

jhurliman · 2024-02-23T04:39:47Z

Public-Facing Changes

Clients to the ROS 2 bridge can publish messages as JSON instead of cdr. JSON payloads will be re-serialized to cdr by the bridge before publishing to the ROS 2 graph

Description

This PR vendors in the unpublished https://github.com/LOEWE-emergenCITY/ros2_babel_fish library and uses it to support JSON message publishing from WebSocket clients. Incoming JSON messages are parsed, transformed into native ROS 2 messages field-by-field, then published to the ROS 2 graph.

TODO

Test with empty {} JSON message, simple message types
Test compound messages, Time/Duration fields, etc
Array support
Solve ROS Iron / Rolling support (disable with #ifdef?)
Add test coverage
Update docs

…ish()

… unit tests

…tainer

ros2_babel_fish is not compatible with Iron, which needs a different implementation using new native rclcpp functionality

achim-k

It would be nice if we could avoid having to copy all the babel fish code. The humble release is currently blocked tho (see LOEWE-emergenCITY/ros_babel_fish#7). Maybe we could also avoid copying all that code with some git submodule / cmake magic?

In general, this approach (with babel fish) should also work for more recent distros (iron, rolling), right?

achim-k · 2024-04-05T15:45:24Z

.devcontainer/Dockerfile

+# Set clang*-15 as the default
+RUN update-alternatives --install /usr/bin/clang clang /usr/bin/clang-15 100
+RUN update-alternatives --install /usr/bin/clangd clangd /usr/bin/clangd-15 100
+RUN update-alternatives --install /usr/bin/clang-format clang-format /usr/bin/clang-format-15 100
+


Any reason for preferring this over default clang install?

achim-k · 2024-04-05T15:52:32Z

ros2_foxglove_bridge/src/ros2_foxglove_bridge.cpp

@@ -61,7 +68,7 @@ FoxgloveBridge::FoxgloveBridge(const rclcpp::NodeOptions& options)
  if (_useSimTime) {
    serverOptions.capabilities.push_back(foxglove::CAPABILITY_TIME);
  }
-  serverOptions.supportedEncodings = {"cdr"};
+  serverOptions.supportedEncodings = {"cdr", "json"};


I guess that should depend on ENABLE_JSON_MESSAGES as well?

achim-k · 2024-04-05T16:51:05Z

foxglove_bridge_base/include/foxglove_bridge/websocket_server.hpp

@@ -759,10 +759,11 @@ inline void Server<ServerConfiguration>::handleBinaryMessage(ConnHandle hdl, Mes
                                          length,
                                          data};
        _handlers.clientMessageHandler(clientMessage, hdl);
-      } catch (const ServiceError& e) {
+      } catch (ServiceError const& e) {


does clang-format-15 introduce this change?

achim-k · 2024-04-05T16:53:23Z

foxglove_bridge_base/include/foxglove_bridge/websocket_server.hpp

        sendStatusAndLogMsg(hdl, StatusLevel::Error, e.what());
-      } catch (...) {
-        sendStatusAndLogMsg(hdl, StatusLevel::Error, "callService: Failed to execute handler");
+      } catch (std::exception const& e) {


You can theoretically throw anything (e.g. also throw 2 is valid) so I would leave the catch-all handler

achim-k · 2024-04-11T18:11:54Z

Dockerfile.ros2

@@ -10,7 +10,7 @@ ENV CC=clang
 ENV CXX=clang++

 # Set environment and working directory
-ENV ROS_WS /ros2_ws
+ENV ROS_WS /ros_ws


Could we split all the dev environment changes out into a seperate PR?

achim-k · 2024-04-11T18:20:12Z

ros2_foxglove_bridge/include/foxglove_bridge/ros2_foxglove_bridge.hpp

+#ifdef ENABLE_JSON_MESSAGES
+#include <ros2_babel_fish/babel_fish.hpp>
+#endif


Since below we only have a pointer to the babel fish instance, we could move this include to the cpp file and replace it here with a forward declaration. However, there is probably no one (besides ros2_foxglove_bridge.cpp) that includes this file, so this is not something important.

I agree, forward declaration might be desirable. Altenrative, we can use the Pimpl idiom.

Just one additional question: in which cases will a user prefer to compile this with ENABLE_JSON_MESSAGES undefined?

achim-k · 2024-04-11T20:54:01Z

ros2_foxglove_bridge/include/ros2_babel_fish/babel_fish.hpp

+#include "ros2_babel_fish/detail/babel_fish_action_client.hpp"
+#include "ros2_babel_fish/detail/babel_fish_publisher.hpp"
+#include "ros2_babel_fish/detail/babel_fish_service.hpp"
+#include "ros2_babel_fish/detail/babel_fish_service_client.hpp"
+#include "ros2_babel_fish/detail/babel_fish_subscription.hpp"
+#include "ros2_babel_fish/idl/type_support_provider.hpp"
+#include "ros2_babel_fish/messages/array_message.hpp"
+#include "ros2_babel_fish/messages/compound_message.hpp"
+#include "ros2_babel_fish/messages/value_message.hpp"


Should we weed out some of these that are not strictly required? E.g. I guess the action client could go

achim-k · 2024-04-11T21:14:26Z

ros2_foxglove_bridge/src/json_to_ros.cpp

+    case MessageType::Array: {
+      // Ensure the JSON value is an array
+      if (!value.is_array()) {
+        break;


shouldn't we throw an exception in that case?

achim-k · 2024-04-11T21:18:43Z

ros2_foxglove_bridge/src/json_to_ros.cpp

+  nlohmann::json json;
+  try {
+    json = nlohmann::json::parse(jsonMessage);
+  } catch (const nlohmann::json::parse_error& e) {
+    return e;
+  }
+
+  // Convert the JSON message to a ROS message using ros2_babel_fish
+  ros2_babel_fish::CompoundMessage::SharedPtr rosMsgPtr;
+  try {
+    rosMsgPtr = babelFish->create_message_shared(schemaName);
+  } catch (const ros2_babel_fish::BabelFishException& e) {
+    return e;
+  }
+  auto& rosMsg = *rosMsgPtr;
+  for (const auto& key : rosMsg.keys()) {
+    if (!json.contains(key)) {
+      continue;
+    }
+
+    try {
+      assignJsonToMessageField(rosMsg[key], json[key]);
+    } catch (const std::exception& e) {
+      return e;
+    }
+  }


that's a lot of try-catch blocks. Having only one try-catch block should make it more readable

Or related to my previous comment: Don't catch exceptions here and return the output message rather than the std::optional

achim-k · 2024-04-11T21:21:44Z

ros2_foxglove_bridge/tests/json_to_ros_test.cpp

+  auto babelFish = ros2_babel_fish::BabelFish::make_shared();
+
+  ros2_babel_fish::CompoundMessage::SharedPtr output;
+  auto res = foxglove_bridge::jsonMessageToRos(payload, "std_msgs/msg/String", babelFish, output);


Can you add a test for a more complex message type? E.g. geometry_msgs/PoseStamped or something similar, ideally containing an array.

jhurliman · 2024-04-12T00:06:12Z

It would be nice if we could avoid having to copy all the babel fish code. The humble release is currently blocked tho (see LOEWE-emergenCITY/ros2_babel_fish#7).

"Currently" is underselling it. Now that this module has been superseded by native ROS 2 API methods in Iron+, it is deprecated and will never be released.

Maybe we could also avoid copying all that code with some git submodule / cmake magic?

My understanding is git submodules and cmake magic are generally frowned upon for ROS / ROS 2 projects that will be built and distributed in the ROS build system and third party developers. If you have a reference example in mind I can take a look.

In general, this approach (with babel fish) should also work for more recent distros (iron, rolling), right?

No, babel fish does not build in Iron+ and has been replaced by API methods. It will require a different (albeit better) approach.

achim-k · 2024-04-12T14:21:30Z

Maybe we could also avoid copying all that code with some git submodule / cmake magic?

My understanding is git submodules and cmake magic are generally frowned upon for ROS / ROS 2 projects that will be built and distributed in the ROS build system and third party developers. If you have a reference example in mind I can take a look.

The mcap_vendor package uses FetchContent:
https://github.com/ros2/rosbag2/blob/bee10b49e6946c9467a421898100f5c93a49b246/mcap_vendor/CMakeLists.txt#L35-L47

StefanFabian · 2024-05-09T15:05:53Z

In general, this approach (with babel fish) should also work for more recent distros (iron, rolling), right?

No, babel fish does not build in Iron+ and has been replaced by API methods. It will require a different (albeit better) approach.

Just read this by chance and would like to say that it does in fact build in jazzy after some small changes addressing API changes. I'm also interested in the API methods that replace its functionality as I couldn't find anything about that.
There's a GenericSubscription and GenericPublisher now but they seem to only work with raw data messages which is the smallest issue ros2_babel_fish had to solve.
You still have to load the required message library and use the introspection api (which has been around since foxy or earlier) to interpret these messages which is the main challenge ros2_babel_fish is simplifying.
If there's an easier API now, I would be very interested in that.

facontidavide · 2024-05-22T16:34:38Z

What is blocking this PR from being merged? @StefanFabian correct me if I am wrong, but ros2_bable_fish haven't been released as a package, right? Maybe we should start fixing that?

StefanFabian · 2024-05-27T06:58:10Z

No, it hasn't been released yet.
As jazzy is the first distro (except for foxy which I find kind of funny) where my fixes are merged (or LTS distro, not sure if it's already in Iron), I didn't release it for the older distros.
We are starting the switch to ROS2 in our team this week and I will prepare the release in the next weeks.
I can also release it for the older distros when I do so, you don't need the part affected by the PRs anyway.

defunctzombie · 2024-07-29T16:21:44Z

Replaced by #307

jhurliman added 4 commits February 22, 2024 19:26

Fix ROS_WS path in Dockerfiles

896bcbd

Vendor ros2_babel_fish code and add to ROS 2 build

bdc8882

Enable -fPIC for ros2_babel_fish.a

1003fde

Convert JSON messages from clients to ROS 2 native (cdr) and rcl_publ…

965ad15

…ish()

defunctzombie marked this pull request as draft February 23, 2024 17:08

jhurliman added 6 commits February 26, 2024 21:55

lint

266a743

Fix an incorrect log message

1e95d28

Fix JSON message parsing, array support, refactor into separate file,…

1f7a2fe

… unit tests

lint

2310f75

Use clangd vscode extension, simplify switching ROS version in devcon…

aaa13c4

…tainer

Conditionally enable JSON message publishing for Humble only

3c7e2e0

ros2_babel_fish is not compatible with Iron, which needs a different implementation using new native rclcpp functionality

achim-k reviewed Apr 11, 2024

View reviewed changes

facontidavide mentioned this pull request Jun 16, 2024

add "rosx_introspection" to support JSON encoding #307

Merged

defunctzombie closed this Jul 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] ROS 2 JSON publishing support #288

[WIP] ROS 2 JSON publishing support #288

jhurliman commented Feb 23, 2024 •

edited

Loading

achim-k left a comment

achim-k Apr 5, 2024

achim-k Apr 5, 2024

achim-k Apr 5, 2024

achim-k Apr 5, 2024

achim-k Apr 11, 2024

achim-k Apr 11, 2024

facontidavide May 22, 2024

achim-k Apr 11, 2024

achim-k Apr 11, 2024

achim-k Apr 11, 2024

achim-k Apr 11, 2024

achim-k Apr 11, 2024

jhurliman commented Apr 12, 2024

achim-k commented Apr 12, 2024

StefanFabian commented May 9, 2024

facontidavide commented May 22, 2024

StefanFabian commented May 27, 2024

defunctzombie commented Jul 29, 2024

[WIP] ROS 2 JSON publishing support #288

[WIP] ROS 2 JSON publishing support #288

Conversation

jhurliman commented Feb 23, 2024 • edited Loading

Public-Facing Changes

Description

TODO

achim-k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhurliman commented Apr 12, 2024

achim-k commented Apr 12, 2024

StefanFabian commented May 9, 2024

facontidavide commented May 22, 2024

StefanFabian commented May 27, 2024

defunctzombie commented Jul 29, 2024

jhurliman commented Feb 23, 2024 •

edited

Loading