Action API and example for dora-ros2-bridge #1194

drindr · 2025-11-14T12:39:31Z

This PR provides solution which make action-related API work in a synchronized function.

To achieve this, I introduce the dynamic stream. I'm wondering if someone has a better solution because Dora seems more fond of using static stuff.

Some of the structs and methods can be written into the API crate if the dynamic stream is acceptable.
Currently, the main implementation is in the examples/ros2-bridge/rust/action-client/node/src/main.rs

drindr · 2025-11-20T11:13:33Z

Another task of this PR is aligning the example with ROS2's minimal example, so that developers from ROS2 can easily find what they want explicitly. It makes this PR so large. Maybe it should be placed in another PR.

haixuanTao · 2025-11-25T13:51:41Z

Sorry for the late response.

So from what I understand from the PR is that it is mainly an example on how to use services and action by basically relaying the ros2-client right?

If that's the case I think I'm okay with merging this

drindr · 2025-11-26T06:27:18Z

Sorry for the late response.

Never mind.

So from what I understand from the PR is that it is mainly an example on how to use services and action by basically relaying the ros2-client right?

Yep.

If that's the case I think I'm okay with merging this

I think it is not ready to be merged. I'd like to place some of its implementation into the API crate, mainly for the API of c++. Before pushing ahead, I'd like to ensure that the implementation is acceptable.

So, from your comments, I think I can start to do this.

Signed-off-by: drindr <dreamchancn@qq.com>

Copilot

Pull request overview

This PR introduces Action API support for the dora-ros2-bridge, enabling synchronized action-related functionality through the use of dynamic streams. The implementation provides both Rust and C++ examples demonstrating action client usage with the Fibonacci action.

Key Changes:

Introduces dynamic stream-based action client API with result, feedback, and status event handling
Refactors UUID conversion in message generation to use as_bytes() instead of encode_lower()
Adds comprehensive action client examples for both Rust and C++ implementations

Reviewed changes

Copilot reviewed 15 out of 17 changed files in this pull request and generated 15 comments.

Show a summary per file

File	Description
`libraries/extensions/ros2-bridge/msg-gen/src/types/action.rs`	Major refactor of action client generation, introducing event-based architecture with separate result, feedback, and status streams
`libraries/extensions/ros2-bridge/msg-gen/src/types/message.rs`	Simplifies UUID conversion by using direct byte access
`libraries/extensions/ros2-bridge/msg-gen/src/lib.rs`	Adds ActionGoalId and ActionStatusEnum type definitions and imports
`libraries/extensions/ros2-bridge/Cargo.toml`	Registers new action client examples
`examples/ros2-bridge/rust/rust-ros2-example-node/src/action_client.rs`	New Rust example demonstrating action client usage with dynamic stream handling
`examples/ros2-bridge/rust/rust-ros2-example-node/Cargo.toml`	Adds futures-concurrency-dynamic dependency and action client binary
`examples/ros2-bridge/rust/action-client/run.rs`	Runner for Rust action client example with ROS2 node orchestration
`examples/ros2-bridge/rust/action-client/dataflow.yml`	Dataflow configuration for Rust action client example
`examples/ros2-bridge/rust/README.md`	Documentation for ROS2 bridge Rust examples
`examples/ros2-bridge/c++/action-client/main.cc`	New C++ example demonstrating action client usage
`examples/ros2-bridge/c++/action-client/run.rs`	Runner for C++ action client example
`examples/ros2-bridge/c++/action-client/dataflow.yml`	Dataflow configuration for C++ action client example
`examples/ros2-bridge/c++/action-client/.gitignore`	Git ignore rules for C++ build artifacts
`examples/ros2-bridge/c++/turtle/run.rs`	Path corrections for build directory
`examples/ros2-bridge/c++/turtle/main.cc`	New C++ turtle example file
`apis/c++/node/build.rs`	Copies cxx.h header to target directory
`Cargo.lock`	Locks futures-concurrency-dynamic dependency

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-04T08:06:10Z

examples/ros2-bridge/c++/action-client/main.cc

+
+            auto feedback_event = client->downcast_feedback(std::move(event));
+
+            if (!feedback_event->matches_goal(*goal_id)) { // if


The comment // if is incomplete and should be removed or completed to explain the intent of this condition check.

Suggested change

if (!feedback_event->matches_goal(*goal_id)) { // if

if (!feedback_event->matches_goal(*goal_id)) { // Skip feedback not belonging to the current goal

Copilot · 2025-12-04T08:06:11Z

libraries/extensions/ros2-bridge/msg-gen/src/types/action.rs

+                            let result = event.event.downcast::<#status_event_name>()
+                                .map_err(|_| eyre::eyre!("downcast to {} failed", "action_msgs__GoalStatus"))?;
+
+                           Ok(result)


[nitpick] There's an inconsistency in spacing around the equals sign. Line 696 has no space before Ok(result), while most other similar lines in the file have consistent spacing. Consider adding a space before Ok for consistency: Ok(result).

Suggested change

Ok(result)

Ok(result)

Copilot · 2025-12-04T08:06:11Z

libraries/extensions/ros2-bridge/msg-gen/src/types/action.rs

+                let status_id = {
+                    let client = Arc::clone(&client);
+                    let stream = futures_lite::stream::unfold(client, |client| async {
+                        Some((client.async_receive_status().await , client))


[nitpick] There's a missing space after the comma in the Some((client.async_receive_status().await , client)) expression. Should be Some((client.async_receive_status().await, client)).

Suggested change

Some((client.async_receive_status().await , client))

Some((client.async_receive_status().await, client))

Copilot · 2025-12-04T08:06:12Z

libraries/extensions/ros2-bridge/msg-gen/src/types/action.rs

+                    // SAFETY:
+                    // There is no modification on the goal_client.
+                    // The wait_for_service() method only requires immutable access to the subscription struct.
+                    let service_client = unsafe {
+                        let ptr = std::sync::Arc::as_ptr(&self.client)
+                            as *mut crate::ros2_client::action::ActionClient< action :: #self_name >;
+                        (&mut *ptr).goal_client()
+                    };


Similar to the feedback_subscription unsafe block, this casts to a mutable pointer and dereferences it despite the SAFETY comment claiming "There is no modification on the goal_client." This pattern of casting Arc to mutable pointer could lead to undefined behavior. Consider using safe alternatives or providing stronger justification for why this is sound.

Suggested change

// SAFETY:

// There is no modification on the goal_client.

// The wait_for_service() method only requires immutable access to the subscription struct.

let service_client = unsafe {

let ptr = std::sync::Arc::as_ptr(&self.client)

as *mut crate::ros2_client::action::ActionClient< action :: #self_name >;

(&mut *ptr).goal_client()

};

// Safe: goal_client() only requires immutable access.

let service_client = self.client.goal_client();

Copilot · 2025-12-04T08:06:12Z

libraries/extensions/ros2-bridge/msg-gen/src/types/action.rs

@@ -1,4 +1,5 @@
 use heck::SnakeCase;
+use nom::error::FromExternalError;


The import nom::error::FromExternalError appears to be unused. This import is not referenced anywhere in the visible code and should be removed.

Suggested change

use nom::error::FromExternalError;

Copilot · 2025-12-04T08:06:15Z

libraries/extensions/ros2-bridge/msg-gen/src/types/action.rs

+
+                           Ok(result)
+                        },
+                        _ => eyre::bail!("not a {} feedback event", #self_name_str),


The error message says "not a {} feedback event" but this is in the downcast_status function, not a feedback downcast. This should be "not a {} status event" for accuracy.

Suggested change

_ => eyre::bail!("not a {} feedback event", #self_name_str),

_ => eyre::bail!("not a {} status event", #self_name_str),

Copilot · 2025-12-04T08:06:15Z

examples/ros2-bridge/c++/action-client/main.cc

+    auto servie_qos = qos_default();
+    servie_qos.durability = Ros2Durability::Volatile;
+    servie_qos.liveliness = Ros2Liveliness::Automatic;
+    servie_qos.reliable = true;
+    servie_qos.max_blocking_time = 0.1;
+    auto qos = actionqos_default();
+    qos.goal_service = servie_qos;
+    qos.result_service = servie_qos;
+    qos.cancel_service = servie_qos;
+    qos.feedback_subscription = servie_qos;
+    qos.status_subscription = servie_qos;


The misspelled variable servie_qos (should be service_qos) is being used here. This needs to be corrected along with the declaration on line 19.

Suggested change

auto servie_qos = qos_default();

servie_qos.durability = Ros2Durability::Volatile;

servie_qos.liveliness = Ros2Liveliness::Automatic;

servie_qos.reliable = true;

servie_qos.max_blocking_time = 0.1;

auto qos = actionqos_default();

qos.goal_service = servie_qos;

qos.result_service = servie_qos;

qos.cancel_service = servie_qos;

qos.feedback_subscription = servie_qos;

qos.status_subscription = servie_qos;

auto service_qos = qos_default();

service_qos.durability = Ros2Durability::Volatile;

service_qos.liveliness = Ros2Liveliness::Automatic;

service_qos.reliable = true;

service_qos.max_blocking_time = 0.1;

auto qos = actionqos_default();

qos.goal_service = service_qos;

qos.result_service = service_qos;

qos.cancel_service = service_qos;

qos.feedback_subscription = service_qos;

qos.status_subscription = service_qos;

Copilot · 2025-12-04T08:06:15Z

examples/ros2-bridge/c++/action-client/main.cc

+            if(feedback_event->matches_goal(*goal_id)) {
+                auto feedback = feedback_event->get_feedback();
+                std::cout << "Feedback: ";
+                for(auto& num: feedback.sequence) {
+                    std::cout << num << " ";
+                }


This condition if (!feedback_event->matches_goal(*goal_id)) is immediately followed by a redundant check if(feedback_event->matches_goal(*goal_id)) on line 96. The second if-block will never execute because if the first condition is false (meaning the feedback does match the goal), we continue. If the first condition is true, we continue and skip the second check. This appears to be a logic error.

Suggested change

if(feedback_event->matches_goal(*goal_id)) {

auto feedback = feedback_event->get_feedback();

std::cout << "Feedback: ";

for(auto& num: feedback.sequence) {

std::cout << num << " ";

}

auto feedback = feedback_event->get_feedback();

std::cout << "Feedback: ";

for(auto& num: feedback.sequence) {

std::cout << num << " ";

Copilot · 2025-12-04T08:06:16Z

libraries/extensions/ros2-bridge/msg-gen/src/types/action.rs

+                        let item = unsafe {
+                            let ptr = Arc::as_ptr(&client) as *mut crate::ros2_client::action::ActionClient< action :: #self_name>;
+                            let sub = (&mut *ptr).feedback_subscription();


The SAFETY comment states "There is no modification on the feedback_subscription," but the code casts to a mutable pointer (as *mut) and then dereferences it. While the async_take() method may only require immutable access, casting to mutable and dereferencing could be undefined behavior if the Arc is shared. Consider whether this unsafe block is truly necessary or if there's a safe alternative using immutable access patterns.

Suggested change

let item = unsafe {

let ptr = Arc::as_ptr(&client) as *mut crate::ros2_client::action::ActionClient< action :: #self_name>;

let sub = (&mut *ptr).feedback_subscription();

let item = {

let sub = (&*client).feedback_subscription();

Copilot · 2025-12-04T08:06:16Z

examples/ros2-bridge/rust/rust-ros2-example-node/src/action_client.rs

+                                }
+                            });
+
+                            // sent = true;


[nitpick] The commented-out line // sent = true; should either be removed or uncommented. Having commented code in a PR suggests incomplete work or unclear intent.

Suggested change

// sent = true;

Copilot

Pull request overview

Copilot reviewed 17 out of 19 changed files in this pull request and generated 9 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

libraries/extensions/ros2-bridge/msg-gen/src/lib.rs

Copilot · 2025-12-04T11:09:52Z

examples/ros2-bridge/rust/rust-ros2-example-node/src/action_client.rs

+                    if let Ok((status, result)) = result {
+                        if status == GoalStatusEnum::Unknown {
+                            client_stream_handle.push(result_stream(&client, id));
+                            // there are tons of unknown status :(


The comment on line 119 says "there are tons of unknown status :(" which seems like a developer note that should be removed or clarified. If this is expected behavior, consider adding a proper comment explaining why Unknown status values are common and why they trigger a retry of the result stream.

Suggested change

// there are tons of unknown status :(

// "Unknown" status values are common immediately after sending a goal,

// as the action server may not have processed the goal yet.

// Retrying the result stream allows us to wait for a definitive status.

libraries/extensions/ros2-bridge/msg-gen/src/types/action.rs

Copilot · 2025-12-04T11:09:53Z

examples/ros2-bridge/rust/README.md

@@ -0,0 +1,17 @@
+# `ros2-bridge` Rust Examples
+
+These examples show how to interact between ROS2 and Dora with Rust, including topic pub/sub service client/server and action client/server.


The phrase "action client/server" is mentioned but this PR only includes action client examples. Either update the description to say "action client" only, or add a note that action server examples are planned for future work.

Suggested change

These examples show how to interact between ROS2 and Dora with Rust, including topic pub/sub service client/server and action client/server.

These examples show how to interact between ROS2 and Dora with Rust, including topic pub/sub, service client/server, and action client. Action server examples are planned for future work.

Copilot · 2025-12-04T11:09:53Z

libraries/extensions/ros2-bridge/msg-gen/src/types/action.rs

+                    // the mutable permission is not required at all.
+                    // There is no modification on the goal service client.
+                    // The other method will access the goal service client is send_goal() which is not required
+                    // multable access as well.
+                    // The wait_for_service() method only requires immutable access to the subscription struct.


The SAFETY comment states "There is no modification on the goal service client" but this appears incorrect. The wait_for_service() method may modify internal state related to service discovery or readiness tracking. Additionally, the comment mentions "The other method will access the goal service client is send_goal()" which has a grammatical error ("will access" should be "that will access"). The safety justification should focus on why concurrent access is safe (e.g., the methods being called are internally synchronized or the access pattern doesn't create data races).

Suggested change

// the mutable permission is not required at all.

// There is no modification on the goal service client.

// The other method will access the goal service client is send_goal() which is not required

// multable access as well.

// The wait_for_service() method only requires immutable access to the subscription struct.

// according to its documentation, the wait_for_service() method is internally synchronized

// and safe to call concurrently. Although we obtain a mutable reference via unsafe code,

// this is only to satisfy the API; no data races will occur as long as all access is through

// methods that are internally synchronized. The only other method that accesses the goal

// service client is send_goal(), which is also safe to call concurrently for the same reason.

Copilot · 2025-12-04T11:09:53Z

libraries/extensions/ros2-bridge/msg-gen/src/types/message.rs

-                        let mut buf = [0u8; 16];
-                        uuid.as_simple().encode_lower(&mut buf);
-                        Self { uuid: buf }
+                        Self { uuid: *uuid.as_bytes() }


The UUID conversion implementation has changed from using as_simple().encode_lower() to as_bytes(). While this is more direct, the original code was converting to a simple (hyphen-less) string representation, while as_bytes() returns the raw 16-byte UUID. These are semantically different representations. If the Self { uuid: ... } field expects a byte array representation of the UUID bytes (not a string encoding), this is correct. However, if backwards compatibility is needed with code expecting the string representation, this could be a breaking change.

Copilot · 2025-12-04T11:09:54Z

examples/ros2-bridge/rust/action-client/run.rs

+
+fn main() -> eyre::Result<()> {
+    let root = Path::new(env!("CARGO_MANIFEST_DIR"));
+    std::env::set_current_dir(root.join("../../../").join(file!()).parent().unwrap())


The path construction on line 11 uses join(file!()).parent().unwrap() which is unusual. The file!() macro returns a string path to the current source file, not a directory. Using parent() on a joined file path may not produce the intended result. Consider using a more straightforward approach like root.join("examples/ros2-bridge/rust/action-client") or verifying this path manipulation produces the correct directory.

Suggested change

std::env::set_current_dir(root.join("../../../").join(file!()).parent().unwrap())

std::env::set_current_dir(root.join("examples/ros2-bridge/rust/action-client"))

Copilot · 2025-12-04T11:09:54Z

examples/ros2-bridge/c++/action-client/dataflow.yml

+    path: build/node_rust_api
+    inputs:
+      tick: dora/timer/millis/500
+    outputs:


[nitpick] The outputs key is defined but has no entries. If this node doesn't produce any outputs, consider removing the empty outputs: key entirely for clarity, or add a comment explaining why it's present but empty.

Suggested change

outputs:

libraries/extensions/ros2-bridge/msg-gen/src/types/action.rs

Signed-off-by: drindr <dreamchancn@qq.com>

drindr · 2025-12-05T06:26:44Z

There are 2 unsafe blocks can be removed if Atostek/ros2-client#64 is merged

drindr · 2025-12-05T06:28:04Z

Ping @phil-opp,
There are some modifications on the ros2-bridge crate and I think they are ready to be reviewed.

phil-opp · 2025-12-05T11:31:39Z

Marked as blocked because this requires Atostek/ros2-client#63.

There are 2 unsafe blocks can be removed if Atostek/ros2-client#63 is merged

The unsafe blocks violate Rust's invariants (&mut must be exclusive), so they are undefined behavior. Unfortunately it doesn't matter that you only call &self methods on the resulting reference, it's still undefined behavior to even create the &mut instances.

phil-opp

Looks good to me overall, thanks!

libraries/extensions/ros2-bridge/msg-gen/src/types/action.rs

drindr · 2025-12-05T14:48:49Z

The unsafe blocks violate Rust's invariants (&mut must be exclusive), so they are undefined behavior. Unfortunately it doesn't matter that you only call &self methods on the resulting reference, it's still undefined behavior to even create the &mut instances.

You are right. The get_mut_unchecked method of Arc is still experimental in the nightly rust. Comparing to the UnsafeCell, I think using our own fork of ros2-client temporarily is considerable.

phil-opp · 2025-12-08T09:22:27Z

You closed your ros2-client PR, was that intentional?

drindr · 2025-12-08T09:28:56Z

You closed your ros2-client PR, was that intentional?

I deleted the original branch by mistake and I have open another PR in Atostek/ros2-client#64

drindr mentioned this pull request Nov 20, 2025

Standalone ros2 example #1236

Merged

drindr marked this pull request as draft November 26, 2025 06:34

drindr force-pushed the ros-action-api branch 2 times, most recently from 8eaf071 to 910376d Compare December 1, 2025 15:11

drindr added 3 commits December 2, 2025 10:20

Add rust ros2 action client example

1ab7d6d

Signed-off-by: drindr <dreamchancn@qq.com>

dora's ros2-bridge action client api for c++

6265467

Signed-off-by: drindr <dreamchancn@qq.com>

Add action-client example of dora ros2-bridge in c++

f2a7c0c

Signed-off-by: drindr <dreamchancn@qq.com>

drindr force-pushed the ros-action-api branch from 910376d to f2a7c0c Compare December 4, 2025 07:56

drindr changed the title ~~RFC: ros2-bridge action api~~ Action API and example for dora-ros2-bridge Dec 4, 2025

drindr marked this pull request as ready for review December 4, 2025 07:58

Copilot AI review requested due to automatic review settings December 4, 2025 07:58

Copilot started reviewing on behalf of drindr December 4, 2025 07:59 View session

Copilot finished reviewing on behalf of drindr December 4, 2025 08:01

Copilot AI reviewed Dec 4, 2025

View reviewed changes

drindr force-pushed the ros-action-api branch from f050eab to 3842e8f Compare December 4, 2025 11:01

drindr requested a review from Copilot December 4, 2025 11:02

Copilot started reviewing on behalf of drindr December 4, 2025 11:06 View session

Copilot finished reviewing on behalf of drindr December 4, 2025 11:08

Copilot AI reviewed Dec 4, 2025

View reviewed changes

clean code about ros2-bridge

20cc155

Signed-off-by: drindr <dreamchancn@qq.com>

drindr force-pushed the ros-action-api branch from 3842e8f to 20cc155 Compare December 4, 2025 12:34

phil-opp added the blocked label Dec 5, 2025

phil-opp reviewed Dec 5, 2025

View reviewed changes

libraries/extensions/ros2-bridge/msg-gen/src/types/action.rs Show resolved Hide resolved

drindr mentioned this pull request Dec 18, 2025

Tracking support for dora-ros2 bridge #1170

Open

21 tasks


		auto feedback_event = client->downcast_feedback(std::move(event));

		if (!feedback_event->matches_goal(*goal_id)) { // if

	Some((client.async_receive_status().await , client))
	Some((client.async_receive_status().await, client))

		@@ -1,4 +1,5 @@
		use heck::SnakeCase;
		use nom::error::FromExternalError;

	_ => eyre::bail!("not a {} feedback event", #self_name_str),
	_ => eyre::bail!("not a {} status event", #self_name_str),

-                            // there are tons of unknown status :(
+                            // "Unknown" status values are common immediately after sending a goal,
+                            // as the action server may not have processed the goal yet.
+                            // Retrying the result stream allows us to wait for a definitive status.

		@@ -0,0 +1,17 @@
		# `ros2-bridge` Rust Examples

		These examples show how to interact between ROS2 and Dora with Rust, including topic pub/sub service client/server and action client/server.

	std::env::set_current_dir(root.join("../../../").join(file!()).parent().unwrap())
	std::env::set_current_dir(root.join("examples/ros2-bridge/rust/action-client"))

Action API and example for dora-ros2-bridge #1194

Are you sure you want to change the base?

Action API and example for dora-ros2-bridge #1194

Uh oh!

Conversation

drindr commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drindr commented Nov 20, 2025

Uh oh!

haixuanTao commented Nov 25, 2025

Uh oh!

drindr commented Nov 26, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

drindr commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drindr commented Dec 5, 2025

Uh oh!

phil-opp commented Dec 5, 2025

drindr commented Nov 14, 2025 •

edited

Loading

drindr commented Dec 5, 2025 •

edited

Loading

drindr commented Dec 5, 2025 •

edited

Loading