Skip to content

Added draco node-level transport pub/sub - #91

Open
ahcorde wants to merge 2 commits into
ahcorde/rolling/draco_test_round_tripfrom
ahcorde/rolling/draco_test_round_trip_transport
Open

Added draco node-level transport pub/sub#91
ahcorde wants to merge 2 commits into
ahcorde/rolling/draco_test_round_tripfrom
ahcorde/rolling/draco_test_round_trip_transport

Conversation

@ahcorde

@ahcorde ahcorde commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

Added draco node-level transport pub/sub

Claude Opus 4.7

Signed-off-by: Alejandro Hernandez Cordero <ahcorde@gmail.com>
@ahcorde ahcorde self-assigned this Jul 3, 2026
@mergify

mergify Bot commented Jul 3, 2026

Copy link
Copy Markdown

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

auto pub = point_cloud_transport::create_publisher(
*node, "test_cloud", rclcpp::SystemDefaultsQoS());

rclcpp::executors::SingleThreadedExecutor exec;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you have have to add nodes to the executor before you use them?

exec.add_node(pub) 
exec.add_node(sub) 

const uint32_t n = 100;
const auto cloud = makeXyzCloud(n);

const auto deadline = std::chrono::steady_clock::now() + 15s;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a huge fan of timeouts being used in this way. I'd much prefer this to be written as a launch_pytest test using lifecycle nodes, where the test launches and then waits on the state transition of the subscriber to move to READY before firing a timer to send out point clouds at a specific rate. The 15s should be specified deadline specified like this ament_add_ros_isolated_gtest(my_isolated_test test/test_my_node.cpp TIMEOUT 15). Why:

  • consolidating timeouts in the build infrastructure is cleaner than ad-hoc within tests
  • lifecycle nodes let you observe actual state of a subscriber before sending messages to it
  • the subscriber can shutdown on reception of the first cloud, apply the check and convey it within a return code that gets communicated to the launch_pytest and use to pass/fail the test.

At a minimum we need to move the test timeout to CMakeLists and use ament_add_ros_isolated_gtest to mitigate destructive interference (@cottsay this sort of PR is what causes nondeterminism in CI, right?).

Ideally, we'd use launch_pytest and lifecycle nodes to make state directly observable in stead of using time as a weak proxy.

@asymingt asymingt left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to push back if you don't agree with my feedback.

Signed-off-by: Alejandro Hernandez Cordero <ahcorde@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants