feat(engine): thermocycler run profile #10921
Conversation
thermocycler run profile
Codecov Report
@@ Coverage Diff @@
## edge #10921 +/- ##
=======================================
Coverage 73.78% 73.78%
=======================================
Files 2076 2076
Lines 57327 57328 +1
Branches 5727 5727
=======================================
+ Hits 42299 42300 +1
Misses 13789 13789
Partials 1239 1239
Flags with carried forward coverage won't be shown. Click here to find out more.
|
jbleon95
added
the
robot-svcs
api/src/opentrons/protocol_engine/commands/thermocycler/run_profile.py
Outdated
Show resolved
Hide resolved
| ..., | ||
| description="Array of profile steps with target temperature and temperature hold time.", | ||
| ) | ||
| blockMaxVolumeUl: float = Field( |
There was a problem hiding this comment.
@shlokamin or @sanni-t, do either of y'all know why this is a required parameter of runProfile but optional for setTargetBlockTemperature? My gut reaction is that it should be optional in runProfile, too.
It's optional once you hit the hardware control API, FWIW
There was a problem hiding this comment.
AFAIK it should be optional, not sure why it's required in the JSON schema. im fine with updating the schema to make it optional as part of this PR (i can do that as part of the frontend side). that cool with you @sanni-t ?
There was a problem hiding this comment.
also note to self, i need to add to the JSON schema migration to make sure <v6 protocols get these parameters updated
There was a problem hiding this comment.
Yep, volume should be optional. It defaults to 25uL in firmware
There was a problem hiding this comment.
Unless we want to make sure that PD protocols always specify the volume for ideal heating. I'm guessing that since PD does volume tracking, the volume will always be specified anyway?
The volume parameter was added to setTargetBlockTemperature only in v6. Hence that one definitely needs to stay optional to allow older protocols to run as expected.
There was a problem hiding this comment.
good point, the thermocycler step form in PD does require volume, but thats a PD concern (not a step generation/schema concern), so ill go ahead and mark it as optional
| { | ||
| "temperature": target_temperature, | ||
| "hold_time_seconds": profile_step.holdSeconds, | ||
| } |
There was a problem hiding this comment.
I would love for the hardware control API to accept a dataclass or NamedTuple rather than a dict. Might add it to the confluence doc
| @@ -843,7 +843,7 @@ | |||
| "commandType": { "enum": ["thermocycler/runProfile"] }, | |||
| "params": { | |||
| "type": "object", | |||
| "required": ["moduleId", "profile", "volume"], | |||
There was a problem hiding this comment.
removing volume (renamed to blockMaxVolumeUl) as a required param
shlokamin
dismissed
mcous’s stale review
dismissing and re-requesting review after changes to the JSON schema
…-thermocycler-run-profile
…-thermocycler-run-profile
Overview
Closes #9559
Adds thermocycler
runProfilecommand to Protocol Engine.Changelog
Review requests
Risk assessment
Low