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(engine): move pipettes away if blocking heater-shaker open latch or start shake #11248

Merged
merged 8 commits into from
Jul 29, 2022

Conversation

jbleon95
Copy link
Contributor

Overview

In protocol engine, adds check to openLabwareLatch and setAndWaitForShakeSpeed to determine if pipette is in an unsafe location for these operations. If they are (or location cannot be determined), the pipettes will home on the z-axis

Changelog

  • Pipette will home on z-axis for openLabwareLatch and setAndWaitForShakeSpeed commands if determined to be in unsafe or unknown location
  • Added methods to MotionView for determining if pipettes are blocking a shake or open latch command
  • Fixed touchTip and thermocycler openLid and closeLid commands not updating engine's current_well properly

Review requests

  • Needs to be tested on real robot
  • check_pipette_blocking_hs_shaker and check_pipette_blocking_hs_latch methods are both very similar, but I couldn't figure out a good method to combine them. Open to any ideas to reduce repeating ourselves there.

Risk assessment

Medium, this only affects two commands but if z-axis homing does not work properly for all expected situations the pipette could be in a bad location when these operations start.

@jbleon95 jbleon95 requested a review from a team as a code owner July 28, 2022 18:10
@codecov
Copy link

codecov bot commented Jul 28, 2022

Codecov Report

Merging #11248 (b4203a9) into edge (4366958) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             edge   #11248   +/-   ##
=======================================
  Coverage   73.82%   73.82%           
=======================================
  Files        2090     2090           
  Lines       57754    57754           
  Branches     5864     5864           
=======================================
  Hits        42636    42636           
  Misses      13833    13833           
  Partials     1285     1285           
Flag Coverage Δ
notify-server 89.17% <ø> (ø)

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

Impacted Files Coverage Δ
api/src/opentrons/protocol_engine/state/motion.py 100.00% <100.00%> (ø)
...pi/src/opentrons/protocol_engine/state/pipettes.py 100.00% <100.00%> (ø)

@jbleon95 jbleon95 added robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience). heater-shaker heater shaker software support labels Jul 28, 2022
Copy link
Member

@sanni-t sanni-t left a comment

Choose a reason for hiding this comment

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

This looks great! Good to merge if tested on a dev server at least. Would probably need to be tested with emulator to check that home gets issued.

Just a couple of comment nitpicks

@@ -56,6 +60,17 @@ async def execute(
# Verify speed from hs module view
validated_speed = hs_module_substate.validate_target_speed(params.rpm)

# Check if pipette would block opening latch if adjacent or on top of module
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Check if pipette would block opening latch if adjacent or on top of module
# Move pipette away if it is close to the heater-shaker

@@ -47,6 +51,17 @@ async def execute(self, params: OpenLabwareLatchParams) -> OpenLabwareLatchResul

hs_module_substate.raise_if_shaking()

# Check if pipette would block opening latch if east, west, or on top of module
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Check if pipette would block opening latch if east, west, or on top of module
# Move pipette away if it is close to the heater-shaker

Copy link
Member

Choose a reason for hiding this comment

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

I would specify the east/west location details in the docstring for check_pipette_blocking_hs_latch. Just a soft preference.

if self._state_view.motion.check_pipette_blocking_hs_latch(
hs_module_substate.module_id
):
await self._movement.home(
Copy link
Member

@sanni-t sanni-t Jul 28, 2022

Choose a reason for hiding this comment

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

From the discussion w/ the group on Tuesday I think we to move towards using pipette retraction instead of homing both here and before thermocycler lid movements. So I'd leave a TODO here to switch to using pipette retraction.

if self._state_view.motion.check_pipette_blocking_hs_shaker(
hs_module_substate.module_id
):
await self._movement.home(
Copy link
Member

Choose a reason for hiding this comment

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

Same TODO comment as above.

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.

Code changes look good to me and add tests are great! nice job! will try to test with the emulator

@sanni-t sanni-t merged commit e77add5 into edge Jul 29, 2022
@sanni-t
Copy link
Member

sanni-t commented Aug 1, 2022

Addresses protocol engine part of #11210, #11209

@sanni-t sanni-t deleted the pe-hs-commands-pipette-restrictions branch August 1, 2022 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
heater-shaker heater shaker software support robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants