Skip to content

Added timesync endpoint for the arm velocity control #179

Merged
khughes-bdai merged 7 commits intobdaiinstitute:mainfrom
strapsai:kabirk/add-timesync
Oct 8, 2025
Merged

Added timesync endpoint for the arm velocity control #179
khughes-bdai merged 7 commits intobdaiinstitute:mainfrom
strapsai:kabirk/add-timesync

Conversation

@kabirkedia
Copy link
Contributor

@kabirkedia kabirkedia commented Sep 3, 2025

This PR addresses bdaiinstitute/spot_ros2#726.

Changes

Updated the arm velocity command callback to include proper timestamp handling.

Instead of restamping with time.time(), the ROS message’s header timestamp is now passed directly into the SDK.

Why this matters

Preserving the original ROS timestamp ensures that the commands reflect the actual time they were generated rather than the time they happened to be processed. This is especially important for teleop scenarios, where even small timing inconsistencies can affect responsiveness and control smoothness.

@coveralls
Copy link

coveralls commented Sep 3, 2025

Pull Request Test Coverage Report for Build 18354379025

Details

  • 2 of 6 (33.33%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.003%) to 44.317%

Changes Missing Coverage Covered Lines Changed/Added Lines %
spot_wrapper/spot_arm.py 0 4 0.0%
Totals Coverage Status
Change from base Build 17986558028: 0.003%
Covered Lines: 2195
Relevant Lines: 4953

💛 - Coveralls

@kabirkedia kabirkedia marked this pull request as draft September 3, 2025 20:26
@kabirkedia kabirkedia marked this pull request as ready for review September 3, 2025 21:49
@kabirkedia
Copy link
Contributor Author

Hey @khughes-bdai @tcappellari-bdai did you have a chance to check?

@khughes-bdai
Copy link
Collaborator

Hey @kabirkedia, we can give this a test/review later this week. Sorry been a bit busy!

@kabirkedia
Copy link
Contributor Author

Okay thank you!

@khughes-bdai
Copy link
Collaborator

Hey @kabirkedia, so sorry for the delay in the review. First thing, there have been a few changes to spot_wrapper coming from #182 that I think are now causing merge conflicts with this PR. That PR switches to using BD's now_sec() instead of time.time() which for the purposes of this PR are equivalent.

My opinion on which way to proceed with this and bdaiinstitute/spot_ros2#734 would be to try the QOS change on the spot ros2 side, and on this side just pass the endpoint appropriately to the robot command. This to me seems like the simplest way to proceed. If this is not enough, then we can evaluate other options like explicitly passing timestamps.

Thanks again for all this work, I think it will be super useful to get in!

@kabirkedia
Copy link
Contributor Author

Hey @khughes-bdai I think I made the correct merge. Please check and let me know!

@khughes-bdai
Copy link
Collaborator

Thanks @kabirkedia ! Looks like the pre-commit check is failing, could you please run pre-commit install and pre-commit run --all and re-push to your branch?
It also looks like one of the pytests is failing, I'll look into that. Seems related to the velocity cmd update which should have not been changed 🤔

@kabirkedia
Copy link
Contributor Author

Hey @khughes-bdai I ran pre-commit on this
Let me know if this still works

@kabirkedia
Copy link
Contributor Author

Let me know when you merge this since I need to resolve sub module conficts in the spot_ros2

Copy link
Collaborator

@khughes-bdai khughes-bdai left a comment

Choose a reason for hiding this comment

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

Just needs a small change to pass CI! Other than that, looks good

v_x: float,
v_y: float,
v_rot: float,
timestamp: float = now_sec(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I found the source of the CI error. Calling now_sec in the arg list like this is causing the failure. Can you replace this with

Suggested change
timestamp: float = now_sec(),
timestamp: float | None = None,

@kabirkedia kabirkedia changed the title Added timesync endpoint for the arm velocity control \ Added timesync endpoint for the arm velocity control Oct 8, 2025
@kabirkedia
Copy link
Contributor Author

@khughes-bdai done!

Copy link
Collaborator

@khughes-bdai khughes-bdai left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @kabirkedia ! I'll merge this in now

@khughes-bdai khughes-bdai merged commit 566bb0f into bdaiinstitute:main Oct 8, 2025
3 checks passed
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.

3 participants