-
Notifications
You must be signed in to change notification settings - Fork 84
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
[WIP] ROS 2 JSON publishing support #288
Changes from all commits
896bcbd
bdc8882
1003fde
965ad15
266a743
1e95d28
1f7a2fe
2310f75
aaa13c4
3c7e2e0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we split all the dev environment changes out into a seperate PR? |
||
WORKDIR $ROS_WS | ||
|
||
# Install system dependencies | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does clang-format-15 introduce this change? |
||
sendStatusAndLogMsg(hdl, StatusLevel::Error, e.what()); | ||
} catch (...) { | ||
sendStatusAndLogMsg(hdl, StatusLevel::Error, "callService: Failed to execute handler"); | ||
} catch (std::exception const& e) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can theoretically throw anything (e.g. also |
||
sendStatusAndLogMsg(hdl, StatusLevel::Error, | ||
std::string{"Failed to execute client message handler: "} + e.what()); | ||
} | ||
} break; | ||
case ClientBinaryOpcode::SERVICE_CALL_REQUEST: { | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,21 @@ | ||||||
#pragma once | ||||||
|
||||||
#include <optional> | ||||||
|
||||||
#include <ros2_babel_fish/babel_fish.hpp> | ||||||
|
||||||
namespace foxglove_bridge { | ||||||
|
||||||
/** | ||||||
* Convert a JSON-serialized message with a given named schema to a ROS message | ||||||
* using ros2_babel_fish. The output message is allocated as a shared pointer | ||||||
* and assigned to the outputMessage argument. The return value is an optional | ||||||
* exception, which if set indicates that an error occurred during the | ||||||
* conversion and `outputMessage` is not valid. | ||||||
*/ | ||||||
std::optional<std::exception> jsonMessageToRos( | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find this function signature a little odd. How about just throwing an exception in the error case and otherwise returning |
||||||
const std::string_view jsonMessage, const std::string& schemaName, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: could pass the string view by ref as well
Suggested change
|
||||||
ros2_babel_fish::BabelFish::SharedPtr babelFish, | ||||||
ros2_babel_fish::CompoundMessage::SharedPtr& outputMessage); | ||||||
|
||||||
} // namespace foxglove_bridge |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,10 @@ | |
#include <foxglove_bridge/server_factory.hpp> | ||
#include <foxglove_bridge/utils.hpp> | ||
|
||
#ifdef ENABLE_JSON_MESSAGES | ||
#include <ros2_babel_fish/babel_fish.hpp> | ||
#endif | ||
Comment on lines
+23
to
+25
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
|
||
namespace foxglove_bridge { | ||
|
||
using ConnectionHandle = websocketpp::connection_hdl; | ||
|
@@ -82,6 +86,9 @@ class FoxgloveBridge : public rclcpp::Node { | |
std::atomic<bool> _subscribeGraphUpdates = false; | ||
bool _includeHidden = false; | ||
std::unique_ptr<foxglove::CallbackQueue> _fetchAssetQueue; | ||
#ifdef ENABLE_JSON_MESSAGES | ||
ros2_babel_fish::BabelFish::SharedPtr _babelFish; | ||
#endif | ||
|
||
void subscribeConnectionGraph(bool subscribe); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for preferring this over default clang install?