-
Notifications
You must be signed in to change notification settings - Fork 179
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
feat(api): Rotate the calibrated module offset if the module was physically moved and rotated. #13441
feat(api): Rotate the calibrated module offset if the module was physically moved and rotated. #13441
Conversation
… the DeckSlotLocation a particular offset was generated in.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## chore_release-7.0.1 #13441 +/- ##
=======================================================
+ Coverage 71.27% 71.32% +0.04%
=======================================================
Files 2427 1588 -839
Lines 68280 52777 -15503
Branches 7998 3443 -4555
=======================================================
- Hits 48669 37643 -11026
+ Misses 17731 14599 -3132
+ Partials 1880 535 -1345
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
api/src/opentrons/protocol_engine/commands/calibration/calibrate_module.py
Show resolved
Hide resolved
A = ( | ||
DeckSlotName.SLOT_D1, | ||
DeckSlotName.SLOT_C1, | ||
DeckSlotName.SLOT_B1, | ||
DeckSlotName.SLOT_A1, | ||
) | ||
B = (DeckSlotName.SLOT_D3, DeckSlotName.SLOT_C3, DeckSlotName.SLOT_B3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do stick with this basic idea of implementing _needs_rotation()
with a fixed set of locations, can we:
- Change
A
andB
toCOL_1
andCOL_3
? "A" and "B" are confusing to me because they look like they're referring to row A and row B. - Add
DeckSlotName.SLOT_A3
, where the fixed trash currently is? I don't want us to forget it when the trash is made movable, and I think it should be harmless to add it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use the slot transform for this? That would simplify a lot of the logic. The explicit "which slots need rotation" considerations are baked into the module definitions (though they may need added entries for flex-style names).
The slot transforms in the module definitions are 3d affine transforms in encoded matrix form; you can either use them as-is by augmenting the calibration offset to 1x4 with a 0
to get only the linear-transform-part or deaugment the affine transforms back to 3d linear transforms and multiply as normal.
Discussed in person with @vegano1 and @sfoster1 : we can't use slot transforms for rotating the calibration offset since that's not what the slot transforms are used as. The slot transforms are entities that will convert the default So the decision is to apply a separate rotation to just the calibration offset if the saved calibration was done on a slot in a different column than the current location. Then this rotated calibration offset will be added to the nominal, slot transformed module offset to get the final, calibrated offset vector for a labware on the module. |
a680b77
to
b8dd3f3
Compare
remove the redundant modules.get_module_offset since the geometries view now handles a lot of that functionality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good once we get some tests for the rotation process and logic in there!
api/src/opentrons/hardware_control/modules/module_calibration.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
As @sfoster1 mentioned, we should add a test that tests the rotation of the offsets.
Overview
When a module is calibrated, the generated module offset vector is valid for the current location the module is in. However, if the module is physically moved and rotated across the deck then the original calibrated offset vector is no longer valid. So we need to apply a rotation of 180 degrees to the calibrated module offset vector if the module was ever moved and rotated.
Test Plan
CalibrateModule
PE command over HTTP.Changelog
ModuleOffsetData
type to containModuleOffsetVector
andDeckSlotLocation
data.CalibrateModuleResult
PE command now returnsDeckSlotLocation
in addition tomoduleOffset
to be used inModuleStore
.ModuleStore
now usesModuleOffsetData
instead ofModuleOffsetVector
since we want to store both the offset and the deck slot location.get_module_calibration_offset
function now rotates the calibrated module offset by 180 degrees if the module was physically moved and rotated to a qualifying deck slot different from the original calibrated location.slot
member in theModuleCalibrationOffset
class into a required field since you won't have an offset without performing a calibration.Optional[ModuleCalibrationOffset]
instead of a default object, as some modules just haven't been calibrated yet and we don't want to assume the slot they were calibrated in.Review requests
_needs_rotation
function as this is a fixed set of locations we use to determine if a rotation is needed or not, so it feels rather brittle. I would love some feedback on this, maybe there's a better way of doing this.Risk assessment