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.

Sign up for GitHub

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

Jump to bottom

Adds timesync timeout to params #552

Merged
merged 9 commits into from
Jan 14, 2025
Merged

Adds timesync timeout to params #552

merged 9 commits into from
Jan 14, 2025

Conversation

mschweig
Copy link
Contributor

Change Overview

PR #544 added the WaitForSync call to establish a timesync with the robot. This function has a timeout parameter which was hardcoded before. This PR now adds the timeout to the ParameterInterface

I needed to add a rclcpp node to default_spot_api to get the parameter values, not sure if this is how you intended the use of the ParameterInterface. If you like another approach I am happy to change it :)

Testing Done

  1. tested various timeouts
  2. connected to a robot via WAN VPN
  3. executed locomotion commands

@khughes-bdai Graphite App
Copy link
Collaborator

Thanks for the PR! Along with the other comments, could you please also add to the unit tests in spot_driver/test/src/test_parameter_interface.cpp with this new parameter?

bjin-bdai
bjin-bdai reviewed Jan 13, 2025
View reviewed changes
Copy link
Contributor

@bjin-bdai bjin-bdai left a comment

Choose a reason for hiding this comment

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

not to familiar with the stack, but based on the diff it seems like we can avoid using int8_t

spot_driver/src/api/default_time_sync_api.cpp Show resolved Hide resolved
spot_driver/src/api/default_time_sync_api.cpp Outdated Show resolved Hide resolved
spot_driver/include/spot_driver/api/default_spot_api.hpp Outdated Show resolved Hide resolved
jcarpinelli-bdai
jcarpinelli-bdai reviewed Jan 13, 2025
View reviewed changes
Copy link

@jcarpinelli-bdai jcarpinelli-bdai left a comment

Choose a reason for hiding this comment

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

Looks good to me! I'm learning this repository myself, so holding off approval because I'm not qualified to issue it. Thank you for the contribution!

spot_driver/include/spot_driver/api/default_time_sync_api.hpp Outdated Show resolved Hide resolved
spot_driver/test/src/test_parameter_interface.cpp Show resolved Hide resolved
@mschweig
Copy link
Contributor Author

mschweig commented Jan 14, 2025 •
edited
Loading

Hi @khughes-bdai and @bjin-bdai, thanks for your review and suggestions.
Implemented timesync_timeout as a required const std::chrono::seconds parameter.

Ran the test cases locally. All green. Tested with a robot via WAN VPN.

bjin-bdai
bjin-bdai approved these changes Jan 14, 2025
View reviewed changes
Copy link
Contributor

@bjin-bdai bjin-bdai left a comment

Choose a reason for hiding this comment

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

I have one comment about a potential risk with the chrono usage, but otherwise LGTM

spot_driver/src/interfaces/rclcpp_parameter_interface.cpp Outdated Show resolved Hide resolved
spot_driver/include/spot_driver/api/default_spot_api.hpp Outdated Show resolved Hide resolved
@mschweig
Copy link
Contributor Author

I have one comment about a potential risk with the chrono usage, but otherwise LGTM

Thanks, added your suggestions and tested again. Should be ready to merge.

khughes-bdai
khughes-bdai approved these changes Jan 14, 2025
View reviewed changes
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 -- just tested on a robot to double check the behavior. I think this actually might have slightly increased our image publishing speeds too 🎉 thanks!

@khughes-bdai Graphite App
Copy link
Collaborator

khughes-bdai commented Jan 14, 2025 •
edited
Loading

Merge activity

  • Jan 14, 3:32 PM EST: A user started a stack merge that includes this pull request via Graphite.
  • Jan 14, 3:32 PM EST: Graphite couldn't merge this PR because it failed for an unknown reason (Stack merges are not currently supported for forked repositories. Please create a branch in the target repository in order to merge).

@khughes-bdai khughes-bdai merged commit d573256 into bdaiinstitute:main Jan 14, 2025
3 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcarpinelli-bdai jcarpinelli-bdai jcarpinelli-bdai left review comments

@bjin-bdai bjin-bdai bjin-bdai approved these changes

@khughes-bdai khughes-bdai khughes-bdai approved these changes

@METEORITENMAX METEORITENMAX Awaiting requested review from METEORITENMAX METEORITENMAX is a code owner

@jbarry-bdai jbarry-bdai Awaiting requested review from jbarry-bdai jbarry-bdai is a code owner

@amessing-bdai amessing-bdai Awaiting requested review from amessing-bdai amessing-bdai is a code owner

@mpickett-bdai mpickett-bdai Awaiting requested review from mpickett-bdai mpickett-bdai is a code owner

@bhung-bdai bhung-bdai Awaiting requested review from bhung-bdai bhung-bdai is a code owner

@tcappellari-bdai tcappellari-bdai Awaiting requested review from tcappellari-bdai tcappellari-bdai is a code owner

@mbrauckmann-bdai mbrauckmann-bdai Awaiting requested review from mbrauckmann-bdai mbrauckmann-bdai was automatically assigned from bdaiinstitute/rapid

@mgoh-bdai mgoh-bdai Awaiting requested review from mgoh-bdai mgoh-bdai was automatically assigned from bdaiinstitute/rapid

@mvictora-bdai mvictora-bdai Awaiting requested review from mvictora-bdai mvictora-bdai was automatically assigned from bdaiinstitute/rapid

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mschweig @khughes-bdai @jcarpinelli-bdai @bjin-bdai