-
Notifications
You must be signed in to change notification settings - Fork 179
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): Addition of Evotip specific commands #17351
feat(api): Addition of Evotip specific commands #17351
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## chore_release-8.3.0 #17351 +/- ##
======================================================
Coverage ? 72.91%
======================================================
Files ? 152
Lines ? 6265
Branches ? 0
======================================================
Hits ? 4568
Misses ? 1697
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. |
A PR has been opened to address analyses snapshot changes. Please review the changes here: #17356 |
@@ -778,6 +778,7 @@ async def move_axes( | |||
position: Mapping[Axis, float], | |||
speed: Optional[float] = None, | |||
max_speeds: Optional[Dict[Axis, float]] = None, | |||
_expect_stalls: bool = False, |
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.
is this supposed to be an internal arg? maybe we don't need the _
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.
Yeah I don't think we need the _
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.
Let's tighten up some of the dispense implementation and get rid of what parameters we can, please. It does look fine in structure. I eagerly anticipate following this up after release by moving some of these exact movement commands out of the engine and into hardware.
@@ -778,6 +778,7 @@ async def move_axes( | |||
position: Mapping[Axis, float], | |||
speed: Optional[float] = None, | |||
max_speeds: Optional[Dict[Axis, float]] = None, | |||
_expect_stalls: bool = False, |
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.
Yeah I don't think we need the _
self, | ||
mount: Union[top_types.Mount, OT3Mount], | ||
home_after: bool = False, | ||
ignore_plunger: Optional[bool] = False, |
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.
this shouldn't be Optional
unless it can be none, and if it can be none we should handle the None
case. looks like just ignore_plunger: bool = False
is fine.
) | ||
|
||
if ( | ||
isinstance(current_location, CurrentWell) |
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 don't think this is right. CurrentWell
will presumably be the "well" of the tips, right? And evotip_dispense
is actually removing liquid from those. The place that it's adding liquid is the intermediate plate, which I think in general the engine won't know about, right? I think we should just not do volume handling here until we know we can do it correctly, which means removing lines 125 to 139.
) | ||
return SuccessData( | ||
public=EvotipDispenseResult(volume=result.public.volume), | ||
state_update=result.state_update.set_liquid_operated( |
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.
and removing the set liquid operated
|
||
|
||
class TipPickUpParams(BaseModel): | ||
prepDistance: float = Field( |
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.
Let's have these default to none and look up defaults from something else, so if we have to change these defaults it doesn't have to alter the command schema.
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.
Also these are only relevant to low-throughput pipettes, right? The 96 uses completely different options. I dunno about these, but I'm open to keeping them
pressDistance: float = Field( | ||
default=3.5, description="The distance to press on tips." | ||
) | ||
ejectorPushmm: float = Field( |
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.
ejectorPushmm: float = Field( | |
ejectorPushMm: float = Field( |
"""Result data from the execution of a EvotipSealPipette.""" | ||
|
||
tipVolume: float = Field( | ||
0, |
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.
do these need defaults?
tip_geometry: TipGeometry, | ||
tip_pick_up_params: TipPickUpParams, | ||
mount: MountType, | ||
press_fit: bool, |
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.
let's just have these be two functions instead of one function that completely changes behavior based on a bool argument. Also IMO we can factor this function out of the object and pass in the gantry mover, and do the state bookkeeping after the call.
) | ||
|
||
# Drive mount down for press-fit | ||
await self._gantry_mover.move_axes( |
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.
let's drop a TODO for this stuff to (a) have its constants factored out and (b) move into the hardware controller.
default_factory=DropTipWellLocation, | ||
description="Relative well location at which to drop the tip.", | ||
) | ||
homeAfter: Optional[bool] = Field( |
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.
do we need this?
tested with @CaseyBatten, gripper pickup offsets need to be corrected for move evotips, but otherwise runs as expected |
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 okay with this in broad strokes but imo we should look at the defaults for tip pickup and have them capture None
instead of data, and we should fix the drop_tips
ignore_plunger
thing.
Also I suspect that we'll want to provide a default behavior of "current labware" for unseal, but I could be convinced otherwise pretty easily
Overview
Covers EXEC-907
Introduces Evotip (or resin-tip) specific commands to the instrument context. These commands achieve the desired sealing, pressurization and unsealing steps for a resin tip protocol.
To Do:
Test Plan and Hands on Testing
Changelog
Addition of new instrument context and engine commands. Addition of necessary optional flags to hardware API fields to allow for specialty Axis movement and skipping of certain home steps in situations involving the evo tips.
Review requests
Do the approaches used for movement coordination seem appropriate under the engine commands being used?
Risk assessment
Low - New behavior that does not have cross-interaction with other commands. These are also beta commands which will not be publicly documented.