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): Add ready-to-aspirate into engine state so analysis correctly fails #17636

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

Conversation

ryanthecoder
Copy link
Contributor

@ryanthecoder ryanthecoder commented Mar 3, 2025

Overview

The "ready to aspirate" state was previously only known to the hardware control layer, this adds the state updates to the protocol engine so that commands will fail analysis when steps would produce an error at runtime without removing any of the hardware layer checks.

This stems from a scenario where the following steps would produce a "not ready to aspirate" error at runtime but pass analysis.

  1. Pipette picks up tips
  2. Does an air gap
  3. Does an aspirate
  4. does a dispense with push out
  5. does an airgap command

Since we automatically do a prepare-to-aspirate as part of the aspirate command and the pick up tip, it successfully passes step 2 but we don't automatically do it during dispense or air gap so when the hardware attempts to start step 5, and step 4 leaves it in an "not-ready" state the hardware controller fails at runtime.

This PR updates all of the various protocol engine command that effect the "ready" state to have a new StateUpdate element that stores the current state in the state view. this way analysis will fail when a particular order of calls would result in an illegal move.

Test Plan and Hands on Testing

Changelog

Review requests

Risk assessment

@ryanthecoder ryanthecoder requested a review from a team as a code owner March 3, 2025 19:33
@ryanthecoder ryanthecoder changed the title fix(api): Add ready-to-aspirate into engine state so analaysis correctly fails fix(api): Add ready-to-aspirate into engine state so analysis correctly fails Mar 3, 2025
@ryanthecoder ryanthecoder force-pushed the RABR-741-incorrect-error-air-gap branch from 44d1571 to 8345292 Compare March 3, 2025 20:28
Copy link

codecov bot commented Mar 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 21.84%. Comparing base (2a694fe) to head (b71f9c8).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             edge   #17636   +/-   ##
=======================================
  Coverage   21.84%   21.84%           
=======================================
  Files        2822     2822           
  Lines      217431   217431           
  Branches     8214     8214           
=======================================
  Hits        47491    47491           
  Misses     169940   169940           
Flag Coverage Δ
protocol-designer 18.91% <ø> (ø)
step-generation 4.36% <ø> (-0.01%) ⬇️

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

🚀 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.

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.

Some inline comments, but also a couple larger things:

  • The state updates that go along with stall or overpressure defined errors in the commands that move the plunger should also mark not-ready-to-aspirate
  • home should mark a pipette as ready to aspirate if that pipette's axis is homed

Thanks for doing this!

@ryanthecoder ryanthecoder force-pushed the RABR-741-incorrect-error-air-gap branch from 5ce719c to ff63f28 Compare March 10, 2025 20:41
@ryanthecoder ryanthecoder force-pushed the RABR-741-incorrect-error-air-gap branch from 78c8217 to b71f9c8 Compare March 11, 2025 15:02
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.

Very nice, looks good to me!

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