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

feat(api): Update robot deck extents to check the full pipette bounds #15532

Merged
merged 9 commits into from
Jul 18, 2024

Conversation

Laura-Danielle
Copy link
Contributor

Overview

Previously, we were only checking for a very specific set of bounds for the robot. Now, we should be able to check the exact "absolute" value that a pipette type can move to and throw and error if we exceed those bounds.

Questions

  1. I have one failing test, I'm not sure if this means the extents I'm using aren't 100% correct or if the older ones were overly generous
  2. Should I bump the deck version?
  3. In AddressableState there are a lot of free floating functions that access the deck definition components directly, I wasn't sure why we were doing this but if you want me to keep the same format I can.
  4. I will need assistance at some point to get my JS env up and running again as I cannot run make format

Test Plan

  • Put a bunch of protocols through an app and see whether the software correctly determines if something is out of bounds

Changelog

  • Added two new components to the deck definition for 'extents' and 'mount offsets'
  • Added accessors for that in the protocol engine
  • Modified _is_within_pipette_extents function to check the absolute bound per pipette type per mount without any offsets

Review requests

Pls check my math, I'm almost sure I did one or two things wrong

Risk assessment

Medium. If we don't get this right it could throw erroneous deck bound errors.

@Laura-Danielle Laura-Danielle requested review from a team as code owners June 26, 2024 20:58
Copy link
Contributor

A PR has been opened to address analyses snapshot changes. Please review the changes here: #15533

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.

Yay! Some questions:

Copy link
Contributor

Choose a reason for hiding this comment

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

I see we have older code that claims to do bounds checking. Where does it get its data from? Does it need to be deduplicated with this, or have # todo comments describing how to do the deduplication in the future?

Copy link
Member

Choose a reason for hiding this comment

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

Good question. Would like to know too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will check it out and try to summon my brain power to remember

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So that check is determining whether the target position is outside of the bounds of the robot but it is only a point and doesn't actually check for the pipette bounding box.

I think it's OK to leave in since it's checking a different thing.

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.

Have some inline comments about some details of this, but it has a more fundamental problem: it doesn't take mount offset or pipette offset calibration into account. This may be the cause of the test failure, but it will definitely be the cause of weird errors around interactions near the edge of the deck. I think we need to get at least some of the mount data from the hardware controller so it can fold in the calibration.

@Laura-Danielle Laura-Danielle force-pushed the EXEC-309-use-extents-for-deck-check branch from 0183e2e to 9abb7ff Compare July 17, 2024 16:53
Copy link
Contributor

@CaseyBatten CaseyBatten left a comment

Choose a reason for hiding this comment

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

Based on changes made and testing executed I believe this satisfies our need to prevent the robot from exceeding the deck extents, particularly in regards to actions involving partial tips commands. The following behavior was tested on a Flex and performed as expected:

  • Attempting Single tip pickup using Nozzle A1 of an 8ch pipette on a tiprack in D3 failed analysis
  • Attempting Single tip pickup using Nozzles A1 and A12 as well as Row A1 of a 96ch on a tiprack in D3 failed analysis
  • Attempting Single tip pickup using Nozzle H1 of a 96ch on a tiprack in A3 failed analysis
  • Attempting Single tip pickup using Nozzle H12 of a 96ch on a tiprack in A1 failed analysis
  • Attempting a Row A1 pickup on Well E of a labware in D3 passed analysis and executed correctly on the robot

With each of these cases performing as expected, I believe we can safely move on to QA testing of the deck extents. Thanks Laura!

Copy link

codecov bot commented Jul 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.06%. Comparing base (3ca7c0a) to head (b654fc5).
Report is 9 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             edge   #15532       +/-   ##
===========================================
- Coverage   92.43%   81.06%   -11.38%     
===========================================
  Files          77      118       +41     
  Lines        1283     4119     +2836     
===========================================
+ Hits         1186     3339     +2153     
- Misses         97      780      +683     
Flag Coverage Δ
g-code-testing 92.43% <ø> (ø)
shared-data 75.91% <100.00%> (?)

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

Files Coverage Δ
...ta/python/opentrons_shared_data/robot/dev_types.py 84.00% <100.00%> (ø)

... and 40 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.

This is excellent, thank you for taking the reviews into account! I'd love to see a followup that takes mount offset values from these deck defs rather than python constants but that definitely doesn't have to be part of this PR.

@Laura-Danielle Laura-Danielle merged commit 5b09f72 into edge Jul 18, 2024
43 of 44 checks passed
@Laura-Danielle Laura-Danielle deleted the EXEC-309-use-extents-for-deck-check branch July 18, 2024 14:57
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.

5 participants