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

Fixes TimeSync with robot #544

Merged
merged 4 commits into from
Jan 7, 2025
Merged

Fixes TimeSync with robot #544

merged 4 commits into from
Jan 7, 2025

Conversation

mschweig
Copy link
Contributor

@mschweig mschweig commented Jan 3, 2025

Change Overview

The DefaultTimeSyncApi is missing the WaitForSync call from Spot SDK. As a result the timesync cannot be established, especially when connecting via WIFI.

I added the WaitForSync call to the DefaultTimeSyncApi with a timeout of 5 seconds (maybe create a config param for that) before checking the timesync with HasEstablishedTimeSync

This results in a successful timesync even via WIFI over VPNs.

Screenshot from 2025-01-03 17-41-07

Corresponding issue:
#527

Testing Done

  1. Launched the driver with various parameters
  2. Tested the image services
  3. Tested robot commands such as dock and undock

@khughes-bdai
Copy link
Collaborator

Thank you for the PR! It looks like the linting CI check is failing. On your branch, could you please run pre-commit install then pre-commit run --all to fix this?

Signed-off-by: Manuel Schweiger <[email protected]>
This reverts commit 323f55c.

Signed-off-by: Manuel Schweiger <[email protected]>
Signed-off-by: Manuel Schweiger <[email protected]>
@mschweig
Copy link
Contributor Author

mschweig commented Jan 7, 2025

Hi @khughes-bdai

happy to help :) ran the formatter and added the sign-off so that DCO does not complain.

CI should be ok now.

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.

Looks good! tested on robot and also tested running CI locally (the failing CI test currently is a known issue for external contributors).
Thanks!

@khughes-bdai khughes-bdai merged commit aae0b4b into bdaiinstitute:main Jan 7, 2025
3 of 4 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.

2 participants