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

Add lease to graph_nav #141

Merged
merged 3 commits into from
Sep 26, 2024
Merged

Conversation

IoTDan
Copy link
Contributor

@IoTDan IoTDan commented Sep 23, 2024

bdaiinstitute/spot_ros2#486

Hello!

thank you for creating and maintaining this repo. It has been extremely helpful.
I did some work with the GraphNav ROS2 services and action server.

While I was able to get it working, i encountered some issues, and wanted to share them.
I very well could have been using the ROS2 interfaces for SPOT incorrectly, but this usage pattern was built off reading the SPOT V4.0.3 documentation and samples, and looking at the examples, source code and documentation in this REPO.

To get SPOT working using GraphNav, I did the following:

  1. ros2 service call /graph_nav_clear_graph spot_msgs/srv/GraphNavClearGraph
  2. ros2 service call /graph_nav_upload_graph spot_msgs/srv/GraphNavUploadGraph "{upload_filepath: ./map.walk'}"
  3. ros2 service call /graph_nav_set_localization spot_msgs/srv/GraphNavSetLocalization "{method: 'waypoint', waypoint_id: 'canny-mite-VQ.uUPtpNIimADm2yVAsWA=='}"

These work without any issue, with the 3rd step being the waypoint where our base station is.
we did all these from teh command line.

the fourth step , which calls the action server navigate_to had issues.

We found we had to make 3 changes to use this ROS2 action server:

  1. in spot_wrapper/spot_Wrapper/spot_graph_nav.py, we had to add the request for a lease in set_initial_localization_waypoint (new lines 296-298)

     self._lease = self._get_lease()
     self._lease_keepalive = LeaseKeepAlive(self._lease_client)
    
  2. in spot_driver/spot_driver/spot_ros2.py (lines 2823 - 2826) we had to change

        upload_path=goal_handle.request.upload_path,
         navigate_to=goal_handle.request.navigate_to,
         initial_localization_fiducial=goal_handle.request.initial_localization_fiducial,
         initial_localization_waypoint=goal_handle.request.initial_localization_waypoint,
    

to
waypoint_id=goal_handle.request.waypoint_id,

  1. in spot_msgs/action/NavigateTO.action we needed to change the definition from

    string upload_path # Absolute path to map_directory, which is downloaded from tablet controller
    string navigate_to # Waypoint id string for where to go
    bool initial_localization_fiducial # Tells the initializer whether to use fiducials
    string initial_localization_waypoint # Waypoint id to trigger localization

    bool success # indicate successful run of triggered service
    string message # informational, e.g. for error messages

    string waypoint_id

to

string waypoint_id # waypoint ID to navigate to
---
bool success   # indicate successful run of triggered service
string message # informational, e.g. for error messages
---
string waypoint_id 

It looks as though the action server interface changed in implementation, but the action server interface was not updated.

did I call the API wrong, or did we find valid bugs?

thanks
Dan

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, thank you for the PR!

@coveralls
Copy link

coveralls commented Sep 24, 2024

Pull Request Test Coverage Report for Build 11054013245

Details

  • 0 of 2 (0.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.02%) to 40.947%

Changes Missing Coverage Covered Lines Changed/Added Lines %
spot_wrapper/spot_graph_nav.py 0 2 0.0%
Files with Coverage Reduction New Missed Lines %
spot_wrapper/spot_dance.py 1 18.52%
spot_wrapper/calibration/calibration_util.py 1 0.0%
Totals Coverage Status
Change from base Build 10890370088: -0.02%
Covered Lines: 1868
Relevant Lines: 4562

💛 - Coveralls

Copy link
Collaborator

@IoTDan can you run pre-commit install and pre-commit run --all on this branch, then re-push? There is a minor linting error

Copy link
Collaborator

@tcappellari-bdai tcappellari-bdai left a comment

Choose a reason for hiding this comment

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

good to merge after linting

@IoTDan
Copy link
Contributor Author

IoTDan commented Sep 26, 2024

@tcappellari-bdai @khughes-bdai pre-commit run and new commit submitted. thanks!

@tcappellari-bdai tcappellari-bdai merged commit c7b1696 into bdaiinstitute:main Sep 26, 2024
6 checks passed
@IoTDan IoTDan deleted the spot_lease branch September 26, 2024 14:15
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