feat(protocol-engine): Add tryLiquidProbe to complement liquidProbe #15667
Conversation
SyntaxColoring
changed the title
Merging for analysis snapshot test updates.
| LiquidProbeCommandType = Literal["liquidProbe"] | ||
| TryLiquidProbeCommandType = Literal["tryLiquidProbe"] |
There was a problem hiding this comment.
The names liquidProbe/tryLiquidProbe are inconsistent with verifyTipPresence/getTipPresence. I really don't know what to do about this at this point, since it seems like it'd be annoying to rename liquidProbe, but I'm open to ideas.
There was a problem hiding this comment.
Would it be annoying? I don't think anybody uses it right now. I think we could do either
VerifyLiquidPresence/GetLiquidPresence, if we want to add a distinct command for get liquid height- If we don't want to change
LiquidProbe,VerifyLiquidPresenceseems like a good standalone command name honestly. Probably would want its own file though.
shared-data/command/schemas/8.json
Outdated
There was a problem hiding this comment.
I'm updating schema 8 in-place because I guess we've been doing that. But I think officially, we're supposed to be doing new stuff in a new schema 9.
One purpose of putting new stuff in new schemas is that the server can reject JSON protocols that are too new for it, efficiently and with a helpful error message, like we do for Python protocols. But that only works if we remember to put new stuff in new schemas, and I guess we haven't remembered, oops.
I'm happy to do whatever here. I'm uncertain whether we want to keep trying the separate-schema thing or just give up on it.
There was a problem hiding this comment.
Yeah. I'm honestly not sure.
I think we hoped to put new commands or at least new patterns of command params in new schemas; so far we definitely haven't. I think it would be nice to start doing it again but I also think on some level we want to avoid having weird update races... I'm interested in @jerader 's opinion here.
|
Re: state updates - this is implemented right now as essentially stateless, if I understand what you're getting at. We're not tracking anything from this and don't really need to until phase 2. I think whether we should do it right now or not really depends on your outlook about state: is it for inputs for future commands, or is it for outside inspection? Nothing really looks at a possible liquid-verified state, so does it need to exist? |
Sorry, I was totally unclear about this. I'm thinking of the "current well" state used for path planning. These commands might leave the pipette in a new well and I think they need to update state to reflect that. |
Resolve conflicts in: * api/src/opentrons/protocol_engine/commands/liquid_probe.py * api/tests/opentrons/protocol_engine/commands/test_liquid_probe.py
This is necessary because PAPI, for convenience, maps param types to command requests and expects that mapping to be 1:1. Hoisted by my own petard. :(
Overview
This goes towards EXEC-598. See that ticket for background on the problem.
In v8.0.0, we think the only place this problem can happen is the
liquidProbecommand.InstrumentContext.detect_liquid_presence()wants to catchliquidProbeerrors and turn them intoFalsereturns.Instead of making that kind of
try/exceptpossible in general, we're solving it here specifically forliquidProbecommands by just having a secondliquidProbevariant,tryLiquidProbe, that returns aresultwithz_position=nullinstead of returning anerror. In other words, it's a variant ofliquidProbewhereliquidNotFounderrors do not enter recovery mode. This is consistent with theverifyTipPresence/getTipPresenceapproach, and it's potentially useful for JSON protocols.Test plan
Just unit tests, so far. Follow-up work will wire this up to
opentrons.protocol_apiand then we can test it through that.Changelog
Add a
tryLiquidProbecommand, as described above.To do:
Review requests
See inline notes.
Risk assessment
To do.
To do