refactor(app): Use currentState for tip detection
#16904
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mjhuff
requested review from
sfoster1,
SyntaxColoring,
TamarZanzouri,
smb2268 and
a team
mjhuff
changed the title
currentState for tip detection
sfoster1
reviewed
Nov 20, 2024
mjhuff
added
the
DO NOT MERGE
sfoster1
approved these changes
Nov 20, 2024
app/src/local-resources/instruments/hooks/useTipAttachmentStatus.ts
Outdated
Show resolved
Hide resolved
SyntaxColoring
approved these changes
Nov 20, 2024
| commands: RunCommandSummary[] | ||
| ): PipetteTipState[] => { | ||
| return Object.entries(tipStates).map(([pipetteId, tipInfo]) => { | ||
| const pipetteInfo = pipetteInfoById[pipetteId] |
There was a problem hiding this comment.
Since pipetteInfoById is sourced from the run record, and pipetteId is sourced from the currentState endpoint, and those are two separate network requests, is there any danger here of this index not being present? Like if a new pipette is loaded, and the frontend receives an update to currentState before it receives an update to the run record?
There was a problem hiding this comment.
In terms of network requesting, the requests occur simultaneously, so I don't believe so.
It is possible that we determine tips are attached to a pipette, and then the user ignores the drop tip CTA, decides to remove the pipette without any sort of detach flow, then places on a different one while the drop tip CTA prompt is up, but I'm ok with the app breaking in this spot.
mjhuff
force-pushed
the
app_current-state-tip-detection
branch
from
a1d2430
to
abf9af0
Compare
mjhuff
removed
the
DO NOT MERGE
SyntaxColoring
added a commit
that referenced
this pull request
Nov 26, 2024
Conflicts: api/src/opentrons/protocol_engine/commands/absorbance_reader/read.py: Conflict in error message wording between PR #16941 in release and PR #16920 in edge. I took the one in release because the one in edge was an outdated cherry pick. app/src/organisms/Desktop/Devices/ProtocolRun/ProtocolRunHeader/RunHeaderModalContainer/hooks/useRunHeaderDropTip.ts: Just import statements. app/src/organisms/DropTipWizardFlows/hooks/useTipAttachmentStatus/getPipettesWithTipAttached.ts: Release removed `cursor: null` and `includeFixitCommands: null` from some network requests, leaving those fields unspecified instead (PR ##16893). Edge deleted this whole file in favor of a different one (PR #16904). I removed `cursor: null` from the new file; `includeFixitCommands: null` was already removed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes EXEC-1027
Overview
After completing a run or during Error Recovery, the app runs tip detection logic to determine whether or not to pop the drop tip wizard. It does so by iterating through protocol analysis, keeping track of attached pipettes, and looking at specific tip exchange commands to determine whether a not a pipette has tips attached. While this approach works, it's both brittle (any changes to tip exchange commands breaks the logic) and also a command scanning operation.
Now that we have tip status on our new
/runs/:runId/currentStateendpoint, we have a more robust option for determining tip status. There's one caveat: the robot and this util do have ideas of when a tip should be considered attached. On the app, because we want to show drop tip wizard when there might be a tip attached, such as during a failed pick up tip command, we assume there is a tip attached. In this instance however, the robot server does not think a tip is attached. Unfortunately, there's no way around manually accounting for these differences, but luckily they are few, and it's still a significant improvement over tracking every tip exchange command.There's another optimization we can make too: we don't actually need to poll
/instrumentsfor pipette data, because now, the only place we use that data is during the tip detection util. Instead, we can fetch it along with the other necessary resources whendetermineTipStatusis invoked.In short, these changes give us:
/instrumentspolling.See "Review Requests" for the big question.
Test Plan and Hands on Testing
Review requests
Risk assessment
-medium. This does change tip detection logic, but it's pretty easy to reason about now.