fix(api): Exclude existing module offsets when running subsequent module calibrations #12667

vegano1 · 2023-05-09T19:06:48Z

Overview

Module Calibrations were failing because the calibration starting position was being shifted by the previous module calibration value for the given module, which meant that eventually the starting position would be way off-center and would not be able to find the calibration square. This PR addresses this issue and fixes another issue with non-connected modules (magnetic block) where we would throw ModuleNotConnectedError when trying to get the labware well position.

Closes: RCORE-738

Test Plan

Make sure subsequent module calibrations start at the same position for each module type
Make sure we don't throw ModuleNotConnectedError when getting the position for labware on top of the magnetic block.

Changelog

Fixed an issue where the starting point for module calibrations would be off by the previous module offset causing subsequent module calibrations to fail.
Fixed a potential issue in get_module_offset if the module is a non-connected module where just getting the well position would throw ModuleNotConnectedError, instead, we want to return the offset without the calibrated offset value.
Added get_raw_module_offset so we can get the loaded module offsets without transforms
Refactored the calibrate_module test script so it handles needing to home after robot-server restart, resetting states, and generally handling failure cases better.

Review requests

Make sure we are comfortable with the get_module_changes which affect get_well_position

Risk assessment

medium, while this affects module calibration directly any changes to get_module_offset also affect get_well_position

…ations

codecov · 2023-05-09T19:13:16Z

Codecov Report

Merging #12667 (8939865) into edge (4a2176f) will increase coverage by 0.26%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             edge   #12667      +/-   ##
==========================================
+ Coverage   73.29%   73.56%   +0.26%     
==========================================
  Files        1506     2260     +754     
  Lines       49424    62140   +12716     
  Branches     2998     6597    +3599     
==========================================
+ Hits        36227    45714    +9487     
- Misses      12741    14834    +2093     
- Partials      456     1592    +1136

Flag	Coverage Δ
app	`72.12% <ø> (+25.27%)`	⬆️
hardware-testing	`∅ <ø> (∅)`
labware-library	`49.74% <ø> (ø)`
notify-server	`89.13% <ø> (ø)`
protocol-designer	`46.34% <ø> (ø)`
shared-data	`76.35% <ø> (+0.99%)`	⬆️
step-generation	`88.20% <ø> (ø)`

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

Impacted Files	Coverage Δ
...pi/src/opentrons/protocol_engine/state/geometry.py	`100.00% <ø> (ø)`
api/src/opentrons/protocol_engine/state/modules.py	`91.48% <100.00%> (-0.05%)`	⬇️

... and 809 files with indirect coverage changes

sfoster1

The functionality looks good but I'm a little nervous doing this get_raw_module_offset and then subtracting it thing. It makes the code in the calibration command depend on the internal behavior of other modules. We're now depending on the implicit contract that the only offset from nominal in the well position is whatever get_raw_module_offset returns, which is a contract across two separate modules.

What if instead we have get_nominal_well_position, which probably internally depends on a get_nominal_module_position (and maybe raises NotImplementedError if you ask it for the nominal position of a well that's not something on a labware)? Now we've made the contract explicit: if you ask for get_nominal_well_position(), it must take into account nothing but the predefined geometry.

I think that would be a little more robust to regressions.

api/src/opentrons/protocol_engine/state/modules.py

api/src/opentrons/protocol_engine/commands/calibration/calibrate_module.py

… function so we can get the module offset vector with or without calibrated module offsets added get_labware_parent_position which has the same functionality as the previous get_labware_position but without the calibrated module offsets get_labware_position now uses the new get_labware_parent_position method and adds the calibrated module offsets with the new get_labware_offset_vector function this lets us use get_nominal_well_position to just get the uncalibrated well position when calibrating modules updated the thermocycler calibration adapter definition to match the source dimensions

…tps:https://github.com/Opentrons/opentrons into RCORE-738_clear-offset-on-new-module-calibration

sfoster1

This looks great to me aside from those couple nits! Nice work!

sfoster1 · 2023-05-12T18:20:26Z

api/src/opentrons/protocol_engine/state/modules.py

@@ -630,7 +630,16 @@ def get_dimensions(self, module_id: str) -> ModuleDimensions:
 """Get the specified module's dimensions."""
 return self.get_definition(module_id).dimensions

- def get_module_offset(
+ def get_module_offset_vector(self, module_id: str) -> ModuleOffsetVector:


nit: can we call this get_module_calibration_offset? i'm getting confused by having both this and get_module_offset

Agreed. The only reason get_labware_offset_vector() is named like it is is because the official terminology is that Labware Position Check saves labware offsets, not labware calibrations. (🤷) But "calibration" is more specific than just "offset," so it's nice to say "calibration" when we can.

sfoster1 · 2023-05-12T18:22:02Z

api/src/opentrons/protocol_engine/state/geometry.py

+ parent_pos = self.get_labware_parent_origin_position(labware_id)
+ origin_offset = self._labware.get_definition(labware_id).cornerOffsetFromSlot
+ well_def = self._labware.get_well_definition(labware_id, well_name)
+ return Point(


nit: could do return parent_pos + origin_offset + well_def + Point(x=0, y=0, z=well_def.depth) but it's up to you whether that's clearer or not

SyntaxColoring

Code makes sense to me. Thanks!

The overall approach also makes sense to me, but with a caveat: it only protects loadModule commands. That means, in the module calibration flows, we'll have to be careful that the app doesn't issue any other commands that would visit this module.

For example, if the app did a moveToWell command, that would still use the module's old calibration, right? The app does issue other commands like moveToWell during Labware Position Check. So we should make sure it doesn't need to do that for module calibration.

SyntaxColoring · 2023-05-12T18:41:34Z

api/src/opentrons/protocol_engine/state/modules.py

@@ -630,7 +630,16 @@ def get_dimensions(self, module_id: str) -> ModuleDimensions:
 """Get the specified module's dimensions."""
 return self.get_definition(module_id).dimensions

- def get_module_offset(
+ def get_module_offset_vector(self, module_id: str) -> ModuleOffsetVector:


Agreed. The only reason get_labware_offset_vector() is named like it is is because the official terminology is that Labware Position Check saves labware offsets, not labware calibrations. (🤷) But "calibration" is more specific than just "offset," so it's nice to say "calibration" when we can.

sanni-t

LGTM!
Just the two naming suggestions similar to @sfoster1 's

api/src/opentrons/protocol_engine/state/geometry.py

sanni-t · 2023-05-12T18:47:07Z

api/src/opentrons/protocol_engine/state/geometry.py

 )

+ def get_labware_parent_position(self, labware_id: str) -> Point:


..and change this to get_labware_parent_calibrated_position

kept it as is for consistency

sanni-t · 2023-05-12T19:09:19Z

The overall approach also makes sense to me, but with a caveat: it only protects loadModule commands. That means, in the module calibration flows, we'll have to be careful that the app doesn't issue any other commands that would visit this module.

For example, if the app did a moveToWell command, that would still use the module's old calibration, right? The app does issue other commands like moveToWell during Labware Position Check. So we should make sure it doesn't need to do that for module calibration.

Am I understanding your concern correctly that if a moveToWell command was issued immediately after a module calibration, in the same run, then the instrument might end up going to a position that was calculated with the calibration offsets that were loaded before the most recent module calibration (unless the module is reloaded or a new run is started after the calibration)? If so, then that's actually not what would happen. There is indeed a state update handler that would update the particular module's calibration offset in state after a successful module calibration so we'll start using the new offset right away for all moveTo commands.

SyntaxColoring · 2023-05-12T21:52:29Z

Am I understanding your concern correctly that if a moveToWell command was issued immediately after a module calibration, in the same run, then the instrument might end up going to a position that was calculated with the calibration offsets that were loaded before the most recent module calibration (unless the module is reloaded or a new run is started after the calibration)?

No—my concern is that if a moveToWell command is issued before the calibrateModule command, the moveToWell command will use an old, possibly bad offset, while the calibrateModule command will use the nominal position. Ideally, I think they'd both use the nominal position.

For example, imagine if the app had a step where it did moveToWell and then paused before doing calibrateModule, and said "OK user, I'm about to calibrate here." And then it...didn't calibrate there.

vegano1 added 2 commits May 9, 2023 14:32

fix(api): Remove module offsets when running subsequent module calibr…

6d29b2e

…ations

fix unit test

4dc763d

vegano1 requested a review from a team as a code owner May 9, 2023 19:06

vegano1 requested review from sfoster1, SyntaxColoring and sanni-t May 9, 2023 19:09

sfoster1 requested changes May 9, 2023

View reviewed changes

api/src/opentrons/protocol_engine/state/modules.py Outdated Show resolved Hide resolved

api/src/opentrons/protocol_engine/commands/calibration/calibrate_module.py Outdated Show resolved Hide resolved

vegano1 added 3 commits May 12, 2023 10:33

Merge branch 'RCORE-738_clear-offset-on-new-module-calibration' of ht…

ae92a93

…tps:https://github.com/Opentrons/opentrons into RCORE-738_clear-offset-on-new-module-calibration

vegano1 requested a review from sfoster1 May 12, 2023 14:47

fix hardware-testing lint

fb50a46

sfoster1 approved these changes May 12, 2023

View reviewed changes

SyntaxColoring approved these changes May 12, 2023

View reviewed changes

sanni-t approved these changes May 12, 2023

View reviewed changes

rename some functions

8939865

vegano1 merged commit e2ce496 into edge May 15, 2023
36 of 50 checks passed

vegano1 deleted the RCORE-738_clear-offset-on-new-module-calibration branch May 15, 2023 12:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(api): Exclude existing module offsets when running subsequent module calibrations #12667

fix(api): Exclude existing module offsets when running subsequent module calibrations #12667

vegano1 commented May 9, 2023 •

edited

Loading

codecov bot commented May 9, 2023 •

edited

Loading

sfoster1 left a comment

sfoster1 left a comment

sfoster1 May 12, 2023

SyntaxColoring May 12, 2023 •

edited

Loading

sfoster1 May 12, 2023

SyntaxColoring left a comment •

edited

Loading

SyntaxColoring May 12, 2023 •

edited

Loading

sanni-t left a comment

sanni-t May 12, 2023

vegano1 May 12, 2023

sanni-t commented May 12, 2023

SyntaxColoring commented May 12, 2023 •

edited

Loading

		)

		def get_labware_parent_position(self, labware_id: str) -> Point:

fix(api): Exclude existing module offsets when running subsequent module calibrations #12667

fix(api): Exclude existing module offsets when running subsequent module calibrations #12667

Conversation

vegano1 commented May 9, 2023 • edited Loading

Overview

Test Plan

Changelog

Review requests

Risk assessment

codecov bot commented May 9, 2023 • edited Loading

Codecov Report

sfoster1 left a comment

Choose a reason for hiding this comment

sfoster1 left a comment

Choose a reason for hiding this comment

sfoster1 May 12, 2023

Choose a reason for hiding this comment

SyntaxColoring May 12, 2023 • edited Loading

Choose a reason for hiding this comment

sfoster1 May 12, 2023

Choose a reason for hiding this comment

SyntaxColoring left a comment • edited Loading

Choose a reason for hiding this comment

SyntaxColoring May 12, 2023 • edited Loading

Choose a reason for hiding this comment

sanni-t left a comment

Choose a reason for hiding this comment

sanni-t May 12, 2023

Choose a reason for hiding this comment

vegano1 May 12, 2023

Choose a reason for hiding this comment

sanni-t commented May 12, 2023

SyntaxColoring commented May 12, 2023 • edited Loading

vegano1 commented May 9, 2023 •

edited

Loading

codecov bot commented May 9, 2023 •

edited

Loading

SyntaxColoring May 12, 2023 •

edited

Loading

SyntaxColoring left a comment •

edited

Loading

SyntaxColoring May 12, 2023 •

edited

Loading

SyntaxColoring commented May 12, 2023 •

edited

Loading