-
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
refactor(app): Use currentState
for tip detection
#16904
base: chore_release-8.2.0
Are you sure you want to change the base?
Conversation
currentState
for tip detection
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 do not think we want this in 8.2 unless we have to create another alpha for other reasons, and maybe not even then if it increases the test surface for the alpha.
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.
Looks great and much more stable! I think that should be in resources
though.
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.
nit: this depends on the robot and uses api-client methods. IMO this should be in resources
, not local-resources
, which is only for stuff that doesn't hit the robot at all
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.
Ahhhh yep, thanks!
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.
TY!
commands: RunCommandSummary[] | ||
): PipetteTipState[] => { | ||
return Object.entries(tipStates).map(([pipetteId, tipInfo]) => { | ||
const pipetteInfo = pipetteInfoById[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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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/currentState
endpoint, 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
/instruments
for 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 whendetermineTipStatus
is invoked.In short, these changes give us:
/instruments
polling.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.