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 WorldObjectService, SpotCheckService, unit tests, Docker CI build pipeline #126

Merged
merged 80 commits into from
Aug 6, 2023

Conversation

jeremysee2
Copy link
Contributor

@jeremysee2 jeremysee2 commented Apr 19, 2023

The corresponding change in spot_wrapper is at: bdaiinstitute/spot_wrapper#6

jeremysee2 and others added 30 commits February 24, 2023 07:32
* added static Typing, unit tests for ros_helpers and graph_nav_util, refactored spot_wrapper.py into separate modules, added Github Actions CI pipeline, tested on Spot 3.2.0 and ROS Noetic
* added static Typing, unit tests for ros_helpers and graph_nav_util, refactored spot_wrapper.py into separate modules, added Github Actions CI pipeline, tested on Spot 3.2.0 and ROS Noetic
* added ROS unit testing of publishers, services, action servers
* Added spot_check ROS service 
---------

Co-authored-by: Ming Jie See <>
* publish pointcloud from VLP16

* add unit tests

---------

Co-authored-by: Yoshiki Obinata <mqcmd196@hotmail.co.jp>
Co-authored-by: Michal Staniaszek <m.staniaszek@gmail.com>
* add docker_export Github action
Sync with upstream, add unit tests for Dock ROS action
@jeremysee2
Copy link
Contributor Author

Added docs for Arm, EAP, GraphNav and Docker.

Copy link
Owner

@heuristicus heuristicus left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this. I think the main thing to ask is that you add some comments to functions that you've added. The driver in general is fairly sparse on comments in the code and I want to start improving that. The same applies to messages and services, which are not particularly well commented either.

I still need to:

  • test it on the robot in conjunction with the wrapper changes
  • check docker stuff

Comment on lines 23 to 24
depth_in_visual: True
mode_parent_odom_tf: vision
Copy link
Owner

Choose a reason for hiding this comment

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

Be specific about what these parameters are for.

@@ -1,7 +1,7 @@
<launch>
<arg name="username" default="dummyusername" />
<arg name="password" default="dummypassword" />
<arg name="hostname" default="192.168.50.3" />
<arg name="hostname" default="192.168.80.3" />
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
<arg name="hostname" default="192.168.80.3" />
<arg name="hostname" default="192.168.50.3" />

This would break things for anyone who was using the default value for this.

state, spot_wrapper, inverse_target_frame
)

transforms = sorted(tf_message.transforms, key=lambda x: x.child_frame_id)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
transforms = sorted(tf_message.transforms, key=lambda x: x.child_frame_id)
transforms = list(
reversed(sorted(tf_message.transforms, key=lambda x: x.header.frame_id))
)

Child frame has two instances of body, so the order is arbitrary. Sorting by header frame ID ad reversing the list makes the test work consistently.

state.system_fault_state.historical_faults, spot_wrapper
)
return system_fault_state_msg


def getBehaviorFaultsFromState(state, spot_wrapper):
"""Maps behavior fault data from robot state proto to ROS BehaviorFaultState message
def GetSpotCheckResultsMsg(
Copy link
Owner

Choose a reason for hiding this comment

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

Needs a comment.

return ros_resp


def GetFrameTreeSnapshotMsg(data: geometry_pb2.FrameTreeSnapshot) -> FrameTreeSnapshot:
Copy link
Owner

Choose a reason for hiding this comment

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

Comment.

return frame_tree_snapshot_msg


def GetAprilTagPropertiesMsg(
Copy link
Owner

Choose a reason for hiding this comment

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

Comment.

return april_tag_properties_msg


def GetImagePropertiesMsg(
Copy link
Owner

Choose a reason for hiding this comment

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

Comment.

return image_properties_msg


def GetWorldObjectsMsg(
Copy link
Owner

Choose a reason for hiding this comment

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

Comment.

return world_object_msg


def GetFrameNamesAssociatedWithObject(
Copy link
Owner

Choose a reason for hiding this comment

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

Comment.


def GetTFFromWorldObjects(
Copy link
Owner

Choose a reason for hiding this comment

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

Comment.

@jeremysee2
Copy link
Contributor Author

Added the requested comments, fixed ros_helpers_test.py (https://github.com/jeremysee2/spot_ros/blob/master/spot_driver/test/ros_helpers_test.py#L545), reverted the default hostname param in ad7687f

The modularity changes meant that images were no longer retrieved with async functions, which would result in very slow updates when running the driver over a wireless network (which isn't recommended, but should still update joint states and other things at a reasonable rate).
@heuristicus heuristicus merged commit e6a274e into heuristicus:master Aug 6, 2023
heuristicus added a commit that referenced this pull request Aug 6, 2023
heuristicus added a commit that referenced this pull request Aug 6, 2023
…lar wrapper (#126)

Updates spot_ros to use the modularised wrapper code, adds code to access world objects and spot check with the driver, adds some unit tests and a docker build pipeline.

co-authored-by: Michal Staniaszek <m.staniaszek@gmail.com>
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