Skip to content
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, shared-data): add location-based gripper offsets to final labware movement offsets #13186

Merged

Conversation

sanni-t
Copy link
Member

@sanni-t sanni-t commented Jul 27, 2023

Closes RLAB-295

Overview

Takes the location-based offsets we have been using in protocols and puts them in definitions so that they are included by default while calculating total gripper offsets.

Test Plan

  • Test the well-tested science protocols that do labware movement with gripper and verify that they work the same without any offsets provided in the protocol.

Changelog

  • adds a new field of gripperOffsets to deck, labware & module schemas
  • updates all the relevant definition models
  • adds offset getters to engine module & labware state views
  • updates the final gripper offset calculations to include these offsets

Review requests

  • make sure the calculations are being done correctly
  • does the shape of the new fields look okay?
  • Updated definition models look fine or can they be improved?

Risk assessment

Medium. The offset calculations are unit tested & will be robot tested well so that's not a risk. But we should make sure the actual offsets being added are correct.

@codecov
Copy link

codecov bot commented Jul 27, 2023

Codecov Report

Merging #13186 (4658923) into internal-release_0.14.0 (fcab19c) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@                     Coverage Diff                     @@
##           internal-release_0.14.0   #13186      +/-   ##
===========================================================
+ Coverage                    72.51%   72.53%   +0.01%     
===========================================================
  Files                         2392     2392              
  Lines                        66004    66032      +28     
  Branches                      7327     7327              
===========================================================
+ Hits                         47865    47895      +30     
+ Misses                       16392    16391       -1     
+ Partials                      1747     1746       -1     
Flag Coverage Δ
app 71.31% <ø> (+<0.01%) ⬆️
g-code-testing 96.44% <ø> (ø)
labware-library 49.17% <ø> (ø)
notify-server 89.13% <ø> (ø)
protocol-designer 47.30% <ø> (ø)
shared-data 79.02% <100.00%> (+0.21%) ⬆️
step-generation 87.18% <ø> (ø)

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

Files Changed Coverage Δ
api/src/opentrons/motion_planning/waypoints.py 100.00% <ø> (ø)
...pi/src/opentrons/protocol_engine/state/geometry.py 100.00% <ø> (ø)
api/src/opentrons/protocol_engine/state/labware.py 100.00% <ø> (ø)
api/src/opentrons/protocol_engine/state/modules.py 91.48% <ø> (ø)
api/src/opentrons/protocol_engine/types.py 97.56% <ø> (ø)
...ata/python/opentrons_shared_data/deck/dev_types.py 100.00% <100.00%> (ø)
.../python/opentrons_shared_data/labware/dev_types.py 100.00% <100.00%> (ø)
...pentrons_shared_data/labware/labware_definition.py 100.00% <100.00%> (ø)
...a/python/opentrons_shared_data/module/dev_types.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

Comment on lines 302 to 275
"required": [
"pickUpOffset",
"dropOffset"
]
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

add default as required

},
total=False,
Copy link
Member Author

Choose a reason for hiding this comment

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

Change this assigned TypedDict to class-based and see if anything breaks. If not, switch to that and split it into required vs optional typed dicts

Copy link
Member Author

Choose a reason for hiding this comment

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

punting this to later

@sanni-t sanni-t changed the base branch from api-refactor_labware_movement to internal-release_0.14.0 July 28, 2023 14:43
@sanni-t sanni-t marked this pull request as ready for review July 28, 2023 14:43
@sanni-t sanni-t requested a review from a team as a code owner July 28, 2023 14:44
@sanni-t sanni-t force-pushed the RLAB-295-add-contextual-gripper-offsets branch from 2e19aa3 to 71156f1 Compare July 28, 2023 14:46
Copy link
Member Author

Choose a reason for hiding this comment

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

Tagging @andySigler @ChrisYarka for verification

Copy link
Member Author

Choose a reason for hiding this comment

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

Tagging @andySigler @ChrisYarka for verification

Copy link
Member Author

Choose a reason for hiding this comment

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

Tagging @andySigler @ChrisYarka for verification

Copy link
Member Author

Choose a reason for hiding this comment

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

Tagging @andySigler @ChrisYarka for verification

Copy link
Contributor

@b-cooper b-cooper left a comment

Choose a reason for hiding this comment

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

Changes look good to me 👍

Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

Here are my results from testing the PR as-is, and from testing it with adjusted offsets that @andySigler gave me in this spreadsheet.

Pickup/drop location Sanniti's original offsets Andy's offsets
armadillo_96_wellplate_200ul_pcr_full_skirt on bare deck slot
armadillo_96_wellplate_200ul_pcr_full_skirt on thermocyclerModuleV2 ❌ In the jaw-open immediately after the drop, the jaws collide with the TC. Pickup not attempted. ❌ Same.
armadillo_96_wellplate_200ul_pcr_full_skirt on temperatureModuleV2 ❌ During both pickup and drop, it collides with the ~2mm lip around the edge of the module's slot. ❌ Same.
armadillo_96_wellplate_200ul_pcr_full_skirt on magneticBlockV1 🤷‍♂️ It seems to think the labware sits ~2 mm higher than it actually does. 🤷‍♂️ Same.
corning_384_wellplate_112ul_flat on opentrons_universal_flat_adapter on heaterShakerModuleV1
armadillo_96_wellplate_200ul_pcr_full_skirt on opentrons_96_pcr_adapter on heaterShakerModuleV1

Here are the Python files that I used.

@sanni-t sanni-t requested a review from a team as a code owner July 31, 2023 19:52
@SyntaxColoring
Copy link
Contributor

Recap from in-person work with @sanni-t, @andySigler, and @jbleon95:

  • thermocyclerModuleV2: A pickup and drop offset of z=4.6 avoids the collision, and happens to align with the official value for how far up the Thermocycler pops the labware. We've added that to this PR.
  • magneticBlockV1: This problem will remain unresolved for this PR.
  • temperatureModuleV2: This problem will remain unresolved for this PR. This might be lower-priority because realistic protocols would load the PCR plate atop an adapter atop the module, which would happen to avoid this problem. We couldn't test with that setup because that adapter hasn't yet been split into its own definition.

Thinking continues on where we should take this work in the medium/long-term.

  • If you pick up closer to the top of a labware, theoretically, you should also drop it higher to compensate.
  • Defining these offsets like we are now is probably not appropriate for labware other than armadillo_96_wellplate_200ul_pcr_full_skirt, with different heights.

@sanni-t sanni-t merged commit 534f0f9 into internal-release_0.14.0 Aug 1, 2023
@SyntaxColoring SyntaxColoring deleted the RLAB-295-add-contextual-gripper-offsets branch September 8, 2023 20:42
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