Skip to content
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(app): implement stall recoveries #17002

Merged
merged 4 commits into from
Dec 2, 2024

Conversation

sfoster1
Copy link
Member

@sfoster1 sfoster1 commented Dec 2, 2024

Adds recovery from stall or collision errors to the app.

This should implement these flows: https://www.figma.com/design/OGssKRmCOvuXSqUpK2qXrV/Feature%3A-Error-Recovery-November-Release?node-id=9765-66609&t=65NkMVGZlPdCG7z6-4

When there's a stall, which can happen on pretty much any command, we should now prompt the user to home and retry. To home, they have to make sure the machine is safe, so we will go through DTWiz.

Reviews

  • did i miss anything

Testing

  • stalls should get you the home and retry button
  • you should enter the DTWiz if >0 pipettes have a tip
  • if 0 pipettes have a tip the DTwiz should be skipped (or if there are no pipettes)
  • Retrying should in fact work

Closes EXEC-725

Not all motion axes are always present on machines. For instance, if you
have just one pipette present, then you won't have a right plunger
motor. This presents a problem for the unsafe/engageAxes and
unsafe/updatePositionEstimators commands, which weren't properly
handling the case where these axes were specified when not present and
the machine was a Flex, where "not present" means "no microcontroller
there to respond". While we'd properly handle this case when a 96 was
present, or when a gripper was absent, in the single low-throughput
pipette case calling unsafe/engageAxes or
unsafe/updatePositionEstimators would time out because the right pipette
node wasn't present. This would cause drop tip wizard to fail.
Adds UI flows for halndling STALL_OR_COLLISION_DETECTED errors from Flex
robots. The user can recover the deck state, home the machine, and
continue their protocol by retrying the failed step.

Closes EXEC-725
@sfoster1 sfoster1 requested review from a team as code owners December 2, 2024 16:12
@sfoster1 sfoster1 requested review from TamarZanzouri and mjhuff and removed request for a team December 2, 2024 16:12
Copy link
Contributor

@mjhuff mjhuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good on the FE! Thanks for doing all this. Just a few clean up comments that I think we should resolve.

@@ -333,6 +357,15 @@ export const RECOVERY_MAP_METADATA: RecoveryRouteStepMetadata = {
[ROBOT_DOOR_OPEN.ROUTE]: {
[ROBOT_DOOR_OPEN.STEPS.DOOR_OPEN]: { allowDoorOpen: false },
},
[HOME_AND_RETRY.ROUTE]: {
[HOME_AND_RETRY.STEPS.PREPARE_DECK_FOR_HOME]: { allowDoorOpen: true },
[HOME_AND_RETRY.STEPS.REMOVE_TIPS_FROM_PIPETTE]: { allowDoorOpen: true },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[HOME_AND_RETRY.STEPS.REMOVE_TIPS_FROM_PIPETTE]: { allowDoorOpen: true },
[HOME_AND_RETRY.STEPS.REMOVE_TIPS_FROM_PIPETTE]: { allowDoorOpen: false },

I think we probably want to keep the door closed during drop tip wizard. I personally haven't tested it, but because DT Wiz is essentially its own API, I'm not certain it will play well with the door open and popping the door open modal.

Might be worth a discussion, but for safety reasons as well as copy considerations, maybe want to default to allowDoorOpen: false unless the step specifically says the door is open?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. I think all the other ones do actually want to allow an open door, though. We do special handling for the "click to home" button.

Comment on lines +126 to +128
tipStatusUtils.areTipsAttached
? RECOVERY_MAP.HOME_AND_RETRY.STEPS.REMOVE_TIPS_FROM_PIPETTE
: RECOVERY_MAP.HOME_AND_RETRY.STEPS.PREPARE_DECK_FOR_HOME
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is a tip check necessary here, since we're always guaranteed to handle tips in PrepareDeckForHome, which always occurs before we get to this step?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so just to preserve the illusion that this is a straight linear flow instead of something optional. If there were tips attached, the user got here by going through the DTWiz, and therefore if they hit "go back" that's where they'll expect to go back to.

Comment on lines +132 to +134
proceedToRouteAndStep(
HOME_AND_RETRY.ROUTE,
HOME_AND_RETRY.STEPS.CLOSE_DOOR_AND_HOME
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm reading this right, this is always navigating us to the the door modal, which we don't want to do per the dev note.

There are doorStatusUtils we can use to conditionally route.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that step is rendering RecoveryDoorOpenSpecial, and the body of that panel has a mount effect for the door status that will automatically route away and do the home

Comment on lines +84 to +91
if (selectedRecoveryOption === HOME_AND_RETRY.ROUTE) {
void proceedToRouteAndStep(
DROP_TIP_FLOWS.ROUTE,
DROP_TIP_FLOWS.STEPS.BEFORE_BEGINNING
)
} else {
void proceedNextStep()
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make it easier to follow, I'd prefer to keep the "to drop tip wizard" routing behavior the same across all recovery options, and this approach differs from the approach used in other recovery flows that do this tip checking.

I'm totally down to do this new routing approach, where we do a primaryOnClick here based on the recovery option, and we include an analogue to

case HOME_AND_RETRY.STEPS.REMOVE_TIPS_FROM_PIPETTE:
        return <BeginRemoval {...props} />

for each route that uses DT Wiz. Alternatively, we just get rid of the primaryOnClick and the buildContent() switch case for this route and follow the existing pattern.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -54,6 +54,8 @@ export function getErrorKind(
errorType === DEFINED_ERROR_TYPES.GRIPPER_MOVEMENT
) {
return ERROR_KINDS.GRIPPER_ERROR
} else if (errorType === 'stallOrCollision') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} else if (errorType === 'stallOrCollision') {
} else if (errorType === DEFINED_ERROR_TYPES.STALL_COLLISION) {

decoy.when(gantry_mover.motor_axis_to_hardware_axis(MotorAxis.Y)).then_return(
Axis.Y
)
gantry_mover.motor_axes_to_present_hardware_axes(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

Copy link
Contributor

@TamarZanzouri TamarZanzouri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good! nice job!

@sfoster1 sfoster1 merged commit 89834d5 into edge Dec 2, 2024
46 checks passed
@sfoster1 sfoster1 deleted the exec-725-wire-up-stall-or-collision branch December 2, 2024 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants