-
Notifications
You must be signed in to change notification settings - Fork 178
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(shared-data, app): add configureNozzleLayout command type in run log #13961
feat(shared-data, app): add configureNozzleLayout command type in run log #13961
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## edge #13961 +/- ##
==========================================
- Coverage 70.75% 70.73% -0.02%
==========================================
Files 2506 2507 +1
Lines 70663 70694 +31
Branches 8679 8693 +14
==========================================
+ Hits 49996 50006 +10
- Misses 18543 18560 +17
- Partials 2124 2128 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
export interface ConfigureNozzleLayoutRunTimeCommand | ||
extends CommonCommandRunTimeInfo, | ||
ConfigureNozzleLayoutCreateCommand { | ||
result?: {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add a TODO here to remind us to add a result type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was looking at the analysis output and the result was just an empty object - @sanni-t is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's correct. The result is an empty object.
interface ConfigureNozzleLayoutParams { | ||
pipetteId: string | ||
configuration_params: NozzleConfigurationParams | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wait i didn't realize a pipetteId
is needed in the params, I was looking at the implementation section on the proposal: https://opentrons.atlassian.net/wiki/spaces/RPDO/pages/3732602905?atlOrigin=eyJpIjoiMTQxNjBkYTBkYzhjNDM0YzgzMGY2YmEwMWZlOGY4NTMiLCJwIjoiY29uZmx1ZW5jZS1jaGF0cy1pbnQifQ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure it's necessary based on this https://github.com/Opentrons/opentrons/blob/edge/api/src/opentrons/protocol_engine/commands/configure_nozzle_layout.py#L33 - @sanni-t can you confirm whether the types I have here are correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, pipette_id
is required for knowing which pipette we are setting to partial nozzle configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, just pending the type questions for Sanniti. I left a comment about camelCasing a few things.
| typeof EMPTY | ||
|
||
interface NozzleConfigurationParams { | ||
primary_nozzle: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we probably want both primary_nozzle
and configuration_params
to be camelCase, like what we do for pipetteId
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would have to be a BE change, wouldn't it?
Overview
This PR adds support for the
configureNozzleLayout
command in run logTest Plan
configureNozzleLayout
command and that pickup tip commands now print a range of wells dependent on how many nozzles are being used at that timeChangelog
configureNozzleLayout
command type and supporting typesCommandText
pickUpTip
command text to use new utilgetWellRange
that will return the range of wells that a pipette is going to pick up tips from based on the number of nozzles the pipette is currently usingReview requests
Look through util and rest of code for sanity and test this out if you can!
Risk assessment
Low