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

Update breaking changes in spot_wrapper refactoring #41

Closed
wants to merge 9 commits into from

Conversation

jeremysee2
Copy link

This addresses the breaking changes from refactoring code in spot_wrapper at bdaiinstitute/spot_wrapper#6

  • Image, world_objects and graph_nav functionality have been broken into separate objects called through spot_wrapper.{object}
  • Refactoring GraphNav navigate_to into initialization then navigate to
    a specific waypoint `spot_wrapper.spot_graph_nav.navigate_to_specific_waypoint'

@jeremysee2
Copy link
Author

I'm not very familiar with ROS2 so I would appreciate if someone who's familiar with this package could review it!

@jeremysee2
Copy link
Author

I've tested this on the surface level, appears to connect to the robot and authenticate fine. Would appreciate someone who uses this regularly to test it out.

self.spot_wrapper._upload_graph_and_snapshots(request.upload_filepath)
response.waypoint_ids = self.spot_wrapper.list_graph(request.upload_filepath)
self.spot_wrapper.spot_graph_nav._clear_graph()
self.spot_wrapper.spot_graph_nav._upload_graph_and_snapshots(request.upload_filepath)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason these are private functions in spot_wrapper if they are going to be used here?

Copy link
Author

Choose a reason for hiding this comment

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

No reason to label these as private, will remove the underscore prefix name

@amessing-bdai amessing-bdai requested a review from kzheng-bdai June 2, 2023 13:27
@@ -49,6 +49,7 @@

import logging
import threading
import traceback
Copy link
Collaborator

Choose a reason for hiding this comment

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

extra import?

@jbarry-bdai
Copy link
Collaborator

This needs the following testing:

Before we do those it'd be good to get the conflicts here and in spot wrapper fixed. @jeremysee2 can you do that?

@heuristicus
Copy link
Contributor

There are a few outstanding PRs on the choreography changes in the wrapper, probably best to get those merged in before we fix conflicts, otherwise we'll have to do it twice. Perhaps a short pause on further PRs into the wrapper until we have the modular wrapper merged. I will try and do some testing with the ROS1 version over the weekend and into next week.

@jeremysee2
Copy link
Author

Yup, I'll look into those conflicts on the weekend.

@amessing-bdai
Copy link
Collaborator

The spot_wrapper PR this corresponds to was broken into pieces and several PRs were made and merged in this repo corresponding to those pieces. As such, this PR is no longer needed.

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