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): update flex fixed-trash for dvt #12532

Merged
merged 5 commits into from
Apr 24, 2023
Merged

Conversation

sfoster1
Copy link
Member

This fixed trash bin has changed shapes.

Review requests

  • is it, like, ok that the position is negative? The depth really does extend below the deck

testing

  • should drop stuff a lot closer to the trash now

@sfoster1 sfoster1 requested review from a team and ChrisYarka April 20, 2023 19:38
@codecov
Copy link

codecov bot commented Apr 20, 2023

Codecov Report

Merging #12532 (207505c) into edge (b8d4033) will increase coverage by 0.30%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #12532      +/-   ##
==========================================
+ Coverage   73.30%   73.61%   +0.30%     
==========================================
  Files        1506     2266     +760     
  Lines       49351    62356   +13005     
  Branches     2997     6597    +3600     
==========================================
+ Hits        36177    45901    +9724     
- Misses      12718    14886    +2168     
- Partials      456     1569    +1113     
Flag Coverage Δ
app 72.28% <ø> (+25.43%) ⬆️
labware-library 49.74% <ø> (ø)
protocol-designer 46.34% <ø> (ø)
shared-data 75.35% <ø> (ø)
step-generation 88.20% <ø> (ø)

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

see 760 files with indirect coverage changes

@@ -27,10 +27,10 @@
"yDimension": 165.67,
"xDimension": 107.11,
"totalLiquidVolume": 3200000,
"depth": 187.5,
"depth": 172.5,
Copy link
Member

Choose a reason for hiding this comment

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

The trash z dimension is 15mm more than the depth, so I'm assuming that's the thickness of the trash bottom material. Is that correct?

"x": 105.375,
"y": 39.875,
"z": 0
"z": -132.5
Copy link
Member

Choose a reason for hiding this comment

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

So this means the trash goes 132.5mm below the deck, ya? But the cornerOffsetFromSlot has a z offset of -94.. I think these two should be the same so not sure which one needs to be corrected.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh I think I need to update the corner offset then

@SyntaxColoring
Copy link
Contributor

SyntaxColoring commented Apr 21, 2023

is it, like, ok that the position is negative? The depth really does extend below the deck

We should keep an eye out for motion planning accidentally clamping z-coordinates to 0.

We should also probably figure out a number for the lowest possible z that a pipette can move to, the same way we already have that for the highest possible z.

@sfoster1
Copy link
Member Author

The negative positions fail protocols in simulation unfortunately. I'll rework this to just have the "above-ground" geometry.
Screenshot 2023-04-24 at 12 14 05 PM

@SyntaxColoring
Copy link
Contributor

SyntaxColoring commented Apr 24, 2023

@sfoster1 The negative positions fail protocols in simulation unfortunately.

It looks like that's stemming from the fact that the well z is restricted to being a positiveNumber:

"description": "z location of center-bottom of well in reference to left-front-bottom of labware",
"$ref": "#/definitions/positiveNumber"

Which makes sense.

@sanni-t So this means the trash goes 132.5mm below the deck, ya? But the cornerOffsetFromSlot has a z offset of -94.. I think these two should be the same so not sure which one needs to be corrected.

Reading the schema docs more closely, I think cornerOffsetFromSlot's z and the well's z should not be the same. My interpretation is:

  • cornerOffsetFromSlot should have a very negative z. This reflects the fact that the bottom surface of the labware is below the surface of the slot.
  • The well z should be a smallish positive number, to represent the thickness of the bin's plastic floor.

In other words, I think the well's z is already relative to the cornerOffsetFromSlot, so it doesn't make sense for it to be negative.

@sanni-t
Copy link
Member

sanni-t commented Apr 24, 2023

In other words, I think the well's z is already relative to the cornerOffsetFromSlot, so it doesn't make sense for it to be negative.

Good catch! This is correct. Verified by calculating position of top of the trash well the way engine calculates it and it works correctly for cornerOffsetFromSlot of (x1, y1, -134.5), well depth of 172.5 and well z of 2.5 (thickness of the trash bottom).

@sfoster1
Copy link
Member Author

That makes sense, thank you! I think though that we need changes like you mentioned @SyntaxColoring about minimum z positions before we allow a negative position that large, just in case someone tries to go to .bottom() or .center() or whatever. I think we should merge this with above-deck-geometry only for now.

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.

Ideally I would like to get the true measurements for the trash bin in there.

That said, I don’t think we have any code in place that will prevent the pipette from trying to go to fixed_trash.bottom() which would be a problem so I can see the pros of using the above-deck-only approach.

But I hope we do follow-up with true measurements + restricting the pipette from trying to reach the bin bottom (either by clipping the pipette travel distance to max allowed or by having the API raise a user-friendly error instead of relying on hardware control's axis limit errors) because you can still specify something like well.bottom(-50) with the current above-deck-only measurements and run into an error.

@sfoster1
Copy link
Member Author

Definitely agree. And we want sub-deck stuff for calibration structures.

@sfoster1 sfoster1 merged commit 4dea652 into edge Apr 24, 2023
@sfoster1 sfoster1 deleted the fix-trash-size branch April 24, 2023 18:46
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