feat(api): Add RobotContext movement commands #16403

Laura-Danielle · 2024-10-02T19:44:20Z

Overview

In this PR, you'll find a couple of major changes including hooking up the RobotContext with the protocol engine, adding the three movement commands (PLAT-351, PLAT-352, PLAT-449, PLAT-452, PLAT-455) in the context as well as http commands.

Remaining work

Gate behind 2.21
Disable for OT-2
Add tests for: robot_core, movement, gantry_mover, motion_utils functions and protocol engine commands
Update absolute motion_utils function to properly handle critical point

Test Plan and Hands on Testing

Robot has been tested with pipette mounts and will need to be tested with the gripper mount.

Changelog

Add robot_context movement commands
Add robot/moveTo, robot/moveAxesRelative and robot/moveAxesTo protocol engine commands
Add a robot core to share between the robot context and moveTo command
Update the ot3controller to handle sending moves to the Q motor if it is requested
Bump the command schema
Add relevant tests

Note

The axis_coordinates_for function can only properly handle Location types at this moment. There will be a follow-up PR to address modules and slots.

Review requests

Double check that routing makes sense and that I'm not missing any movement conversions before passing off to the hardware controller.

Risk assessment

Low. This is a use at your own risk API.

sfoster1

inline comments. also i think something got lost in moveAxes vs moveAxesRelative, it seems like the gantry_mover function is missing an argument or a separate implementation for relative moves

sfoster1 · 2024-10-03T13:37:48Z

api/src/opentrons/protocol_engine/execution/gantry_mover.py

+
+        Args:
+            axis_map: The mapping of axes to command.
+            relative_move: Specifying whether a relative move needs to be handled or not.


Updated the description, let me know if it makes more sense now.

api/src/opentrons/protocol_engine/commands/robot/move_axes_relative.py

api/src/opentrons/protocol_engine/types.py

api/src/opentrons/types.py

sfoster1 · 2024-10-03T13:45:28Z

api/src/opentrons/protocol_api/core/engine/robot.py

+from opentrons.protocol_api.core.robot import AbstractRobot
+
+_AXIS_TYPE_TO_MOTOR_AXIS = {
+    AxisType.X: MotorAxis.X,


this kind of seems like it should be in the engine

It is in the engine, or did you mean a specific file?

This isn't the engine - this is the protocol api engine bindings. My point is really that in my opinion, we shouldn't need to move AxisType to opentrons.types; instead, we should push MotorAxis upwards. Types that are used for the hardware controller shouldn't be part of user API.

OK I guess that makes sense as well. I'll make the change, but as a PR based off this one since there is already a lot going on and there will be a decent amount of import changes.

api/src/opentrons/protocol_api/core/legacy/legacy_protocol_core.py

sfoster1 · 2024-10-03T13:48:14Z

api/src/opentrons/protocol_api/robot_context.py

-        abs_axis_map: Dict[hw_types.Axis, hw_types.AxisMapValue],
-        velocity: float,
-        critical_point: Optional[hw_types.CriticalPoint],
+        axis_map: Union[AxisMapType, StringAxisMap],


should these be hardware axes or engine axes? I think we should probably use engine axes, since this goes to the engine and that's what it uses, and also that will keep it the same as json protos

See comment from here

TamarZanzouri · 2024-11-01T17:22:57Z

api/src/opentrons/protocol_api/robot_context.py

@@ -69,9 +150,54 @@ def open_gripper_jaw(self) -> None:
        raise NotImplementedError()

    def axis_coordinates_for(


random question, is this intended to be called from a protocol before using move_axes_relative or move_axes_to?

move_axes_relative can be called directly with a user dict, but axis_coordinates_for is intended to help users build a proper absolute position axis map from location. In a follow-up PR I will also build out the slot and module locations but I felt like there was already a lot here and decided not to address it as we only need "location" at the moment for the evo protocol.

api/src/opentrons/protocol_engine/execution/gantry_mover.py

api/src/opentrons/protocol_api/protocol_context.py

api/src/opentrons/protocol_api/robot_context.py

TamarZanzouri

a few nips but overall looks super intuitive and makes total sense. I just want to make sure we dont need to update the state right? (sorry I was not part of the convo's of this feature)

sfoster1

There's a couple last little things we need to do. Some of them can be todos, but we definitely should update the command schema since it'll be quick.

I'm also a little concerned about the big changes to ot3controller. That's all to allow users to specify gear motor motions from the robot context, right? could we split that part out of this pr?

api/src/opentrons/hardware_control/backends/ot3controller.py

api/src/opentrons/hardware_control/ot3api.py

sfoster1 · 2024-11-05T22:23:24Z

api/src/opentrons/hardware_control/ot3api.py

@@ -1228,7 +1230,9 @@ async def move_axes(  # noqa: C901
                    message=f"{axis} is not present", detail={"axis": str(axis)}
                )

+        self._log.info(f"Attempting to move {position} with speed {speed}.")


note which coordinates this is in in the log please

sfoster1 · 2024-11-05T22:28:01Z

api/src/opentrons/protocol_engine/commands/robot/move_axes_relative.py

+        # TODO (lc 08-16-2024) implement `move_axes` for OT 2 hardware controller
+        # and then we can remove this validation.
+        ensure_ot3_hardware(self._hardware_api)
+


another thing we need to add here, and in the other commands, is that we changed the way the engine keeps track of positions to be based on StateUpdate objects that command implementations return. specifically, these will need to return state_updates with set_pipette_location() as aspirate now does for protocol engine commands after this one to properly know where the pipette currently is. we can have a todo for this, but it will be outright broken until then.

I mean it would still execute the movements no? We have tickets for updating state for the robot context and I'd rather not add more to the PR than necessary.

Yeah, they will. It'll just move weird after.

shared-data/command/schemas/11.json

sfoster1

Okay, I feel great about this now. Thanks for bearing with me, and thanks for splitting that stuff out. Great work!

sfoster1 · 2024-11-07T22:59:06Z

api/src/opentrons/protocol_engine/commands/robot/move_axes_relative.py

+        # TODO (lc 08-16-2024) implement `move_axes` for OT 2 hardware controller
+        # and then we can remove this validation.
+        ensure_ot3_hardware(self._hardware_api)
+


Yeah, they will. It'll just move weird after.

… and add tests

Laura-Danielle requested review from ecormany, sfoster1 and CaseyBatten October 2, 2024 19:45

sfoster1 requested changes Oct 3, 2024

View reviewed changes

Laura-Danielle force-pushed the PLAT-351-move-axes-to-position branch from 52bd17b to b91630b Compare November 1, 2024 14:23

Laura-Danielle requested a review from sfoster1 November 1, 2024 15:37

Laura-Danielle marked this pull request as ready for review November 1, 2024 15:38

Laura-Danielle requested review from a team as code owners November 1, 2024 15:38

TamarZanzouri reviewed Nov 1, 2024

View reviewed changes

api/src/opentrons/protocol_engine/execution/gantry_mover.py Show resolved Hide resolved

TamarZanzouri reviewed Nov 1, 2024

View reviewed changes

api/src/opentrons/protocol_engine/execution/gantry_mover.py Outdated Show resolved Hide resolved

ecormany reviewed Nov 1, 2024

View reviewed changes

api/src/opentrons/protocol_api/protocol_context.py Outdated Show resolved Hide resolved

ecormany reviewed Nov 1, 2024

View reviewed changes

api/src/opentrons/protocol_api/robot_context.py Outdated Show resolved Hide resolved

TamarZanzouri approved these changes Nov 1, 2024

View reviewed changes

Laura-Danielle force-pushed the PLAT-351-move-axes-to-position branch from 632b75e to 5fa07d7 Compare November 3, 2024 17:58

sfoster1 requested changes Nov 5, 2024

View reviewed changes

Laura-Danielle force-pushed the PLAT-351-move-axes-to-position branch 2 times, most recently from 5d0e3c8 to 02b8dde Compare November 7, 2024 22:01

Laura-Danielle changed the base branch from edge to PLAT-351-allow-Q-motor-moves November 7, 2024 22:03

Laura-Danielle force-pushed the PLAT-351-move-axes-to-position branch from 02b8dde to 8a6873d Compare November 7, 2024 22:07

sfoster1 approved these changes Nov 7, 2024

View reviewed changes

Base automatically changed from PLAT-351-allow-Q-motor-moves to edge November 8, 2024 20:58

Laura-Danielle force-pushed the PLAT-351-move-axes-to-position branch from 8a6873d to 0297272 Compare November 8, 2024 21:10

Laura-Danielle added 6 commits November 9, 2024 00:00

feat(hardware_control): allow Q moves to be processed by hw._move

73fd4d7

feat: add movement commands to robot_context

e1ab989

feat: add robot core

535a0d5

feat: add protocol engine commands for robot movement commands

966457f

feat: add new axis types

d1cf948

feat: Expose additional movement options in gantry mover and movement

d577abf

Laura-Danielle added 27 commits November 9, 2024 00:00

missed if statement block

3f15ba3

use the right MotorAxis key

9d5dcf7

fix: check that the axis is in the axis map

0e7ef61

fix: correctly handle Q motor moves in the function and add a test

7f98b78

pass the right locations from gantry mover to the hardware controller…

8a7338d

… and add tests

adding protocol engine command tests

90750c1

formatting

0499118

bump api version to 2.21

b8cf9bb

add logs for motion utilities

6e92b5b

wrapper hardware offsets as point

d1e716f

fix move_axes_to move, add more logs for move_axes_relative

e5d468d

make machine to deck conversion func public hw API

b8911f9

more linter fixes

aecb1c3

fix relative movements

e4ee72e

handle robot context loading skipping for legacy protocols

c93de27

additional attempt at fixing move rel

5ceb32d

fix tests, lint and formatting

028a49a

fixups from rebase

1336292

bump schemma to version 11

7e9440e

fix move group runner changes

b205a87

fix failing linters and formatting

4fae9a1

fix more linter errors

6113fe9

fix linter errors

7c2e804

refresh command schema

25f5916

add additional commands to schema 11

2b5dcd2

fixup from last rebase

6eeede9

more rebase fixes

1fb5978

Laura-Danielle force-pushed the PLAT-351-move-axes-to-position branch from 5271362 to 1fb5978 Compare November 8, 2024 22:00

Laura-Danielle merged commit 0efe0af into edge Nov 8, 2024
61 of 63 checks passed

Laura-Danielle deleted the PLAT-351-move-axes-to-position branch November 8, 2024 22:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): Add RobotContext movement commands #16403

feat(api): Add RobotContext movement commands #16403

Laura-Danielle commented Oct 2, 2024 •

edited

Loading

sfoster1 left a comment

sfoster1 Oct 3, 2024

Laura-Danielle Nov 3, 2024

sfoster1 Oct 3, 2024

Laura-Danielle Oct 6, 2024

sfoster1 Oct 24, 2024

Laura-Danielle Nov 3, 2024

sfoster1 Oct 3, 2024

Laura-Danielle Nov 3, 2024

TamarZanzouri Nov 1, 2024 •

edited

Loading

Laura-Danielle Nov 3, 2024

TamarZanzouri left a comment

sfoster1 left a comment •

edited

Loading

sfoster1 Nov 5, 2024

sfoster1 Nov 5, 2024

Laura-Danielle Nov 7, 2024

sfoster1 Nov 7, 2024

sfoster1 left a comment

sfoster1 Nov 7, 2024

		@@ -69,9 +150,54 @@ def open_gripper_jaw(self) -> None:
		raise NotImplementedError()

		def axis_coordinates_for(

feat(api): Add RobotContext movement commands #16403

feat(api): Add RobotContext movement commands #16403

Conversation

Laura-Danielle commented Oct 2, 2024 • edited Loading

Overview

Remaining work

Test Plan and Hands on Testing

Changelog

Note

Review requests

Risk assessment

sfoster1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TamarZanzouri Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TamarZanzouri left a comment

Choose a reason for hiding this comment

sfoster1 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfoster1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Laura-Danielle commented Oct 2, 2024 •

edited

Loading

TamarZanzouri Nov 1, 2024 •

edited

Loading

sfoster1 left a comment •

edited

Loading