Skip to content

Adjust moveit_servo clock to work with fake_hardware#3529

Merged
AndyZe merged 4 commits intomoveit:mainfrom
Zarnack:main
Oct 31, 2025
Merged

Adjust moveit_servo clock to work with fake_hardware#3529
AndyZe merged 4 commits intomoveit:mainfrom
Zarnack:main

Conversation

@Zarnack
Copy link
Contributor

@Zarnack Zarnack commented Jul 18, 2025

This PR resolves an initialization issue in moveit_servo when used with ros2_control mock hardware components. The robot must be manually moved at startup to trigger joint state updates, otherwise moveit_servo fails to initialize due to a mismatch in clock types between the PlanningSceneMonitor and the ServoNode.

This fixes issue #3040

Description

At startup, servo_node checks whether a recent robot_state has been received by comparing the current time with the timestamp of the latest update (last_update_time_). However, when using a ros2_control mock component, it may wait indefinitely—even if joint states are being published regularly. This issue stems from a mismatch in clock types between the PlanningSceneMonitor and the moveit_servo node.

Mock components typically publish ideal, noise-free joint states, which results in the robot appearing idle.
Consequently:

  • The joint state doesn't change.
  • PlanningSceneMonitor does not update last_update_time_ using RCL_ROS_TIME. (it only updates when the joint states change)
    • Instead, last_update_time_ is updated with RCL_SYSTEM_TIME.
  • moveit_servo, however, performs its time comparisons using RCL_ROS_TIME.
  • This discrepancy causes the freshness check to fail, preventing initialization.

Solution:

This PR introduces a clock type conversion to ensure that time comparisons between moveit_servo and PlanningSceneMonitor are consistent, regardless of the underlying clock type.
This resolves the startup issue when using mock components that produce static, noise-free joint states and therefore do not trigger joint state updates.

There are no relevant API changes.

Checklist

@codecov
Copy link

codecov bot commented Jul 22, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 46.19%. Comparing base (4cbeb9b) to head (faa0d99).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3529      +/-   ##
==========================================
- Coverage   46.21%   46.19%   -0.02%     
==========================================
  Files         720      720              
  Lines       62742    59180    -3562     
  Branches     7594     7595       +1     
==========================================
- Hits        28991    27330    -1661     
+ Misses      33584    31683    -1901     
  Partials      167      167              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Zarnack Zarnack force-pushed the main branch 2 times, most recently from 79586bd to e35a295 Compare July 24, 2025 17:54
Copy link
Member

@AndyZe AndyZe left a comment

Choose a reason for hiding this comment

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

Thanks. I'm good to merge this if CI passes

@AndyZe AndyZe changed the title fixed moveit_servo to work with fake_hardware (fixes issue #3040) Adjust moveit_servo clock to work with fake_hardware Jul 24, 2025
@AndyZe AndyZe enabled auto-merge (squash) July 24, 2025 19:10
@AndyZe
Copy link
Member

AndyZe commented Jul 24, 2025

Maybe @nbbrooks will push this across the finish line

@AndyZe AndyZe requested review from MarqRazz and nbbrooks August 1, 2025 15:19
Copy link
Contributor

@MarqRazz MarqRazz left a comment

Choose a reason for hiding this comment

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

PlanningSceneMonitor does not update last_update_time_ using RCL_ROS_TIME. (it only updates when the joint states change)
Instead, last_update_time_ is updated with RCL_SYSTEM_TIME.

This sounds like the actual issue. Have we dug into why it is hard coded to SYSTEM_TIME?

@AndyZe
Copy link
Member

AndyZe commented Aug 9, 2025

This sounds like the actual issue. Have we dug into why it is hard coded to SYSTEM_TIME?

I don't think anybody has the appetite to make changes in planning_scene_monitor right now. I'll make the anonymous namespace change, though

auto-merge was automatically disabled August 11, 2025 12:48

Head branch was pushed to by a user without write access

@MarqRazz
Copy link
Contributor

Add a note of why we needed to add the function and I think it's good.

@AndyZe AndyZe enabled auto-merge (squash) August 11, 2025 16:47
@Zarnack
Copy link
Contributor Author

Zarnack commented Sep 3, 2025

@AndyZe @nbbrooks any update on this?

@AndyZe
Copy link
Member

AndyZe commented Sep 3, 2025

I would merge it if I were able to...

@nbbrooks nbbrooks disabled auto-merge September 3, 2025 18:46
@nbbrooks
Copy link
Contributor

nbbrooks commented Sep 3, 2025

Are the 3 failing checks something new? That is why it has not already been auto-merged.

I can bypass the checks but it'd be good to know what is going on there.

@nbbrooks
Copy link
Contributor

nbbrooks commented Sep 3, 2025

The checks are also failing on this PR we have been wanting to merge in for a while #2994 and I have re-run them a couple times. So if something is flaky, it must be fairly reliably flaky at this point?

@nbbrooks
Copy link
Contributor

nbbrooks commented Sep 3, 2025

Looks like they are related to TF2 h to hpp deprecations https://github.com/moveit/moveit2/actions/runs/16886460567/job/47835375146?pr=3529#step:11:1188

Which #3533 and #3562 have started to address.

@moveit moveit deleted a comment from github-actions bot Oct 31, 2025
@AndyZe
Copy link
Member

AndyZe commented Oct 31, 2025

I'm going to attempt to rebase and merge 👍

sebastian zarnack and others added 4 commits October 31, 2025 14:35
The planning scene monitor may use a different clock type (e.g., system clock)
than the servo node (ROS clock), causing timestamp comparison failures that
prevented servo from starting properly with fake_hardware controllers.
--> Add convert_clock_type() helper function to handle clock type conversions
@AndyZe AndyZe enabled auto-merge (squash) October 31, 2025 19:36
@AndyZe AndyZe merged commit 0dd551d into moveit:main Oct 31, 2025
9 of 10 checks passed
@github-project-automation github-project-automation bot moved this to ✅ Done in MoveIt Oct 31, 2025
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.

4 participants