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(app, shared-data, step-generation): frontend low volume support #13526

Merged
merged 13 commits into from
Sep 13, 2023

Conversation

jerader
Copy link
Collaborator

@jerader jerader commented Sep 12, 2023

closes RAUT-714

Overview

This adds low volume front-end support

Test Plan

in PD/Step-generation:

sandbox: https://sandbox.designer.opentrons.com/app_pd-low-volume-support/

  • create a Flex protocol with a p50 pipette (single or 8-channel) and a p1000 flex pipette (single or 8-channel).
  • Add 2x transfer steps (1 for each pipette)
  • save and download the protocol and open up the json file. see that configureForVolume command occurs before the aspirate with the p50 pipette but not before the aspirate with the p1000 pipette.
  • create a new Flex protocol with a p50 pipette (single or 8-channel).
  • this time add a mix step with mixing 1uL
  • download the protocol, in the json file you should see only 1 configureForVolume command happening before the mix's aspirate and dispense commands

in the App:

  • upload the protocol to the app and should pass protocol analysis. Check the run log command text for the configureForVolume commands.

Changelog

in shared-data:

  • add the types for ConfigureForVolumeCreateCommand and ConfigureForVolumeRunTimeCommand, add the optional pushOut param to dispense command

in app:

  • add logic in commandText and add test cases

in PD:

  • there are no visual changes to PD!

in Step-generation:

  • create new configureForVolume.ts atomic command, add test for it
  • add logic for emitting the command in transfer.ts before an aspirate happens and volume is 1uL and with the correct pipettes.
  • add logic for emitting the command in mix.ts to happen before the set of mix aspirate and dispense commands.
  • add a comment explaining that pushOut won't be specified in dispense.ts

Review requests

see test plan

Risk assessment

low

@jerader jerader marked this pull request as ready for review September 12, 2023 17:02
@jerader jerader requested review from a team as code owners September 12, 2023 17:02
@jerader jerader requested review from brenthagen and removed request for a team September 12, 2023 17:02
@codecov
Copy link

codecov bot commented Sep 12, 2023

Codecov Report

Merging #13526 (103b957) into low-volume-pipetting (c1312e3) will increase coverage by 0.08%.
Report is 2 commits behind head on low-volume-pipetting.
The diff coverage is 58.82%.

Additional details and impacted files

Impacted file tree graph

@@                   Coverage Diff                    @@
##           low-volume-pipetting   #13526      +/-   ##
========================================================
+ Coverage                 71.29%   71.38%   +0.08%     
========================================================
  Files                      2382     2434      +52     
  Lines                     66907    68076    +1169     
  Branches                   7772     7926     +154     
========================================================
+ Hits                      47703    48596     +893     
- Misses                    17364    17615     +251     
- Partials                   1840     1865      +25     
Flag Coverage Δ
app 69.05% <85.71%> (+0.14%) ⬆️
hardware-testing ∅ <ø> (∅)
protocol-designer 46.09% <ø> (ø)
shared-data 74.30% <ø> (+0.45%) ⬆️
step-generation 86.82% <40.00%> (-0.36%) ⬇️

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

Files Changed Coverage Δ
...-generation/src/commandCreators/atomic/dispense.ts 86.48% <ø> (ø)
...ration/src/commandCreators/compound/consolidate.ts 94.73% <0.00%> (-2.57%) ⬇️
...eration/src/commandCreators/compound/distribute.ts 92.00% <0.00%> (-1.88%) ⬇️
...tep-generation/src/commandCreators/compound/mix.ts 95.45% <0.00%> (-4.55%) ⬇️
...eneration/src/commandCreators/compound/transfer.ts 97.46% <0.00%> (-1.25%) ⬇️
...neration/src/getNextRobotStateAndWarnings/index.ts 71.83% <ø> (ø)
...n/src/commandCreators/atomic/configureForVolume.ts 66.66% <66.66%> (ø)
app/src/organisms/CommandText/index.tsx 84.09% <80.00%> (-0.53%) ⬇️
...src/organisms/CommandText/PipettingCommandText.tsx 76.47% <100.00%> (ø)

... and 101 files with indirect coverage changes

@@ -163,6 +165,20 @@ export const transfer: CommandCreator<TransferArgs> = (
changeTipNow = isInitialSubtransfer || destWell !== prevDestWell
}

const configureForVolumeCommand: CurriedCommandCreator[] =
Copy link
Collaborator Author

@jerader jerader Sep 12, 2023

Choose a reason for hiding this comment

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

note: this command emits every time for a p50 flex pipette in the beginning of a transfer. So if you were to aspirate/dispense multiple times in the same transfer (at the same volume, since you're not allowed to change volumes in the middle of a transfer in PD), this command will only happen once at the beginning. But if you were to add a new transfer right afterwards with the same volume, the command would emit again. That is where i'm not too sure how to refactor to not emit it again 🤔 , would need to do so in a follow up PR

@sfoster1 sfoster1 force-pushed the low-volume-pipetting branch from 1f47e0c to 6ead3d6 Compare September 12, 2023 17:22
@sfoster1 sfoster1 requested a review from a team as a code owner September 12, 2023 17:22
@jerader jerader changed the title feat(app, shared-data, protocol-designer, step-generation): frontend low volume support feat(app, shared-data, step-generation): frontend low volume support Sep 12, 2023
@sfoster1
Copy link
Member

App command render looks good!

Copy link
Contributor

@mjhuff mjhuff left a comment

Choose a reason for hiding this comment

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

It's hard for me to tell if conceptually these changes are correct (since I'm still learning PD), but I tested per your instructions and everything works. The JS lgtm as well! 🚀

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.

Stuff from initial testing

  • ✅ app rendering of configure looks good
    • ❓ though it says "Configuring 1 ul for aspiration with " and maybe it should say "Configuring pipette to aspirate 1ul"
  • ❌ PD UI for push out needs to change; we should either not
    • not have push out UI at all (my pref) because specifying it is complex (see below) and not emit the arg in step-generation so it uses default settings or
    • have push out be a check box that defaults to checked, such that when it's checked step-generation doesn't emit the push out arg and when it's not checked step-generation emits it with a 0 value
    • have push out be the same "default or set value" ui field that we have elsewhere where defaulted is null/not present in s-g, and a real value is set to that value. note that here it can only be shown on full dispenses and the limits for the value have to be computed from the liquid settings as (bottom-blow_out)*plunger_surface_area
  • haven't tested the actual generated protocol yet though

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.

configureForVolume needs to be emitted more frequently. right now it's emitted only before 1ul aspirates, which means the pipette could never switch out of the low volume mode. we need to, on flex 50ul pipettes, emit before each aspirate that starts with the plunger empty, with the total volume we'll aspirate until the next time the plunger is empty

@jerader jerader requested a review from sfoster1 September 13, 2023 13:42
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.

  • made a protocol in PD branch with 50ul 1 and 8 channels
    • with a variety of transfer volumes, always saw configure for volume
    • protocol analyzed successfully in app branch
    • ODD and app runlog displayed configures successfully
    • configures executed properly traced through to api logs
  • made protocol with mix in PD branch
    • saw configure before mix multistep
  • made protocol with 1000ul pipettes and saw no configure for volumes

Looks good to me in my dev testing!

@jerader jerader merged commit 37f06a9 into low-volume-pipetting Sep 13, 2023
44 of 45 checks passed
@jerader jerader deleted the app_pd-low-volume-support branch September 13, 2023 15:21
sfoster1 pushed a commit that referenced this pull request Sep 15, 2023
…13526)

Front-end support for flex low volume

closes RAUT-714
---------

Co-authored-by: Andy Sigler <[email protected]>
sfoster1 pushed a commit that referenced this pull request Sep 15, 2023
…13526)

Front-end support for flex low volume

closes RAUT-714
---------

Co-authored-by: Andy Sigler <[email protected]>
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