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(shared-data): H/S labware offsets and TD calibration adapter measurements #13604

Open
wants to merge 3 commits into
base: edge
Choose a base branch
from

Conversation

sanni-t
Copy link
Member

@sanni-t sanni-t commented Sep 20, 2023

Overview

We were mistakenly using the z- labware offset for labware (adapters) on H/S on a Flex as the 'unscrewed' position of the heater-shaker's labware bed. When you screw in an adapter, the labware bed dips down a little so this should be included in the labware offset. (so the z-offset on Flex changes from z=18.95 to z=18.35). This value was also a little off for the h/s on OT2 so corrected that as well.

Also found small mistake in temp module calibration block's well's measurements.

NOTE: This is not too critical for 7.0.0. Because these offsets/mistakes gets compensated for with module calibration, it doesn't affect actual labware positions on the Flex much.

Test Plan

On Flex test that this PR:

  • improves (reduces) the z-offset found during H/S module calibration
  • improves the x, y offset found during temperature module calibration

On the OT2, test that this PR:

  • reduces the z-offset for a labware on H/S during LPC

Changelog

  • updated the x, y coordinates of well A1 of the temp module calibration block
  • updated the H/S labwareOffset z for both the OT-2 & Flex.

Review requests

  • Now that we have adapters separated out and have a stacking overlap measurement in the definitions, should we move this 'h/s bed dip' measurement as an overlap value for H/S adapters and adapter+labware combos? Or would that cause backwards compatibility issues with custom labware and json protocols?
  • I'm updating the TD calibration block values in-place since this goes in the fast-follow release (and will be the official release for all, except a few we will be helping with installations). But let me know if this should get a version bump.

Risk assessment

Very low. These are small improvements for positional accuracy. Doesn't touch any code.

@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (581bc6c) 71.32% compared to head (89ba176) 71.28%.
Report is 808 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #13604      +/-   ##
==========================================
- Coverage   71.32%   71.28%   -0.05%     
==========================================
  Files        2422     2426       +4     
  Lines       68142    68288     +146     
  Branches     7934     7998      +64     
==========================================
+ Hits        48605    48680      +75     
- Misses      17680    17725      +45     
- Partials     1857     1883      +26     
Flag Coverage Δ
app 68.81% <ø> (-0.15%) ⬇️

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

see 70 files with indirect coverage changes

@sfoster1 sfoster1 changed the base branch from chore_release-7.0.0 to chore_release-7.0.1 September 27, 2023 21:22
@sanni-t sanni-t changed the title fix(shared-data): TC labware offsets and TD calibration adapter measurements fix(shared-data): H/S labware offsets and TD calibration adapter measurements Oct 3, 2023
@sanni-t sanni-t marked this pull request as ready for review October 3, 2023 12:41
@sanni-t sanni-t requested a review from a team as a code owner October 3, 2023 12:41
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.

Looks good, thanks!

Now that we have adapters separated out and have a stacking overlap measurement in the definitions, should we move this 'h/s bed dip' measurement as an overlap value for H/S adapters and adapter+labware combos? Or would that cause backwards compatibility issues with custom labware and json protocols?

No strong opinion from me.

If the springy plate always gets depressed by the same distance, regardless of the adapter, I think it's reasonable to keep it as you have it.

However, let's definitely document this in the module schema.

I'm updating the TD calibration block values in-place since this goes in the fast-follow release (and will be the official release for all, except a few we will be helping with installations). But let me know if this should get a version bump.

I actually think it's always going to be appropriate to change these calibration adapters in-place. It's not like they're "real labware" that people load in their protocols. They're an internal implementation detail of module calibration. They're only labware definitions because that let us share the geometry calculation code easier, I guess.

"z": 68.275
"z": 68.525
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a note to the module schema saying what this physically corresponds to for the Heater-Shaker?

@sanni-t sanni-t changed the base branch from chore_release-7.0.1 to edge October 19, 2023 19:08
@sanni-t sanni-t requested a review from a team as a code owner October 19, 2023 19:08
@SyntaxColoring
Copy link
Contributor

Also reminder to unify this with Jeremy's changes to add module offsets to slot/cutout A3

@sanni-t sanni-t added the DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available label Feb 5, 2024
@sanni-t
Copy link
Member Author

sanni-t commented Feb 5, 2024

This PR is planned to be merged with rest of the deck positioning work in https://opentrons.atlassian.net/browse/RSS-444. Do not merge until then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants