Skip to content
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

Use node clock for diagnostic_aggregator and diagnostic_updater #210

Merged
merged 1 commit into from
Oct 18, 2021

Conversation

kenji-miyake
Copy link
Contributor

Overview

I found there is a bug when using diagnostic_aggregator and diagnostic_updater with use_sim_time=true.
To fix this, I changed to use node's clock.

FYI: As another way, I also tried simply to use RCL_ROS_TIME like clock_(new rclcpp::Clock(RCL_ROS_TIME)), but it had no effect.

How to reproduce the problem

Distribution: ROS Galactic

git clone https://github.com/kenji-miyake/test-diagnostics-sim-time.git
cd test-diagnostics-sim-time
source /opt/ros/galactic/setup.bash
colcon build --symlink-install --cmake-args -DCMAKE_BUILD_TYPE=Release
source install/setup.bash

# Terminal 1
ros2 launch test_diagnostics test.launch.xml use_sim_time:=true

# Terminal 2
ros2 topic echo /clock

# Terminal 3
ros2 topic echo /diagnostics
# sim time isn't reflected to diagnostic_updater

# Terminal 4
ros2 topic echo /diagnostics_agg --no-arr
# sim time isn't reflected to diagnostic_aggregator

How to test this PR

git clone https://github.com/kenji-miyake/test-diagnostics-sim-time.git
cd test-diagnostics-sim-time
git clone https://github.com/tier4/diagnostics.git -b fix-sim-time-bug
source /opt/ros/galactic/setup.bash
colcon build --symlink-install --cmake-args -DCMAKE_BUILD_TYPE=Release
source install/setup.bash

# Terminal 1
ros2 launch test_diagnostics test.launch.xml use_sim_time:=true

# Terminal 2
ros2 topic echo /clock

# Terminal 3
ros2 topic echo /diagnostics
# sim time is reflected to diagnostic_updater

# Terminal 4
ros2 topic echo /diagnostics_agg --no-arr
# sim time is reflected to diagnostic_aggregator

@clalancette
Copy link
Contributor

@kenji-miyake With #209 merged, the PR jobs should now succeed. Can you rebase this onto the latest ros2-devel code?

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>
@kenji-miyake
Copy link
Contributor Author

@clalancette Thank you, rebased!

@clalancette clalancette merged commit b9e0bcd into ros:ros2-devel Oct 18, 2021
@kenji-miyake
Copy link
Contributor Author

@clalancette Can this be backported to Galactic if I send a PR?

@kenji-miyake kenji-miyake deleted the fix-sim-time-bug branch October 19, 2021 01:20
@clalancette
Copy link
Contributor

@clalancette Can this be backported to Galactic if I send a PR?

Technically, this patch breaks API and ABI. But since this isn't a core package, and doesn't have a Quality Declaration, that is typically allowed. Also the code it changes looks to be internal, so it might be OK anyway.

I'd say open the backport PR. The maintainers can assess whether they should take it. I'll be clear that I'm not a maintainer here; I just saw some PRs that looked reasonable and could be merged, so I did that.

kenji-miyake added a commit to kenji-miyake/diagnostics that referenced this pull request Oct 19, 2021
)

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>
kenji-miyake added a commit to kenji-miyake/diagnostics that referenced this pull request Nov 2, 2021
)

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>
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