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): Zero out cornerOffsetFromSlot in opentrons_tough_pcr_auto_sealing_lid #17723

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

Conversation

SyntaxColoring
Copy link
Contributor

Overview

Currently, opentrons_tough_pcr_auto_sealing_lid has a cornerOffsetFromSlot of (0, 0, -0.71). So, hypothetically, if you placed it in a deck slot, its bottom would be 0.71 mm "underground," below the deck. This seems wrong.

It seems like this was part of a snafu related to gripper offsets. Investigation is ongoing in EXEC-1268, which you should read for some background. But it seems clear at this point that cornerOffsetFromSlot, at least, ought to be zero.

Test Plan and Hands on Testing

  • Robot behavior is unchanged.
    • As described in EXEC-1268, the robot always behaved as if this offset was (0, 0, 0) because of an unrelated bug, EXEC-1285.

Changelog

  • Zero out cornerOffsetFromSlot in the as-yet-unreleased v2 of this definition.

  • Zero out cornerOffsetFromSlot in v1 of this definition, which has already been released.

    This is "bad"; we should usually not change definitions retroactively like this. I think it's justified in this case because someday we will fix EXEC-1285 (intentionally or not), so if we leave the offset in place, we're setting ourselves up for a change in behavior in the future. Better to nip it in the bud now, when this feature is still new.

    Also, @CaseyBatten changed the definition in fix(api, shared-data): Pick up tc lid directly off deck slot fix #17683, and if he can do it then so can I. :P

    But this isn't a hill I'll die on. I'm happy to keep the offset in the v1 definition if we accept the risk of it making more of a mess in the future.

Review requests

  • OK with adjusting the already-released v1 of this definition?
  • Any more insight to add to the investigation in EXEC-1268?

Risk assessment

Low.

@SyntaxColoring SyntaxColoring requested review from CaseyBatten and a team March 11, 2025 18:31
@SyntaxColoring SyntaxColoring requested a review from a team as a code owner March 11, 2025 18:31
Copy link

codecov bot commented Mar 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 21.73%. Comparing base (2a694fe) to head (13e2506).
Report is 40 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #17723      +/-   ##
==========================================
- Coverage   21.84%   21.73%   -0.11%     
==========================================
  Files        2822     2838      +16     
  Lines      217431   219923    +2492     
  Branches     8214     8958     +744     
==========================================
+ Hits        47491    47796     +305     
- Misses     169940   172127    +2187     
Flag Coverage Δ
protocol-designer 18.94% <ø> (+0.02%) ⬆️
step-generation 4.38% <ø> (+0.01%) ⬆️

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

see 72 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@SyntaxColoring SyntaxColoring added the gen-analyses-snapshot-pr Generate a healing PR if the analyses snapshot test fails label Mar 11, 2025
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gen-analyses-snapshot-pr Generate a healing PR if the analyses snapshot test fails
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant