feat(api): add extension mount and axes to top level types #12671

sanni-t · 2023-05-10T13:18:16Z

Closes RLAB-219 & RLAB-231

Overview

Inside opentrons.types,

Adds Extension mount to Mount, MountType
Adds extension mount's axes to Axis.

Test Plan

Test homing an OT3's gripper mount with home command over http
Test that doing the above raises an error on OT2
Test that moveToMaintenancePosition with gripper mount, moves the gantry to the given x, y location, ignoring the z offset. (this will potentially fail until point2 in review requests is addressed).

Changelog

The only user-facing changes are:

Addition of EXTENSION type to Mount and MountType
Addition of Z_G and G axes to Axis type
protocol_context.load_instruments now raises error when Mount.EXTENSION is used for mount arg.

Client-facing change for HTTP layer is:

You can now specify extension_z and extension_jaw for axis in home command

Non-user/client facing changes mostly fall under these categories:

updating conversion from one type to another, now includes conversion of extension mount & axes types
hardware control layer changes to expect Mount and Axis to include extension mount and axes
for places that iterate over Mount type, restrict the iteration to OT2 mount types when using OT2, or restrict the iteration to pipette mounts only when the code is relevant to pipette mounts only.

Review requests

The main reason to add Extension mount's axes was to implement homing the gripper axes using HTTP home command. This simple requirement trickled down into needing to update the top level Axis type, which required updating the Mount & MountType types for keeping their inter-dependency valid. Do you think the chain of changes is justified and in the end is a change we want to make- i.e., we want types.Axis to reflect all the axes available- or would it be instead better to not have to update the top level types and keep the change restricted to just protocol engine? (basically by updating only engine.types.MotorAxis and handling homing the gripper axes with a special call to ot3api.home with axes specified in OT3Axis type)
The main reason to add Extension mount type was to allow moveToMaintenancePosition to move the gripper mount to maintenance position during attach/detach. But this cannot be fully supported without a change in hardware controller since there is no 'gripper mount' available to move when there is no gripper attached. So in essence we want to just move the gantry to a specified x, y location in order to perform gripper attachment. So one can argue that we don't necessarily need the extension mount type specified in order to do such a move.
- Either way, this would require a change to the hardware controller to either add a new 'move_gantry' method or allow ot3api.move_to to move the hardware mounting plate of gripper mount to a specific (x, y) location when executing move_to(mount=OT3Mount.GRIPPER, abs_position=..) without a gripper attached. Which way sounds better? If it's a significant change then I'll address it in a follow-up PR.

Risk assessment

Medium-High:

Mainly for the error that iterating over Mounts would now produce, but the probability of that happening in user protocols is low. I have gone through all the protocols in https://github.com/Opentrons/Protocols to make sure none of them will break because of the additional Mount in type. Also checked with QA & apps engg and verified that no known user or Opentrons-made protocol iterates over Mount types.
There is an additional low probability of an internal code doing this iteration over mounts and axis that I could have missed; hopefully extensive testing will catch any missed ones.
Then lastly internal testing scripts might also be relying on having just 2 mounts and 6 axes, those will also need to be updated.

sfoster1

Couple hardware-specific changes I'd like to see unfortunately

sfoster1 · 2023-05-10T17:50:16Z

api/src/opentrons/hardware_control/api.py

+        if axes:
+            assert all(
+                axis in Axis.ot2_axes() for axis in axes
+            ), "Received a non-OT2 axis for homing."


I think this is correct modulo using a more specific exception than AssertionError - I think we want like a NotSupportedByHardware error

sfoster1 · 2023-05-10T17:50:47Z

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

@@ -138,7 +138,7 @@ def _sanitize_attached_instrument(

        self._attached_instruments = {
            m: _sanitize_attached_instrument(attached_instruments.get(m))
-            for m in types.Mount
+            for m in types.Mount  # Check if addition of extension Mount creates issues for this


feels like we should just use ot2_mounts here?

sfoster1 · 2023-05-10T17:53:47Z

api/src/opentrons/hardware_control/types.py

+    X = 0  # Gantry X
+    Y = 1  # Gantry Y
+    Z = 2  # left pipette mount Z
+    A = 3  # right pipette mount Z


I don't want to change the ot3 stuff to use Z/A/B/C. I think we should handle Z_L, Z_R, P_L, P_R. I think this can be handled in one of two ways:

Just add them. Add per-machine iterators ot2_axes and flex_axes like we have for the mounts. It's non-unique now, that's fine.

Make it two types again but now they're top-level and have some sort of equivalence relationship

Sorry, not sure which change you are talking about here. There are still two different axis types- Axis and OT3Axis, and OT3Axis uses Z_L, Z_R, etc. Here I've only added comments and Gripper Z & Jaw to the existing Axis type.

…-motor-axis-type

codecov · 2023-05-11T13:19:17Z

Codecov Report

Merging #12671 (36654ca) into edge (b8ebbbe) will increase coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             edge   #12671      +/-   ##
==========================================
+ Coverage   73.35%   73.42%   +0.06%     
==========================================
  Files        2261     2267       +6     
  Lines       61480    62313     +833     
  Branches     6782     6666     -116     
==========================================
+ Hits        45098    45751     +653     
- Misses      14763    14951     +188     
+ Partials     1619     1611       -8

Flag	Coverage Δ
app	`71.66% <ø> (ø)`
g-code-testing	`96.44% <ø> (ø)`
notify-server	`89.13% <ø> (ø)`
protocol-designer	`46.35% <ø> (ø)`
react-api-client	`72.85% <ø> (ø)`
shared-data	`76.35% <ø> (+3.35%)`	⬆️
step-generation	`88.20% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
api/src/opentrons/hardware_control/api.py	`82.51% <ø> (ø)`
.../opentrons/hardware_control/backends/controller.py	`71.59% <ø> (ø)`
...c/opentrons/hardware_control/backends/simulator.py	`87.58% <ø> (-0.09%)`	⬇️
api/src/opentrons/hardware_control/types.py	`96.39% <ø> (-0.10%)`	⬇️
api/src/opentrons/protocol_api/protocol_context.py	`91.57% <ø> (ø)`
...i/src/opentrons/protocol_engine/errors/__init__.py	`100.00% <ø> (ø)`
...src/opentrons/protocol_engine/errors/exceptions.py	`100.00% <ø> (ø)`
...c/opentrons/protocol_engine/execution/equipment.py	`100.00% <ø> (ø)`
api/src/opentrons/protocol_engine/state/motion.py	`100.00% <ø> (ø)`
api/src/opentrons/protocol_engine/state/state.py	`100.00% <ø> (ø)`
... and 3 more

... and 56 files with indirect coverage changes

sfoster1

Ok, looks good to me based on a convo - let's merge Axis and OT3Axis in a followup!

…-motor-axis-type

sanni-t added 7 commits May 2, 2023 15:45

added gripper axes to MotorAxis and hardware Axis, updated mappers

05c0953

use only ot2 axes and mounts for ot2's hardware controller

a41bd3c

updated hw control, papi, engine & robot-server to use new axis & mount

0b71669

raise errors when using extension mount and axes incorrectly

5701ab3

use opentrons.types.MountType in instruments router

a441c88

update test

e6d1348

added todo

622e041

sfoster1 requested changes May 10, 2023

View reviewed changes

Merge branch 'edge' into RLAB-219-add-gripper-axes-to-protocol-engine…

5748877

…-motor-axis-type

sanni-t marked this pull request as ready for review May 11, 2023 20:45

sanni-t requested review from a team as code owners May 11, 2023 20:45

sfoster1 approved these changes May 11, 2023

View reviewed changes

sanni-t added 3 commits May 11, 2023 17:44

addressed review comments

8e9e77e

added mount axes selector, fixed axes homing in tc & H/S commands

050cc2a

Merge branch 'edge' into RLAB-219-add-gripper-axes-to-protocol-engine…

36654ca

…-motor-axis-type

sanni-t merged commit 61a3290 into edge May 17, 2023

sanni-t deleted the RLAB-219-add-gripper-axes-to-protocol-engine-motor-axis-type branch May 17, 2023 21:01

sanni-t mentioned this pull request Jun 1, 2023

refactor(api): merge OT3Axis into Axis #12834

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): add extension mount and axes to top level types #12671

feat(api): add extension mount and axes to top level types #12671

sanni-t commented May 10, 2023 •

edited

Loading

sfoster1 left a comment

sfoster1 May 10, 2023

sfoster1 May 10, 2023

sfoster1 May 10, 2023

sanni-t May 10, 2023

codecov bot commented May 11, 2023 •

edited

Loading

sfoster1 left a comment

feat(api): add extension mount and axes to top level types #12671

feat(api): add extension mount and axes to top level types #12671

Conversation

sanni-t commented May 10, 2023 • edited Loading

Overview

Test Plan

Changelog

Review requests

Risk assessment

sfoster1 left a comment

Choose a reason for hiding this comment

sfoster1 May 10, 2023

Choose a reason for hiding this comment

sfoster1 May 10, 2023

Choose a reason for hiding this comment

sfoster1 May 10, 2023

Choose a reason for hiding this comment

sanni-t May 10, 2023

Choose a reason for hiding this comment

codecov bot commented May 11, 2023 • edited Loading

Codecov Report

sfoster1 left a comment

Choose a reason for hiding this comment

sanni-t commented May 10, 2023 •

edited

Loading

codecov bot commented May 11, 2023 •

edited

Loading