Solved bug with incompatible QoS policies #276
Conversation
| find_path(WEBSOCKETPP_INCLUDE_DIR NAMES websocketpp/websocketpp.hpp) | ||
| message(STATUS "WEBSOCKETPP_INCLUDE_DIR: " ${WEBSOCKETPP_INCLUDE_DIR}) |
There was a problem hiding this comment.
This should not be necessary, WEBSOCKETPP_INCLUDE_DIR should be populated by find_package(websocketpp REQUIRED) (it should fail otherwise)
There was a problem hiding this comment.
For some reason, when building ros-foxglove-bridge with a ROS 2 installation from robostack (in a conda env), CMake/clang installed in the same env, and with websocketpp installed globally via homebrew or apt, this variable was not populated correctly. However, if I install websocketpp in the conda env, it works just fine.
I will try to understand further the bug
There was a problem hiding this comment.
You could also look into adapting the robotstack foxglove_bridge patch. That patch might need some updating as it's relatively old, but building on MacOS worked back then
| @@ -81,6 +83,7 @@ target_include_directories(foxglove_bridge_base | |||
| $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/foxglove_bridge_base/include> | |||
| $<INSTALL_INTERFACE:include> | |||
| ) | |||
| target_include_directories(foxglove_bridge_base SYSTEM PUBLIC ${WEBSOCKETPP_INCLUDE_DIR}) | |||
There was a problem hiding this comment.
No need for a new target_include_directories, you can add it to the one above
| // If there are no other publishers, we can't determine anything on reliability or durability, | ||
| // so we keep the system default. | ||
| if (publisherInfo.size() == 0) { | ||
| RCLCPP_WARN(this->get_logger(), | ||
| "No publishers on topic '%s'. Falling back to QoSReliabilityPolicy.RELIABLE as it " | ||
| "will connect to all publishers", | ||
| topic.c_str()); | ||
| } else { | ||
| if (reliabilityReliableEndpointsCount > 0) { | ||
| RCLCPP_WARN( | ||
| this->get_logger(), | ||
| "Some, but not all, publishers on topic '%s' are offering QoSReliabilityPolicy.RELIABLE. " | ||
| "Falling back to QoSReliabilityPolicy.BEST_EFFORT as it will connect to all publishers", | ||
| topic.c_str()); | ||
| // If all endpoints are reliable, ask for reliable | ||
| if (reliabilityReliableEndpointsCount == publisherInfo.size()) { | ||
| qos.reliable(); | ||
| } else { | ||
| if (reliabilityReliableEndpointsCount > 0) { | ||
| RCLCPP_WARN( | ||
| this->get_logger(), | ||
| "Some, but not all, publishers on topic '%s' are offering QoSReliabilityPolicy.RELIABLE. " | ||
| "Falling back to QoSReliabilityPolicy.BEST_EFFORT as it will connect to all publishers", | ||
| topic.c_str()); | ||
| } | ||
| qos.best_effort(); | ||
| } | ||
| qos.best_effort(); | ||
| } | ||
|
|
||
| // If all endpoints are transient_local, ask for transient_local | ||
| if (durabilityTransientLocalEndpointsCount == publisherInfo.size()) { | ||
| qos.transient_local(); | ||
| } else { | ||
| if (durabilityTransientLocalEndpointsCount > 0) { | ||
| RCLCPP_WARN(this->get_logger(), | ||
| "Some, but not all, publishers on topic '%s' are offering " | ||
| "QoSDurabilityPolicy.TRANSIENT_LOCAL. Falling back to " | ||
| "QoSDurabilityPolicy.VOLATILE as it will connect to all publishers", | ||
| topic.c_str()); | ||
| // If all endpoints are transient_local, ask for transient_local | ||
| if (durabilityTransientLocalEndpointsCount == publisherInfo.size()) { | ||
| qos.transient_local(); | ||
| } else { | ||
| if (durabilityTransientLocalEndpointsCount > 0) { | ||
| RCLCPP_WARN(this->get_logger(), | ||
| "Some, but not all, publishers on topic '%s' are offering " | ||
| "QoSDurabilityPolicy.TRANSIENT_LOCAL. Falling back to " | ||
| "QoSDurabilityPolicy.VOLATILE as it will connect to all publishers", | ||
| topic.c_str()); | ||
| } | ||
| qos.durability_volatile(); |
There was a problem hiding this comment.
I think here you could just add !publisherInfo.empty() && to the if conditions.
if (!publisherInfo.empty() && reliabilityReliableEndpointsCount == publisherInfo.size()) {
} else {
// best effort (compatible with all reliability QoS policies)
}
if (!publisherInfo.empty() && durabilityTransientLocalEndpointsCount == publisherInfo.size()) {
} else {
// durability_volatile (compatible with all durability QoS policies)
}There was a problem hiding this comment.
Yes, you are right. I just added a separate if condition to call RCLCPP_WARN. I will remove it in the next PR
|
I am not sure why the CI failed tho. It did so with the first other PR I created: #277 |
Build failures are related to osrf/docker_images#697. Might take a bit until the base docker images are rebuild to include the updated key. If this doesn't happen, we will have to do that manually in our ROS1/ROS2 Dockerfiles. |
|
@tudoroancea are you still working on this? |
|
@tudoroancea since both mentioned PRs are in, this can be closed now? |
FYI the ROS images on dockerhub have been rebuilt with the updated GPG key. |
Public-Facing Changes
None
Description
I changed 3 files for 3 different reasons:
CMakeLists.txt: fixed build issue with websocket++ that wasn't found properly. The issue was also mentioned in this other PR. In comparison with it, I did not add the/usr/local/includethat wasn't cross-platform.ros2_foxglove_bridge/src/parameter_interface.cpp: line 97 raised an error when compiling on macOS. My guess is that the default clang compilation flags don't allow implicit calls to constructors.ros2_foxglove_bridge/src/ros2_foxglove_bridge.cpp: this solves the following issue.