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

fix(api): use old TC Gen2 labwareOffset values in legacy core #14414

Merged
merged 2 commits into from
Feb 5, 2024

Conversation

sanni-t
Copy link
Member

@sanni-t sanni-t commented Feb 2, 2024

Fixes issue in RESC-196

Overview

In this PR, we changed how we calculate the final origin of any labware placed on a TC gen2. Previously we assumed that all labware sit 98.26mm above the OT2 slot; we call it the TC labware offset. This offset was correct for the Armadillo plate; while other pcr plates would sit +/-0.1mm differently. In 7.0.1, we updated this labware offset to be the top plane of metal cylinders of the thermocycler (108.96mm), and introduced a stackingOffsetWithModule parameter in the labware definitions to inform us of the distance a labware actually rests at from top of those cylinders. For labware that didn’t have these offsets, we added a fallback of using the previous labware offset of 98.26.

Issue is, we forgot to add that fallback in v2.13, which is a legacy core based API so now the backend assumes all labware sit at 108.96mm on TC Gen2, ignoring any stackingOffsetWithModule because the legacy core has no concept of stacking offsets.

I compiled all this, with options for solutions in this spreadsheet and we decided to go with option 1- revert the behavior of API v2.13 to what it was prior to the change in 7.0.1.

Test Plan

  • Load a TC Gen2 protocol on the OT2 without loading any previous LPC offsets and verify that pipettes go to the correct z height for the labware on the thermocycler, rather than moving 1cm above the labware.
  • Also verify that TC gen1 nor any other modules are affected.

Changelog

  • when loading module labware offsets in module geometry, it will now load separate, older offset values for TC gen2
  • other minor code refactoring

Review requests

  • Is it enough to save the older offsets in shared-data like that or do we want to have a better system for it?

Risk assessment

Medium. This adds/ reverts a change in positioning such that previous LPC offsets should not used or they'll result in pipettes crashing into labware bottom. We should also convey this to users.

@sanni-t sanni-t requested a review from a team as a code owner February 2, 2024 04:35
Copy link

codecov bot commented Feb 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (35a8b78) 68.24% compared to head (f162218) 68.12%.
Report is 21 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #14414      +/-   ##
==========================================
- Coverage   68.24%   68.12%   -0.12%     
==========================================
  Files        2514     2514              
  Lines       71865    72244     +379     
  Branches     9175     9307     +132     
==========================================
+ Hits        49041    49217     +176     
- Misses      20657    20843     +186     
- Partials     2167     2184      +17     
Flag Coverage Δ
app 64.58% <ø> (-0.28%) ⬇️
g-code-testing 96.48% <ø> (ø)
protocol-designer 37.92% <ø> (-0.10%) ⬇️
shared-data 75.25% <100.00%> (+<0.01%) ⬆️
step-generation 86.90% <ø> (ø)

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

Files Coverage Δ
...ta/python/opentrons_shared_data/module/__init__.py 93.75% <100.00%> (+0.20%) ⬆️

... and 17 files with indirect coverage changes

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Changes and cocnept LGTM

@sanni-t sanni-t merged commit dcdd2b5 into edge Feb 5, 2024
53 checks passed
@sanni-t sanni-t deleted the api-fix_tc_gen2_labware_positioning_on_legacy_core branch February 5, 2024 15:15
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.

2 participants