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,shared-data): Correct addressable area positions #14100

Merged

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented Dec 5, 2023

Overview

This closes RSS-408.

Test Plan

  • 8-Channel drops tips in a waste chute in the right place
  • 1-Channel drops tips in a waste chute in the right place
  • 96-Channel drops tips in a waste chute in the right place
  • Gripper drops labware in the waste chute in the right place
  • All pipettes drop tips in movable trash in the right place
  • The gripper and all pipettes safely clear the rim of the waste chute

I've used these protocols to test: Addressable area tests.zip

I only have an old waste chute to test with, so I couldn't directly test the y-position for 1- and 8-channel tip drops. (I cannot use the new waste chute cover.) So I held up the cover while the protocol was paused and eyeballed it.

Changelog

  • Update the positions of all waste chute addressable areas to match the new hardware, which has a lower cover with a different hole position.
  • ⚠️ Breaking: Correct the positioning of the 1-channel pipette by splitting the 1and8ChannelWasteChute addressable area into 1ChannelWasteChute and 8ChannelWasteChute.
  • Fix a Python Protocol API bug that had the 8-channel and 96-channel positions swapped with each other.
  • Refactor the deck definition to remove some redundant, unused, and confusing fields.
    • Delete dropTipsOffset from the addressable area definitions. This was unused.
    • Delete dropLabwareOffset from the addressable area definitions. This was used by Protocol Engine.
    • To replace both of those, move the addressable area offsetFromCutoutFixtures and boundingBoxes so the bounding box's top-center is at the expected interaction point.

Review requests

None in particular.

Risk assessment

Medium. This affects motion planning, so we need to manually test a bunch of pipette positions and make sure there are no collisions.

Copy link

codecov bot commented Dec 5, 2023

Codecov Report

Merging #14100 (dfbd88c) into chore_release-7.1.0 (134f0b2) will decrease coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@                   Coverage Diff                   @@
##           chore_release-7.1.0   #14100      +/-   ##
=======================================================
- Coverage                70.46%   70.46%   -0.01%     
=======================================================
  Files                     2512     2512              
  Lines                    71185    71183       -2     
  Branches                  8957     8957              
=======================================================
- Hits                     50158    50156       -2     
  Misses                   18836    18836              
  Partials                  2191     2191              
Flag Coverage Δ
app 67.63% <ø> (ø)
g-code-testing 96.44% <ø> (ø)
notify-server 89.13% <ø> (ø)
protocol-designer 44.99% <ø> (ø)
shared-data 75.01% <ø> (-0.02%) ⬇️
step-generation 86.84% <ø> (ø)

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

Files Coverage Δ
...pi/src/opentrons/protocol_engine/state/geometry.py 100.00% <ø> (ø)
api/src/opentrons/protocol_engine/types.py 97.50% <ø> (ø)
...ata/python/opentrons_shared_data/deck/dev_types.py 100.00% <ø> (ø)

…tions

Resolve conflict in api/src/opentrons/protocol_engine/types.py.
There are additional JavaScript things, which are being addressed in a different PR.
…tions

Resolve a conflict in api/tests/opentrons/protocol_engine/state/test_addressable_area_store.py.
@SyntaxColoring SyntaxColoring marked this pull request as ready for review December 6, 2023 21:27
@SyntaxColoring SyntaxColoring requested review from a team as code owners December 6, 2023 21:27
@SyntaxColoring SyntaxColoring requested review from mjhuff, a team and jerader and removed request for a team and mjhuff December 6, 2023 21:27
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.

thank you! This makes sense to me. I tested uploading a JSON file where i manually changed the 1and8ChannelWasteChute addressable area name to 1ChannelWasteChute and it passed analysis 🥳

Copy link
Member

@sanni-t sanni-t left a comment

Choose a reason for hiding this comment

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

Reviewed in person. I'll go through the calculations thoroughly for my understanding later but the logic makes sense and looks good to merge!

I don't understand how to interpret the dimensions in the old fixed trash labware, and I'm scared of messing with the drop-tip point, so I guess let's make these 0-sized points in the same place as the old dropTipsOffsets.

This means the fixedTrash and shortFixedTrash addressable areas will no longer have the same origin as the v3 defs' slot 12. That's fine and actually makes sense. Those addressable areas are supposed to be equivalent to a v3 well position, not a v3 deck slot position.
…tions

Resolve a conflict in api/src/opentrons/protocol_api/core/engine/instrument.py.
@SyntaxColoring SyntaxColoring merged commit 2659373 into chore_release-7.1.0 Dec 6, 2023
59 checks passed
@SyntaxColoring SyntaxColoring deleted the correct_addressable_area_positions branch December 6, 2023 23:47
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