Several fixes on ros2 bridge and readme update by drindr · Pull Request #1253 · dora-rs/dora

drindr · 2025-12-08T17:47:28Z

Support boolean literal True and False according to the nav2_msgs.
Accept buildtool_depend and build_depend tags as the dependencies while parsing the ROS2 package.
Support transitive dependencies for ROS2 messages.
Ensure the ROS node always being killed correctly in the examples' run.rs.
Update the README file for the main ROS2 example of each language.
Remove the ros2-examples feature gate

Copilot

Pull request overview

This PR implements several fixes and improvements to the ROS2 bridge functionality:

Adds support for boolean literals True and False in ROS2 message parsing
Extends dependency parsing to include buildtool_depend and build_depend tags
Implements transitive dependency resolution for ROS2 messages using depth-first search
Ensures proper cleanup of ROS nodes in example run scripts
Updates documentation with clearer instructions and removes obsolete feature flags

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
libraries/extensions/ros2-bridge/msg-gen/src/parser/literal.rs	Adds capitalized boolean literal support (`True`/`False`) for compatibility with nav2_msgs
libraries/extensions/ros2-bridge/msg-gen/src/parser/package.rs	Implements transitive dependency resolution and adds support for additional dependency tags
libraries/extensions/ros2-bridge/Cargo.toml	Removes `ros2-examples` feature flag and moves dependencies to dev-dependencies
examples/ros2-bridge/rust/turtle/run.rs	Refactors cleanup logic to ensure ROS nodes are killed even if dataflow fails
examples/ros2-bridge/rust/topic-sub/run.rs	Refactors cleanup logic to ensure ROS nodes are killed even if dataflow fails
examples/ros2-bridge/rust/topic-pub/run.rs	Refactors cleanup logic to ensure ROS nodes are killed even if dataflow fails
examples/ros2-bridge/rust/service-server/run.rs	Removes error handling from run() call (error is now silently ignored)
examples/ros2-bridge/rust/service-client/run.rs	Refactors cleanup logic to ensure ROS nodes are killed even if dataflow fails
examples/ros2-bridge/python/turtle/run.rs	Refactors cleanup logic to ensure ROS nodes are killed even if dataflow fails
examples/ros2-bridge/c++/turtle/run.rs	Refactors cleanup logic to ensure ROS nodes are killed even if dataflow fails
examples/ros2-bridge/rust/turtle/README.md	Updates documentation with generic ROS distribution placeholder and simplified run command
examples/ros2-bridge/python/turtle/README.md	Updates documentation with generic ROS distribution placeholder and corrects file paths
examples/ros2-bridge/c++/turtle/README.md	Reorganizes documentation with clearer structure and updated run instructions
.github/workflows/ci.yml	Updates CI workflow to remove `ros2-examples` feature flag from cargo commands

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

Copilot · 2025-12-08T17:55:34Z

libraries/extensions/ros2-bridge/msg-gen/src/parser/package.rs

+    // flatten the dependencies
+    let dependencies = {
+        let mut flattened_dependencies = HashMap::<String, BTreeSet<String>>::new();
+        let mut finished_set: BTreeSet<String> = dependencies.keys().cloned().into_iter().collect();


The .into_iter() call is redundant here. The .cloned() iterator adapter already returns an iterator, so chaining .into_iter() after it has no effect.

Consider simplifying to:

let mut finished_set: BTreeSet<String> = dependencies.keys().cloned().collect();

Suggested change

let mut finished_set: BTreeSet<String> = dependencies.keys().cloned().into_iter().collect();

let mut finished_set: BTreeSet<String> = dependencies.keys().cloned().collect();

Copilot · 2025-12-08T17:55:34Z

libraries/extensions/ros2-bridge/msg-gen/src/parser/package.rs

+            .get(&package.name)
+            .unwrap()
            .iter()
            .filter_map(|dep| map.get(dep).and_then(|pkg| Some(pkg.clone())))


The and_then with Some is redundant here. When using filter_map, you can directly return the value from map.get(dep) since it already returns Option<&Package>. The .and_then(|pkg| Some(pkg.clone())) is equivalent to just .map(|pkg| pkg.clone()).

Consider simplifying to:

.filter_map(|dep| map.get(dep).map(|pkg| pkg.clone()))

Suggested change

.filter_map(|dep| map.get(dep).and_then(|pkg| Some(pkg.clone())))

.filter_map(|dep| map.get(dep).map(|pkg| pkg.clone()))

Copilot · 2025-12-08T17:55:35Z

examples/ros2-bridge/rust/service-server/run.rs

    let (finish_tx, finish_rx) = mpsc::channel();
    let dataflow_task = std::thread::spawn(move || {
-        run("dataflow.yml".to_string(), false).unwrap();
+        run("dataflow.yml".to_string(), false);


The return value of the run function (which returns eyre::Result<()>) is being silently ignored. If the dataflow fails to run, this error will be lost and won't be propagated.

Consider handling the result:

let dataflow_task = std::thread::spawn(move || { let result = run("dataflow.yml".to_string(), false); finish_tx.send(()); result });

Then later, you could check the result with dataflow_task.join() if needed.

Copilot · 2025-12-08T17:55:35Z

libraries/extensions/ros2-bridge/msg-gen/src/parser/package.rs

 }

+fn dfs_collect(
+    name: &String,


The parameter type name: &String is non-idiomatic. It's generally better to use &str instead of &String for function parameters as it's more flexible (accepts both &String and &str) and follows Rust best practices.

Consider changing to:

fn dfs_collect( name: &str, // ... rest of parameters

You'll need to update the call sites accordingly (e.g., use name.as_str() or &name when calling).

Copilot · 2025-12-08T17:55:35Z

libraries/extensions/ros2-bridge/msg-gen/src/parser/package.rs

+    let direct_deps: BTreeSet<String> = dependencies.get(name).unwrap().iter().cloned().collect();
+    for dep_name in &direct_deps {
+        let indirect_deps = dfs_collect(
+            &dep_name,
+            dependencies,
+            flattened_dependencies,
+            finished_set,
+        );
+        add_deps.extend(indirect_deps);
+    }
+    add_deps.extend(direct_deps);


This line creates an unnecessary intermediate BTreeSet by cloning all the dependency names. The dependencies.get(name).unwrap() already returns a &BTreeSet<String>, which can be iterated directly.

Consider simplifying to:

for dep_name in dependencies.get(name).unwrap() {

This avoids the allocation and cloning of all strings.

Suggested change

let direct_deps: BTreeSet<String> = dependencies.get(name).unwrap().iter().cloned().collect();

for dep_name in &direct_deps {

let indirect_deps = dfs_collect(

&dep_name,

dependencies,

flattened_dependencies,

finished_set,

);

add_deps.extend(indirect_deps);

}

add_deps.extend(direct_deps);

for dep_name in dependencies.get(name).unwrap() {

let indirect_deps = dfs_collect(

dep_name,

dependencies,

flattened_dependencies,

finished_set,

);

add_deps.extend(indirect_deps);

}

add_deps.extend(dependencies.get(name).unwrap().iter().cloned());

Copilot · 2025-12-08T17:55:36Z

examples/ros2-bridge/c++/turtle/README.md

+## Alternative(manual)
+Not recommended, because manually build c++ is heavy job.


There are several grammar and spelling issues in this section:

"Alternative(manual)" should have a space: "Alternative (manual)"

"manually build c++ is heavy job" should be "manually building C++ is a heavy job"

Suggested change

## Alternative(manual)

Not recommended, because manually build c++ is heavy job.

## Alternative (manual)

Not recommended, because manually building C++ is a heavy job.

Copilot · 2025-12-08T17:55:36Z

examples/ros2-bridge/c++/turtle/README.md


-then start DORA service client from another terminal
-```
+3. From terminal 3 in the folder of dora repository. Note the source command here is necessary as this allow ROS2 message types to be found and compile dynamically.


Grammar issue: "this allow" should be "this allows" (singular subject requires singular verb form).

Suggested change

3. From terminal 3 in the folder of dora repository. Note the source command here is necessary as this allow ROS2 message types to be found and compile dynamically.

3. From terminal 3 in the folder of dora repository. Note the source command here is necessary as this allows ROS2 message types to be found and compile dynamically.

haixuanTao

Looks great overall!

Made some comment and agree with copilot review m

examples/ros2-bridge/c++/turtle/README.md

libraries/extensions/ros2-bridge/msg-gen/src/parser/package.rs

Some ros2 message use the pascal case boolean literal. ref: https://github.com/ros-navigation/navigation2/blob/0fc9a10b9c2cdb28ac7a1ff8bd726650a4a9b8f1/nav2_msgs/action/DockRobot.action#L3 Signed-off-by: drindr <dreamchancn@qq.com>

Some ros2 msg like the `robot_localization` depends `builtin_interface` with the `builttool_depend`. Some ros2 msg like the `slam_toolbox` use the transitive depenedency through the `tf2_geometry_msgs` Signed-off-by: drindr <dreamchancn@qq.com>

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

The examples of ROS2 have been placed under the crate `dora-ros2-bridge`, hence removing the `ros2-examples` feature-gate Signed-off-by: drindr <dreamchancn@qq.com>

…r's function name Signed-off-by: drindr <dreamchancn@qq.com>

drindr · 2025-12-12T10:39:28Z

This PR is updated. But I do not know why the CI is always timeout😭️. Should we configure the timeout with a longer timeout tolerance?

phil-opp · 2025-12-12T12:20:12Z

Should we configure the timeout with a longer timeout tolerance?

I don't think that this will help. Looks like it hangs for some reason:

(note the timestamps)

The command that hangs seems to be dora run examples/python-async/dataflow.yaml --uv

I just restarted the build to see whether it's reproducible.

cc @haixuanTao

phil-opp · 2025-12-12T13:13:34Z

Retrying seemed to "fix" it. Not sure what was causing this, we should look into it when we have time. But it seems to be unrelated to this PR.

drindr · 2025-12-12T13:20:04Z

The command that hangs seems to be dora run examples/python-async/dataflow.yaml --uv

dora/examples/python-async/receive_data.py

Line 9 in 24c2f3a

event = await node.recv_async()

Maybe a timeout for the async waiting can solve it.

phil-opp · 2025-12-12T13:25:10Z

Maybe a timeout for the async waiting can solve it.

There should be periodic timer ticks. Once the input is closed, the recv_async function should return immediately with None. But for some reason the input seems to stay open, but no more events are sent...

drindr · 2025-12-17T01:01:15Z

@haixuanTao Could you review this PR again. Flaws you mentioned above have been solved.

Copilot AI review requested due to automatic review settings December 8, 2025 17:47

Copilot started reviewing on behalf of drindr December 8, 2025 17:51 View session

Copilot AI reviewed Dec 8, 2025

View reviewed changes

drindr force-pushed the fix-ros2-example branch 3 times, most recently from f6a9e60 to bccee1f Compare December 9, 2025 09:39

haixuanTao approved these changes Dec 10, 2025

View reviewed changes

examples/ros2-bridge/c++/turtle/README.md Show resolved Hide resolved

libraries/extensions/ros2-bridge/msg-gen/src/parser/package.rs Outdated Show resolved Hide resolved

drindr added 5 commits December 10, 2025 19:37

fix: bool literal in ros2 msg parser

5f5731c

Some ros2 message use the pascal case boolean literal. ref: https://github.com/ros-navigation/navigation2/blob/0fc9a10b9c2cdb28ac7a1ff8bd726650a4a9b8f1/nav2_msgs/action/DockRobot.action#L3 Signed-off-by: drindr <dreamchancn@qq.com>

fix: ensure the ros2 example kill the ros2's node correctly

3c065f7

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

Remove the ROS2 examples features gate and fix the README instructions

df668b7

The examples of ROS2 have been placed under the crate `dora-ros2-bridge`, hence removing the `ros2-examples` feature-gate Signed-off-by: drindr <dreamchancn@qq.com>

add installation readme for ros2 example, refactor ros2 message parse…

7a9cee2

…r's function name Signed-off-by: drindr <dreamchancn@qq.com>

drindr force-pushed the fix-ros2-example branch from bccee1f to 7a9cee2 Compare December 10, 2025 11:38

Merge branch 'main' into fix-ros2-example

d1c87cf

haixuanTao approved these changes Dec 17, 2025

View reviewed changes

haixuanTao merged commit e3309a8 into dora-rs:main Dec 17, 2025
72 of 76 checks passed

	let mut finished_set: BTreeSet<String> = dependencies.keys().cloned().into_iter().collect();
	let mut finished_set: BTreeSet<String> = dependencies.keys().cloned().collect();

	.filter_map(\|dep\| map.get(dep).and_then(\|pkg\| Some(pkg.clone())))
	.filter_map(\|dep\| map.get(dep).map(\|pkg\| pkg.clone()))

		## Alternative(manual)
		Not recommended, because manually build c++ is heavy job.

	3. From terminal 3 in the folder of dora repository. Note the source command here is necessary as this allow ROS2 message types to be found and compile dynamically.
	3. From terminal 3 in the folder of dora repository. Note the source command here is necessary as this allows ROS2 message types to be found and compile dynamically.

Conversation

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

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

haixuanTao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

drindr commented Dec 12, 2025

Uh oh!

phil-opp commented Dec 12, 2025

Uh oh!

phil-opp commented Dec 12, 2025

Uh oh!

drindr commented Dec 12, 2025

Uh oh!

phil-opp commented Dec 12, 2025

Uh oh!

drindr commented Dec 17, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

drindr commented Dec 8, 2025 •

edited

Loading