feat(api, robot-server, app): Handle the Plate Reader lid while held by the gripper when the Estop is pressed. #16574

vegano1 · 2024-10-22T23:17:29Z

Overview

We don't want customers manually handling the Plate Reader lid as this can cause damage to the device when you press the estop while the gripper is holding the plate reader lid it will eventually home the gantry and hold onto the lid. When a new protocol is started the gripper jaws will open, causing the plate reader lid to fall and potentially get damaged. This pull request makes it so we automatically place the plate reader lid in its dock (staging area) when the robot resumes operating after physically and logically disengaging the estop.

Closes: PLAT-451

Test Plan and Hands on Testing

Make sure we can run a plate reader protocol without issues
Make sure that the plate reader open/close_lid commands still work
Make sure the plate reader lid is returned to its dock (staging area) after the stop is physically and logically disengaged.
Make sure the run is canceled when the estop is pressed
Make sure the estop modal is shown while placing the plate reader lid back in its dock
Make sure that we only call placeLabware when the estop is pressed
Make sure all the above works on the ODD and Desktop app.
Make sure the plate reader does not get disconnected when the e-stop is pressed
Make sure the plate reader lid can still be moved when loading the module from the protocol setup.

from typing import cast
from opentrons import protocol_api
from opentrons.protocol_api.module_contexts import AbsorbanceReaderContext

# metadata
metadata = {
    'protocolName': 'Absorbance Reader Multi read test',
    'author': 'Platform Expansion',
}

requirements = {
    "robotType": "Flex",
    "apiLevel": "2.21",
}

# protocol run function
def run(protocol: protocol_api.ProtocolContext):
    mod = cast(AbsorbanceReaderContext, protocol.load_module("absorbanceReaderV1", "C3"))
    plate = protocol.load_labware("nest_96_wellplate_200ul_flat", "C2")
    tiprack_1000 = protocol.load_labware(load_name='opentrons_flex_96_tiprack_1000ul', location="B2")
    trash_labware = protocol.load_trash_bin("B3")
    instrument = protocol.load_instrument("flex_8channel_1000", "right", tip_racks=[tiprack_1000])
    instrument.trash_container = trash_labware

    # pick up tip and perform action
    instrument.pick_up_tip(tiprack_1000.wells_by_name()['A1'])
    instrument.aspirate(100, plate.wells_by_name()['A1'])
    instrument.dispense(100, plate.wells_by_name()['B1'])
    instrument.return_tip()
   
    # Initialize to a single wavelength with reference wavelength
    # Issue: Make sure there is no labware here or youll get an error
    mod.close_lid()
    mod.initialize('single', [600], 450)

    # NOTE: CANNOT INITIALIZE WITH THE LID OPEN

    # Remove the Plate Reader lid using the Gripper.
    mod.open_lid()
    protocol.move_labware(plate, mod, use_gripper=True)
    mod.close_lid()

    # Take a reading and show the resulting absorbance values.
    # Issue: cant read before you initialize or you an get an error
    result = mod.read()
    msg = f"single: {result}"
    protocol.comment(msg=msg)
    protocol.pause(msg=msg)

    # Initialize to multiple wavelengths
    protocol.pause(msg="Perform Multi Read")
    mod.open_lid()
    protocol.move_labware(plate, "C2", use_gripper=True)
    mod.close_lid()
    mod.initialize('multi', [450, 570, 600])

    # Open the lid and move the labware into the reader
    mod.open_lid()
    protocol.move_labware(plate, mod, use_gripper=True)

    # pick up tip and perform action on labware inside plate reader
    instrument.pick_up_tip(tiprack_1000.wells_by_name()['A1'])
    instrument.aspirate(100, plate.wells_by_name()['A1'])
    instrument.dispense(100, plate.wells_by_name()['B1'])
    instrument.return_tip()

    mod.close_lid()

    # Take reading
    result = mod.read()
    msg = f"multi: {result}"
    protocol.comment(msg=msg)
    protocol.pause(msg=msg)

    # Place the Plate Reader lid back on using the Gripper.
    mod.open_lid()
    protocol.move_labware(plate, "C2", use_gripper=True)
    mod.close_lid()

Changelog

Add a new unsafe/placeLabware command to PE to place labware the gripper is already holding on to.
Add a new placeLabwareState to the runs/{run_id}/currentState endpoint to inform the client of the plate reader lid.
Add a new usePlacePlateReaderLid hook on the client to conditionally call the unsafe/placeLabware command when the placeLabwareState returns the appropriate values.
Fix an issue with the plate reader disconnecting when the estop was pressed
Fix an issue where the plate reader lid was not loaded if the module was added during module setup.

Review requests

My react is rusty, please ensure I'm not doing something dumb.
Is there anything else we should be checking in the unsafe/placeLabware command?

Risk assessment

med, this changes post-estop behavior.

…er is holding somewhere on the deck.

…rrentState endpoint.

…al to handle plate reader lid.

mjhuff

Nice first go at one of the more complicated parts of React/JS!

See comments. I think the serious things are:

We're always creating a maintenance run when the estop modal is popped.
We can use existing utils to do most of the heavy lifting in usePlacePlateReaderLid.
We're always closing the estop modal while plate reader commands may be running.

app/src/App/OnDeviceDisplayApp.tsx

app/src/organisms/EmergencyStop/EstopPressedModal.tsx

app/src/resources/modules/hooks/usePlacePlateReaderLid.ts

app/src/organisms/EmergencyStop/EstopPressedModal.tsx

app/src/organisms/EmergencyStop/EstopTakeover.tsx

app/src/organisms/EmergencyStop/EstopPressedModal.tsx

app/src/organisms/EmergencyStop/EstopTakeover.tsx

…entState

…show the remote-controlled takeover modal when we are automatically moving the plate reader lid

mjhuff

Nice job! I do like the e-stop control flow much more now.

(I only reviewed the app side of things).

mjhuff · 2024-10-24T17:25:14Z

app/src/organisms/EmergencyStop/EstopPressedModal.tsx

+          closeModal={closeModal}
+          isWaitingForResumeOperation={isWaitingForResumeOperation}
+          setIsWaitingForResumeOperation={setIsWaitingForResumeOperation}


Two separate setters responsible for the same state seems strange to me, but I get that this code was here already, and I don't think it's worth addressing now.

codecov · 2024-10-24T20:58:58Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.02%. Comparing base (9797d74) to head (ddc24cb).
Report is 75 commits behind head on edge.

❗ There is a different number of reports uploaded between BASE (9797d74) and HEAD (ddc24cb). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (9797d74) HEAD (ddc24cb)

shared-data 3 2

Additional details and impacted files

@@            Coverage Diff             @@
##             edge   #16574      +/-   ##
==========================================
- Coverage   79.88%   74.02%   -5.87%     
==========================================
  Files         120       43      -77     
  Lines        4410     3176    -1234     
==========================================
- Hits         3523     2351    -1172     
+ Misses        887      825      -62

Flag	Coverage Δ
g-code-testing	`?`
shared-data	`74.02% <ø> (-0.72%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 80 files with indirect coverage changes

CaseyBatten

Engine side logic looks good, nice work on this. A non-blocking question down below regarding the fallback case for how some of our geometry is calculated/resolved in this implementation that may effect future labware recovery behavior, but otherwise this looks great!

CaseyBatten · 2024-10-28T13:49:08Z

api/src/opentrons/protocol_api/core/engine/protocol.py

            )
+            if substate.lid_id is None:


This looks good, should provide coverage for the current run setup bug.

CaseyBatten · 2024-10-28T14:34:59Z

api/src/opentrons/protocol_engine/commands/unsafe/unsafe_place_labware.py

+        final_offsets = self._state_view.labware.get_labware_gripper_offsets(
+            labware_id, None
+        )
+        drop_offset = cast(Point, final_offsets.dropOffset) if final_offsets else None


Everything in the UnsafePlaceLabware command logic looks good, but just want to make sure I understand. When we're doing this None case here we are basically just lowering and releasing the labware in place + its movement center? So if we were between two slots, would we end up releasing the labware in a awkward deck space? Not really too much of an issue for this PR since the primary use case for now is plate reader lids which will always have resolved locations with final offsets.

No, since these are just the dropoff offsets, in that case, we would navigate to the slot and drop off the labware +- some hardware stack-up margin. We should never be dropping labware in between 2 slots for example, unless the definition for that is way off.

ahiuchingau

Nice!! Just got a small question

ahiuchingau · 2024-10-28T18:45:10Z

api/src/opentrons/hardware_control/modules/absorbance_reader.py

-        await self._poller.stop()
-        await self._driver.disconnect()
+        """Deactivate the module."""
+        pass


What does it mean when we deactivate the module now?

In the context of modules, the deactivate function means we stop whatever target action we are doing, such as heating, shaking, etc. We don't have any actions other than reading for the plate reader, but these happen relatively quickly, so we don't have to worry about them. The problem here was that I thought deactivate meant stop and disconnect the module, which is not correct as deactivate gets called when the estop is pressed. In the case of the plate reader it would disconnect and reconnect when the estop was pressed. This change fixes that behavior by removing the stop and disconnect to the cleanup method.

SyntaxColoring

Some drive-by comments. No objections to any of this merging as-is if you need to get it in for the v8.2.0 feature freeze.

api/src/opentrons/protocol_engine/commands/unsafe/unsafe_place_labware.py

SyntaxColoring · 2024-10-29T16:07:03Z

api/src/opentrons/protocol_engine/commands/unsafe/unsafe_place_labware.py

+    async def execute(
+        self, params: UnsafePlaceLabwareParams
+    ) -> SuccessData[UnsafePlaceLabwareResult, None]:
+        """Place Labware."""


Having not really been involved in this, I would find it helpful if one of these docstrings described when it's appropriate and legal for a client to use this unsafe command, and when it's not.

i.e., why is this called unsafe/placeLabware, and not just placeLabware.

I would also consider that the unfamiliar audience is probably comparing this with moveLabware and loadLabware, so we should describe how it's distinct from those.

SyntaxColoring · 2024-10-29T16:09:22Z

robot-server/robot_server/runs/run_models.py

 class RunCurrentState(BaseModel):
    """Current details about a run."""

+    estopEngaged: bool = Field(..., description="")


Nit: If we don't have a description to offer, let's just leave description unset instead of doing description="".

i.e. estopEngaged: bool instead of estopEngaged: bool = Field(..., description="").

SyntaxColoring · 2024-10-29T16:14:06Z

robot-server/robot_server/runs/run_models.py

+
+    labwareId: str = Field(..., description="The ID of the labware to place.")
+    location: OnDeckLabwareLocation = Field(
+        ..., description="The location the Plate Reade lid should be in."


Suggested change

..., description="The location the Plate Reade lid should be in."

..., description="The location the Plate Reader lid should be in."

SyntaxColoring · 2024-10-29T16:15:21Z

robot-server/robot_server/runs/run_models.py

+class PlaceLabwareState(BaseModel):
+    """Details the labware being placed by the gripper."""
+
+    labwareId: str = Field(..., description="The ID of the labware to place.")
+    location: OnDeckLabwareLocation = Field(
+        ..., description="The location the Plate Reade lid should be in."
+    )
+    shouldPlaceDown: bool = Field(
+        ..., description="Whether the gripper should place down the labware."
+    )
+
+


placeLabwareState.location is defined here in terms of the plate reader lid, specifically, as opposed to labware locations in general. So, should we either:

Rename placeLabwareState to something that reflects that it's Plate-Reader–specific, and is not meant to be used for labware in general

Or, redefine placeLabwareState.location to not be specifically about the plate reader lid

api/src/opentrons/protocol_engine/commands/unsafe/unsafe_place_labware.py

vegano1 added 11 commits October 18, 2024 11:57

save

369bb40

Merge branch 'edge' into PLAT-451-handle-estop-with-plate-reader

b7b989f

save

d42b4ba

working place labware command

893da7a

feat(api): add unsafe/placeLabware command to place labware the gripp…

2fcce8e

…er is holding somewhere on the deck.

feat(robot-server): add placeLabwareState and estopEngaged to runs/cu…

0e201e0

…rrentState endpoint.

clean up

3ae5e2f

format

f0b4766

api, always move the plate reader lid to its dock

a35161b

feat(app, shared-data): add usePlacePlateReaderLid to EstopPressedMod…

c7e8264

…al to handle plate reader lid.

save

2a7a4b4

vegano1 requested review from a team as code owners October 22, 2024 23:17

vegano1 requested review from TamarZanzouri, smb2268, SyntaxColoring and mjhuff and removed request for a team and TamarZanzouri October 22, 2024 23:17

mjhuff requested changes Oct 22, 2024

View reviewed changes

conditionally call unsafe/placeLabware command based on the runs/curr…

2cb3765

…entState

vegano1 force-pushed the PLAT-451-handle-estop-with-plate-reader branch from cafa0c5 to 2cb3765 Compare October 23, 2024 14:19

vegano1 added 7 commits October 23, 2024 13:37

simplify usePlacePlateReaderLid hook

837fe9a

Merge branch 'edge' into PLAT-451-handle-estop-with-plate-reader

74b02cc

fix bad edge merge

6861d3d

move EstopTakeover component below maintenanceRunTakeover so we dont …

6c4de3e

…show the remote-controlled takeover modal when we are automatically moving the plate reader lid

refactor estopPressed

de39cbe

clean up

3dee541

Merge branch 'edge' into PLAT-451-handle-estop-with-plate-reader

39fb23c

vegano1 requested review from mjhuff and ahiuchingau October 24, 2024 17:15

mjhuff approved these changes Oct 24, 2024

View reviewed changes

default the filename arg of absorbance read command to None

ddc24cb

vegano1 force-pushed the PLAT-451-handle-estop-with-plate-reader branch from 7af2905 to ddc24cb Compare October 24, 2024 20:55

format

d9f9175

CaseyBatten approved these changes Oct 28, 2024

View reviewed changes

ahiuchingau approved these changes Oct 28, 2024

View reviewed changes

add unit tests

337b515

SyntaxColoring reviewed Oct 29, 2024

View reviewed changes

vegano1 added 5 commits October 29, 2024 13:35

format

6fe1b96

Merge branch 'edge' into PLAT-451-handle-estop-with-plate-reader

2da1b7a

cleanup

2023531

format

8c96485

formatr

8cf7a54

vegano1 merged commit 272c4b6 into edge Oct 29, 2024
61 checks passed

vegano1 deleted the PLAT-451-handle-estop-with-plate-reader branch October 29, 2024 19:51

SyntaxColoring mentioned this pull request Nov 8, 2024

fix(api): Do not load the absorbance reader lid with loadLabware #16734

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api, robot-server, app): Handle the Plate Reader lid while held by the gripper when the Estop is pressed. #16574

feat(api, robot-server, app): Handle the Plate Reader lid while held by the gripper when the Estop is pressed. #16574

vegano1 commented Oct 22, 2024 •

edited

Loading

mjhuff left a comment •

edited

Loading

mjhuff left a comment •

edited

Loading

mjhuff Oct 24, 2024

codecov bot commented Oct 24, 2024

CaseyBatten left a comment

CaseyBatten Oct 28, 2024

CaseyBatten Oct 28, 2024

vegano1 Oct 28, 2024

ahiuchingau left a comment

ahiuchingau Oct 28, 2024

vegano1 Oct 28, 2024 •

edited

Loading

SyntaxColoring left a comment

SyntaxColoring Oct 29, 2024

SyntaxColoring Oct 29, 2024

SyntaxColoring Oct 29, 2024

SyntaxColoring Oct 29, 2024

	..., description="The location the Plate Reade lid should be in."
	..., description="The location the Plate Reader lid should be in."

feat(api, robot-server, app): Handle the Plate Reader lid while held by the gripper when the Estop is pressed. #16574

feat(api, robot-server, app): Handle the Plate Reader lid while held by the gripper when the Estop is pressed. #16574

Conversation

vegano1 commented Oct 22, 2024 • edited Loading

Overview

Test Plan and Hands on Testing

Changelog

Review requests

Risk assessment

mjhuff left a comment • edited Loading

Choose a reason for hiding this comment

mjhuff left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 24, 2024

Codecov Report

CaseyBatten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahiuchingau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vegano1 Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

SyntaxColoring left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vegano1 commented Oct 22, 2024 •

edited

Loading

mjhuff left a comment •

edited

Loading

mjhuff left a comment •

edited

Loading

vegano1 Oct 28, 2024 •

edited

Loading