Refactoring code to be more modular #6

jeremysee2 · 2023-04-18T21:45:46Z

This continues work on the ROS1 version of the Spot wrapper to modularize the code and add testing to the wrapper, from heuristicus/spot_ros#104

List of breaking changes to the ROS interface:

front/side/rear images now follow the ROS2 implementation of synchronous calls instead of AsyncPeriodicQuery
All spot_arm calls have to be updated (e.g. self.spot_wrapper.spot_arm.arm_stow() instead of self.spot_wrapper.arm_stow(), adding the module name inbetween
All docking calls have to be updated
All GraphNav calls have to be updated
All Images calls have to be updated
The list_world_objects call has to be updated
graph_nav_util has been folded into the GraphNav class, with the BD copyright notice marking where the copied code starts

New features:

GraphNav has been significantly refactored into navigate_init, navigate_to and navigate_route
Spot Check has been added

Currently, the only unit tests are for some parts of graph_nav_util, written in unittest and triggered automatically via Github Actions CI.

The PR is not ready to merge, requires testing and comments.

jeremysee2 · 2023-04-18T22:09:46Z

Hand images use the old AsyncPeriodicQuery, because I don't want the ROS interface to be aware of whether the arm exists; this should be handled directly by the Wrapper.

heuristicus · 2023-04-19T09:13:09Z

Thanks for the work you've put in on this. I'll take some time to review soon.

* use pytest instead * add pytest dependency * fail test on purpose * remove failing test

jeremysee2 · 2023-04-20T07:26:25Z

I've made PR's to prepare for the breaking changes, in both the ROS1 and ROS2 implementations of spot_ros:

ROS1: heuristicus/spot_ros#126
ROS2: bdaiinstitute/spot_ros2#41

…o main

jeremysee2 · 2023-04-21T14:35:17Z

Tested the code with the ROS1 and ROS2 wrapper, appears to work on the surface level (claim, stand, sit).

amessing-bdai · 2023-06-09T18:20:11Z

Ran the BDAII tests with that and the associated HIL tests

The bike repair test succeeded
Graph Nav test failed with the below error @kaiyu-zheng

[spot_ros2-1]   File "/workspaces/bdai/ws/build/spot_wrapper/spot_wrapper/spot_graph_nav.py", line 544, in clear_graph                                                                                             
[spot_ros2-1]     result = self._graph_nav_client.clear_graph(lease=self._lease.lease_proto)                                                                                                                       
[spot_ros2-1] AttributeError: 'SpotGraphNav' object has no attribute '_lease'

kaiyu-zheng · 2023-06-09T19:07:04Z

Ran the BDAII tests with that and the associated HIL tests

The bike repair test succeeded
Graph Nav test failed with the below error @kaiyu-zheng

[spot_ros2-1]   File "/workspaces/bdai/ws/build/spot_wrapper/spot_wrapper/spot_graph_nav.py", line 544, in clear_graph                                                                                             
[spot_ros2-1]     result = self._graph_nav_client.clear_graph(lease=self._lease.lease_proto)                                                                                                                       
[spot_ros2-1] AttributeError: 'SpotGraphNav' object has no attribute '_lease'

Looks like SpotWrapper originally has a self._lease field, and it has functions like getLease that set that field. How is this handled now?

amessing-bdai · 2023-06-09T19:11:47Z

Also, I want to reiterate that all of the arm functions should use block_until_arm_arrives instead of time.sleep

jeremysee2 · 2023-06-09T21:50:47Z

Ran the BDAII tests with that and the associated HIL tests

The bike repair test succeeded
Graph Nav test failed with the below error @kaiyu-zheng

[spot_ros2-1]   File "/workspaces/bdai/ws/build/spot_wrapper/spot_wrapper/spot_graph_nav.py", line 544, in clear_graph                                                                                             
[spot_ros2-1]     result = self._graph_nav_client.clear_graph(lease=self._lease.lease_proto)                                                                                                                       
[spot_ros2-1] AttributeError: 'SpotGraphNav' object has no attribute '_lease'

Looks like SpotWrapper originally has a self._lease field, and it has functions like getLease that set that field. How is this handled now?

Currently, the self._lease field is created when initializing GraphNav with a map, or when telling the robot to navigate to a specific waypoint. Clearing the graph (currently) does not attempt to claim the lease.

It's not clear if clear_graph requires the lease.

If clearing the graph doesn't require claiming the lease, I recommend wrapping that call in an if statement to avoid this problem. If it does, we can add in the following snippet to claim the lease:

self._lease = self._lease_wallet.get_lease()

jeremysee2 · 2023-06-09T23:07:00Z

Ran the BDAII tests with that and the associated HIL tests

The bike repair test succeeded

Graph Nav test failed with the below error @kaiyu-zheng
[spot_ros2-1]   File "/workspaces/bdai/ws/build/spot_wrapper/spot_wrapper/spot_graph_nav.py", line 544, in clear_graph                                                                                             
[spot_ros2-1]     result = self._graph_nav_client.clear_graph(lease=self._lease.lease_proto)                                                                                                                       
[spot_ros2-1] AttributeError: 'SpotGraphNav' object has no attribute '_lease'                                                                                                                                      
Looks like SpotWrapper originally has a self._lease field, and it has functions like getLease that set that field. How is this handled now?
Currently, the self._lease field is created when initializing GraphNav with a map, or when telling the robot to navigate to a specific waypoint. Clearing the graph (currently) does not attempt to claim the lease.

It's not clear if clear_graph requires the lease.

If clearing the graph doesn't require claiming the lease, I recommend wrapping that call in an if statement to avoid this problem. If it does, we can add in the following snippet to claim the lease:
self._lease = self._lease_wallet.get_lease()

Addressed with the addition of self._get_lease in the clear_graph method 651fed6

…ntil_arm_arrives SDK function

heuristicus · 2023-06-10T16:22:40Z

Also, I want to reiterate that all of the arm functions should use block_until_arm_arrives instead of time.sleep

Addressed this in e6df481. The grasp_3d command comes from the manipulation API and doesn't seem to have an equivalent function.

kaiyu-zheng · 2023-06-10T18:14:17Z

Ran the BDAII tests with that and the associated HIL tests

The bike repair test succeeded

Graph Nav test failed with the below error @kaiyu-zheng
[spot_ros2-1]   File "/workspaces/bdai/ws/build/spot_wrapper/spot_wrapper/spot_graph_nav.py", line 544, in clear_graph                                                                                             
[spot_ros2-1]     result = self._graph_nav_client.clear_graph(lease=self._lease.lease_proto)                                                                                                                       
[spot_ros2-1] AttributeError: 'SpotGraphNav' object has no attribute '_lease'                                                                                                                                      
Looks like SpotWrapper originally has a self._lease field, and it has functions like getLease that set that field. How is this handled now?
Currently, the self._lease field is created when initializing GraphNav with a map, or when telling the robot to navigate to a specific waypoint. Clearing the graph (currently) does not attempt to claim the lease.

It's not clear if clear_graph requires the lease.

If clearing the graph doesn't require claiming the lease, I recommend wrapping that call in an if statement to avoid this problem. If it does, we can add in the following snippet to claim the lease:
self._lease = self._lease_wallet.get_lease()

Since only one node can possess the lease to a Spot at a time, would it be better if the lease wallet is owned by the SpotWrapper object, instead of by the individual components? Correct me if I am misunderstanding something.

heuristicus · 2023-06-11T11:23:38Z

Since only one node can possess the lease to a Spot at a time, would it be better if the lease wallet is owned by the SpotWrapper object, instead of by the individual components? Correct me if I am misunderstanding something.

I think that because it's part of the same node, getting a lease object should just return the same values. But I'm not certain. I tried doing some stuff with the lease wallet previously and it didn't behave the way I expected, so there may be some complications.

amessing-bdai · 2023-06-11T15:27:25Z

Also, I want to reiterate that all of the arm functions should use block_until_arm_arrives instead of time.sleep

Addressed this in e6df481. The grasp_3d command comes from the manipulation API and doesn't seem to have an equivalent function.

That's fine. You are basically doing the same thing that they do internally. Also, spot_ros2 doesn't use the grasp_3d command and instead uses a more general manipulation_api command. I might still recommend that you have a timeout option similar to the existing block commands.

amessing-bdai · 2023-06-11T15:57:09Z

Since only one node can possess the lease to a Spot at a time, would it be better if the lease wallet is owned by the SpotWrapper object, instead of by the individual components? Correct me if I am misunderstanding something.

I think that because it's part of the same node, getting a lease object should just return the same values. But I'm not certain. I tried doing some stuff with the lease wallet previously and it didn't behave the way I expected, so there may be some complications.

I don't think whether it is part of the same node matters so much. I think because we created the lease client from a single robot instance, all leases acquired/taken have the same owner. I think it will be the same lease, but possibly have an updated request time. The only time it would be a new lease would be if something else took the lease (e.g. the tablet or another program) and then you took it back.

I'm not sure there is a difference between using the get_lease or just leaving the default value of None which says it will use the client's lease. Probably depends on whether they mean the lease_client's lease or the last lease that was passed into the graph_nav_client. When the various functions create subleases and pass those to the graph_nav_client is when I think having the lease_wallet matters.

Also, I am also curious about having the lease_keepalive internally and if there are any conflicts with the lease_keepalive that is held in SpotWrapper. Potentially there are times when there are two threads that are trying to keep the lease alive. Probably doesn't hurt in terms of messing with the lease, but just potentially extra computation and communication. Also, there are times where various functions shutdown the internal keepalive, usually just before they pass a sublease to the graph_nav_client, so I'd be curious if the external keepalive (which presumably is still running) creates a conflict.

amessing-bdai · 2023-06-12T11:30:51Z

spot_wrapper/wrapper.py

-            self._init_current_graph_nav_state()
+        self._estop_endpoint = None
+        self._estop_keepalive = None
+        self._robot_id = None


The property down below uses the robot_params dict instead of self._robot_id. If this isn't used anywhere they it should be removed to avoid confusion

I've made it so that the wrapper class no longer uses the convenience dict for accessing these values or returning them from properties. It still has the dict to pass to things which it instantiates.

…of the timesync endpoint and end time

heuristicus · 2023-06-26T16:26:15Z

I've fixed up some of the issues which were mentioned by @amessing-bdai and @myeatman-bdai. I'd like to get this merged and it would be good to know if there are any other concerns which would be considered blocking so I can try and fix them.

heuristicus · 2023-06-28T14:51:08Z

Per the discussion on slack, I will close this PR and split it out into individual PRs for each module which achieves the same result but is easier to test.

jeremysee2 added 15 commits April 18, 2023 18:48

refactored code into modules

adfb0b1

refactored image service

76f4899

cleanup imports

f026a3e

add unit tests for some graph_nav_util functions

88ae203

pip install requirements

be35078

typing changes

86a604f

absolute paths

f4e9e43

clone to specific directory

03c5997

check file structure for CI

89213d8

check files

1ffab67

check files

860c796

add test script

0097875

install spot wrapper

5f04353

install script

7a9811b

remove asyncimageservice

15addcd

jeremysee2 mentioned this pull request Apr 18, 2023

Draft: Refactoring code to be more modular heuristicus/spot_ros#125

Closed

use robot_params to share state variables

3fa6430

jeremysee2 mentioned this pull request Apr 19, 2023

Add WorldObjectService, SpotCheckService, unit tests, Docker CI build pipeline heuristicus/spot_ros#126

Merged

jeremysee2 added 2 commits April 19, 2023 17:41

Pytest replacement (#1)

55c68a1

* use pytest instead * add pytest dependency * fail test on purpose * remove failing test

replace print() with self._logger.error()

cf262e4

jeremysee2 mentioned this pull request Apr 20, 2023

Update breaking changes in spot_wrapper refactoring bdaiinstitute/spot_ros2#41

Closed

jeremysee2 added 4 commits April 21, 2023 13:18

image publishing works well

4928997

moved SPOT_CLIENT_NAME

0c2bb45

static typing for claim()

6779aa5

Merge branch 'main' of https://github.com/jeremysee2/spot_wrapper int…

53fd6bf

…o main

jeremysee2 changed the title ~~Draft: Refactoring code to be more modular~~ Refactoring code to be more modular Apr 21, 2023

jeremysee2 added 3 commits June 9, 2023 14:46

cleanup unused spot_image and renamed graphnav private methods

5d53c58

update graph_nav private methods

17dd90c

merge upstream changes

bb7c5c1

jeremysee2 added 2 commits June 9, 2023 23:57

custom arm not found Exception

0ec8556

_get_lease private method in graphNav

651fed6

heuristicus added 3 commits June 10, 2023 17:07

fix black

d64f65f

import ordering and removal of unused imports

7843a48

wait for arm commands to complete rather than sleeping, using block_u…

e6df481

…ntil_arm_arrives SDK function

amessing-bdai reviewed Jun 12, 2023

View reviewed changes

heuristicus added 5 commits June 18, 2023 13:30

no longer use convenience dict to access robot params in wrapper class

ea44635

missed some robot params usages in wrapper

7e9515c

Merge branch 'main' into jeremysee-main

8a9d27d

add block_until_manipulation_completes method

776e840

improve arm module comments, manipulation request actually makes use …

ef2174d

…of the timesync endpoint and end time

heuristicus requested review from myeatman-bdai and amessing-bdai June 22, 2023 10:51

Merge branch 'main' into jeremysee-main

94dcde5

heuristicus closed this Jun 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring code to be more modular #6

Refactoring code to be more modular #6

jeremysee2 commented Apr 18, 2023

jeremysee2 commented Apr 18, 2023

heuristicus commented Apr 19, 2023

jeremysee2 commented Apr 20, 2023

jeremysee2 commented Apr 21, 2023

amessing-bdai commented Jun 9, 2023

kaiyu-zheng commented Jun 9, 2023

amessing-bdai commented Jun 9, 2023

jeremysee2 commented Jun 9, 2023

jeremysee2 commented Jun 9, 2023

heuristicus commented Jun 10, 2023

kaiyu-zheng commented Jun 10, 2023 •

edited

Loading

heuristicus commented Jun 11, 2023 •

edited

Loading

amessing-bdai commented Jun 11, 2023

amessing-bdai commented Jun 11, 2023 •

edited

Loading

amessing-bdai Jun 12, 2023 •

edited

Loading

heuristicus Jun 18, 2023

heuristicus commented Jun 26, 2023

heuristicus commented Jun 28, 2023

Refactoring code to be more modular #6

Refactoring code to be more modular #6

Conversation

jeremysee2 commented Apr 18, 2023

jeremysee2 commented Apr 18, 2023

heuristicus commented Apr 19, 2023

jeremysee2 commented Apr 20, 2023

jeremysee2 commented Apr 21, 2023

amessing-bdai commented Jun 9, 2023

kaiyu-zheng commented Jun 9, 2023

amessing-bdai commented Jun 9, 2023

jeremysee2 commented Jun 9, 2023

jeremysee2 commented Jun 9, 2023

heuristicus commented Jun 10, 2023

kaiyu-zheng commented Jun 10, 2023 • edited Loading

heuristicus commented Jun 11, 2023 • edited Loading

amessing-bdai commented Jun 11, 2023

amessing-bdai commented Jun 11, 2023 • edited Loading

amessing-bdai Jun 12, 2023 • edited Loading

Choose a reason for hiding this comment

heuristicus Jun 18, 2023

Choose a reason for hiding this comment

heuristicus commented Jun 26, 2023

heuristicus commented Jun 28, 2023

kaiyu-zheng commented Jun 10, 2023 •

edited

Loading

heuristicus commented Jun 11, 2023 •

edited

Loading

amessing-bdai commented Jun 11, 2023 •

edited

Loading

amessing-bdai Jun 12, 2023 •

edited

Loading