Skip to content

Conversation

nicksanford
Copy link
Member

@nicksanford nicksanford commented Dec 8, 2023

Ticket

Note:

New methods are annotated as Experimental until the RPC definition of MoveOnGlobe is changed to be that of MoveOnGlobeNew.

Follow up PR will rename move_on_globe_new to move_on_globe & delete the old move_on_globe & remove the Experimental annotations.

I'm making these changes now so I can confirm that the docs update links to the python documentation work as expected.

Changes:

  • Add move_on_globe_new, marked as Experimental
  • Add stop_plan, marked as Experimental
  • Add get_plan, marked as Experimental
  • Add list_plan_statuses, marked as Experimental

Copy link
Member

@purplenicole730 purplenicole730 left a comment

Choose a reason for hiding this comment

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

Some nits, but LGTM!

destination (GeoPoint): The destination point
movement_sensor_name (ResourceName): The ``MovementSensor`` which will be used to check robot location
obstacles (Optional[Sequence[GeoObstacle]], optional): Obstacles to be considered for motion planning. Defaults to None.
heading (Optional[float], optional): Compass heading to achieve at the destination, in degrees [0-360). Defaults to None.
Copy link
Member

Choose a reason for hiding this comment

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

nit: in degrees [0-360) ends with ) instead of ].

Copy link
Member

Choose a reason for hiding this comment

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

This might be on purpose -- I assume it's set notation? 0 is inclusive, 360 is not?

Copy link
Member Author

Choose a reason for hiding this comment

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

nope, @purplenicole730 was right, was just a typo

obstacles (Optional[Sequence[GeoObstacle]], optional): Obstacles to be considered for motion planning. Defaults to None.
heading (Optional[float], optional): Compass heading to achieve at the destination, in degrees [0-360). Defaults to None.
linear_meters_per_sec (Optional[float], optional): Linear velocity to target when moving. Defaults to None.
angular_deg_per_sec (Optional[float], optional): Angular velocity to target when turning. Defaults to None.
Copy link
Member

Choose a reason for hiding this comment

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

What is the extra optional in Optional[float], optional for?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure, I was copying the pattern from move_on_globe which this is based on but which I didn't write. should this be just Optional[float]?

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

) -> ListPlanStatusesResponse:
"""**Experimental**
Returns the status of plans created by MoveOnGlobe requests that are executing
OR are part of an execution which changed it statewithin the a 24HR TTL OR until the robot reinitializes.
Copy link
Member

Choose a reason for hiding this comment

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

...which changed its state within a 24HR TTL...

extra = {}

if only_active_plans is None:
only_active_plans = False
Copy link
Member

Choose a reason for hiding this comment

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

Why not default it to False instead of None?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Member

@njooma njooma left a comment

Choose a reason for hiding this comment

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

LGTM % Nicole's nits

@nicksanford nicksanford merged commit 0a6ef27 into viamrobotics:main Dec 8, 2023
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.

3 participants