-
Notifications
You must be signed in to change notification settings - Fork 21
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
Refactoring code to be more modular #6
Conversation
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. |
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
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 |
Tested the code with the ROS1 and ROS2 wrapper, appears to work on the surface level (claim, stand, sit). |
Ran the BDAII tests with that and the associated HIL tests
|
Looks like SpotWrapper originally has a |
Also, I want to reiterate that all of the arm functions should use |
Currently, the It's not clear if If clearing the graph doesn't require claiming the lease, I recommend wrapping that call in an
|
Addressed with the addition of self._get_lease in the clear_graph method 651fed6 |
Addressed this in e6df481. The grasp_3d command comes from the manipulation API and doesn't seem to have an equivalent function. |
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. |
That's fine. You are basically doing the same thing that they do internally. Also, |
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 I'm not sure there is a difference between using the Also, I am also curious about having the |
self._init_current_graph_nav_state() | ||
self._estop_endpoint = None | ||
self._estop_keepalive = None | ||
self._robot_id = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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. |
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. |
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:
self.spot_wrapper.spot_arm.arm_stow()
instead ofself.spot_wrapper.arm_stow()
, adding the module name inbetweenlist_world_objects
call has to be updatedgraph_nav_util
has been folded into the GraphNav class, with the BD copyright notice marking where the copied code startsNew features:
navigate_init
,navigate_to
andnavigate_route
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.