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(labware-library): labware creator adapter z offset fit #17347

Merged
merged 4 commits into from
Jan 27, 2025

Conversation

alexjoel42
Copy link
Contributor

@alexjoel42 alexjoel42 commented Jan 24, 2025

Overview

Addressing a customer reported bug where labware's stacked offsets while loaded onto just a module don't account for the module's height.

Also closes https://opentrons.atlassian.net/browse/RESC-382

Test Plan and Hands on Testing

Make custom labware using the module's hights and then check the offsets

Changelog

One line change

Review requests

I want to verify that this doesn't have any wider scope code risk. I don't understand Labware-Creator very well as an ecosystem yet.

Risk assessment

Medium

…acking offset with module

We had a bug in #17238

BREAKING CHANGE: This will change how the stacking offset is calculated so any quirky customer
workarounds might stop working. Also if the changes needed to be bigger that will also be tricky

GitHub ticket 17238
@alexjoel42 alexjoel42 requested a review from jerader January 24, 2025 23:14
@alexjoel42 alexjoel42 requested a review from a team as a code owner January 24, 2025 23:14
Comment on lines 130 to 133
z:
fields.labwareZDimension -
parseFloat(String(z)) +
moduleDefinition.dimensions.bareOverallHeight,
Copy link
Collaborator

@jerader jerader Jan 27, 2025

Choose a reason for hiding this comment

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

does this get us the right number if you smoke test it? make -C labware-library dev launches the labware-library dev environment.

Copy link
Contributor Author

@alexjoel42 alexjoel42 Jan 27, 2025

Choose a reason for hiding this comment

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

fields.labwareZDimension -
 parseFloat(String(z)) +
moduleDefinition.dimensions.bareOverallHeight,

I tested it in an example and so far so good!
thermobareoverall_96_wellplate_2000ul.json

  1. fields.labwareZDimension = 43
  2. Total Height (in this case parseFloat String(z) ) = 55
  3. moduleDefinition.dimensions.bareOverallHeight = 45

Stacking Offset should equal 33.

Going to review with @sanni-t how to best test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we're in good shape though.

fields.labwareZDimension -
 parseFloat(String(z)) +
moduleDefinition.dimensions.bareOverallHeight,

fields.labwareZDimension = 10
parseFloat(String(z)) = 108
moduleDefinition.dimensions.bareOverallHeight = 108.96

Stacking offset with module "z": 10.959999999999994

Which is close enough!

tcreservoirtest_96_reservoir_111ul.json

I think we're in good shape. Probably going to push tomorrow so I can focus on robot stack.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome! Ya, feel free to merge in whenever, now that you tested it 🙌

Copy link
Contributor Author

@alexjoel42 alexjoel42 Jan 27, 2025

Choose a reason for hiding this comment

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

edit: The plot did not thicken. I was was measuring wrong. All is well and we can push this out. for posterity. I have exchanged moduleDefinition.dimensions.bareOverallHeight for moduleDefinition.labwareOffset.z which is more accurate.

Copy link
Collaborator

@jerader jerader left a comment

Choose a reason for hiding this comment

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

code looks correct, just want to double check that we smoke test it and that the offset is what we would expect!

Some nits:

  1. please title this PR pydantically so: fix(labware-library): labware creator adapter z offset fit or something like that
  2. you have no way of knowing this so don't worry but we like to name our branches like projectName_short-description, so for this branch, I would've called it something like ll_adapter-fix. Once you merge this in, remember to delete the branch 😄

Once you smoke test and address nit number 1, this can merge in! Nice work! 🚀

@alexjoel42 alexjoel42 changed the title Labware creator adapter fix attempt fix(labware-library): labware creator adapter z offset fit Jan 27, 2025
@alexjoel42
Copy link
Contributor Author

alexjoel42 commented Jan 27, 2025

Talked to @sanni-t and there is likely a future lift that's much more user friendly to go from the bed of the cylinders instead of the bottom of the modules due to how the Thermocycler is loaded onto the deck.

Flex now places the Thermocycler on the deck in a different way then the OT-2 so it's harder to measure from the bottom of the physical Thermocycler that's below the deck.

Magnetic Module

Stacking offset with module = Module height (to the top of the cylinder which is 38mm) + labware height - combined height from user input (z)

Thermocycler and Magnetic module files that are pulled from to get the "module height"

TC2

GEN1 Magnetic Block

Question we resolved.

moduleDefinition.dimensions.bareOverallHeight is not equal to the module height to the top of the cylinder. It's moduleDefinition.labwareOffset.z. Woo!

We're now within 0.1mm of one of our official labware and no longer negative and are getting the same adapter values. Going to test reservoirs for throughness and then call it.

@alexjoel42 alexjoel42 merged commit d05ef3f into edge Jan 27, 2025
14 checks passed
@alexjoel42 alexjoel42 deleted the LabwareCreatorAdapterFixAttempt branch January 27, 2025 22:41
caila-marashaj pushed a commit that referenced this pull request Feb 11, 2025
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