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

fix(engine): pause hardware API when engine is paused #10882

Merged
merged 4 commits into from
Jun 30, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions api/src/opentrons/hardware_control/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ def __init__(
# home() call succeeds or fails.
self._motion_lock = asyncio.Lock()
self._door_state = DoorState.CLOSED
self._pause_manager = PauseManager(self._door_state)
self._pause_manager = PauseManager()
ExecutionManagerProvider.__init__(self, isinstance(backend, Simulator))
RobotCalibrationProvider.__init__(self)
InstrumentHandlerProvider.__init__(
Expand All @@ -136,11 +136,8 @@ def door_state(self, door_state: DoorState) -> None:
def _update_door_state(self, door_state: DoorState) -> None:
mod_log.info(f"Updating the window switch status: {door_state}")
self.door_state = door_state
self._pause_manager.set_door(self.door_state)
for cb in self._callbacks:
hw_event = DoorStateNotification(
new_state=door_state, blocking=self._pause_manager.blocked_by_door
)
hw_event = DoorStateNotification(new_state=door_state)
try:
cb(hw_event)
except Exception:
Expand Down
7 changes: 2 additions & 5 deletions api/src/opentrons/hardware_control/ot3api.py
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ def __init__(
# home() call succeeds or fails.
self._motion_lock = asyncio.Lock()
self._door_state = DoorState.CLOSED
self._pause_manager = PauseManager(self._door_state)
self._pause_manager = PauseManager()
self._transforms = build_ot3_transforms(self._config)
self._gantry_load = GantryLoad.NONE
self._move_manager = MoveManager(
Expand Down Expand Up @@ -199,11 +199,8 @@ async def set_gantry_load(self, gantry_load: GantryLoad) -> None:
def _update_door_state(self, door_state: DoorState) -> None:
mod_log.info(f"Updating the window switch status: {door_state}")
self.door_state = door_state
self._pause_manager.set_door(self.door_state)
for cb in self._callbacks:
hw_event = DoorStateNotification(
new_state=door_state, blocking=self._pause_manager.blocked_by_door
)
hw_event = DoorStateNotification(new_state=door_state)
try:
cb(hw_event)
except Exception:
Expand Down
26 changes: 5 additions & 21 deletions api/src/opentrons/hardware_control/pause_manager.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
from typing import List

from opentrons.config import feature_flags as ff

from .types import DoorState, PauseType, PauseResumeError
from .types import PauseType


class PauseManager:
Expand All @@ -12,32 +10,18 @@ class PauseManager:
timer runs out) and the pause resume (trigged by user via the app).
"""

def __init__(self, door_state: DoorState) -> None:
def __init__(self) -> None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked in with @sfoster1 on this one: the HW API would rather simply report door open and close events. This lines up nicely with a desire on our end to stop deeply nesting access to the config singleton.

Given that this PR gives the ProtocolEngine enough information to reject an invalid resume while the door is open, I removed the unnecessary door logic from PauseManager

Copy link
Contributor

Choose a reason for hiding this comment

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

My opinion isn't a super informed one, but I'm a big fan of this. It seems way easier to reason about the HW API if we're always controlling it and it's never controlling itself independently.

self.queue: List[PauseType] = []
self._blocked_by_door = self._evaluate_door_state(door_state)

@property
def should_pause(self) -> bool:
return bool(self.queue)

@property
def blocked_by_door(self) -> bool:
return self._blocked_by_door

def _evaluate_door_state(self, door_state: DoorState) -> bool:
if ff.enable_door_safety_switch():
return door_state is DoorState.OPEN
return False

def set_door(self, door_state: DoorState) -> None:
self._blocked_by_door = self._evaluate_door_state(door_state)

def resume(self, pause_type: PauseType) -> None:
# door should be closed before a resume from the app can be received
if self._blocked_by_door and pause_type is PauseType.PAUSE:
raise PauseResumeError
if pause_type in self.queue:
try:
self.queue.remove(pause_type)
except ValueError:
pass

def pause(self, pause_type: PauseType) -> None:
if pause_type not in self.queue:
Expand Down
9 changes: 2 additions & 7 deletions api/src/opentrons/hardware_control/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -295,16 +295,15 @@ class HardwareEventType(enum.Enum):
ERROR_MESSAGE = enum.auto()


@dataclass
@dataclass(frozen=True)
class DoorStateNotification:
event: Literal[
HardwareEventType.DOOR_SWITCH_CHANGE
] = HardwareEventType.DOOR_SWITCH_CHANGE
new_state: DoorState = DoorState.CLOSED
blocking: bool = False


@dataclass
@dataclass(frozen=True)
class ErrorMessageNotification:
message: str
event: Literal[HardwareEventType.ERROR_MESSAGE] = HardwareEventType.ERROR_MESSAGE
Expand Down Expand Up @@ -431,10 +430,6 @@ def build(cls, name: str, flags: List[enum.Enum]) -> "AionotifyEvent":
return cls(flags=Flag, name=name)


class PauseResumeError(RuntimeError):
pass


class ExecutionCancelledError(RuntimeError):
pass

Expand Down
11 changes: 2 additions & 9 deletions api/src/opentrons/protocol_engine/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,7 @@
CommandType,
CommandIntent,
)
from .state import (
State,
StateView,
CommandSlice,
CurrentCommand,
EngineConfigs,
StateSummary,
)
from .state import State, StateView, StateSummary, CommandSlice, CurrentCommand, Config
from .plugins import AbstractPlugin

from .types import (
Expand Down Expand Up @@ -52,8 +45,8 @@
# main factory and interface exports
"create_protocol_engine",
"ProtocolEngine",
"EngineConfigs",
"StateSummary",
"Config",
# error types
"ProtocolEngineError",
"ErrorOccurrence",
Expand Down
19 changes: 5 additions & 14 deletions api/src/opentrons/protocol_engine/create_protocol_engine.py
Original file line number Diff line number Diff line change
@@ -1,40 +1,31 @@
"""Main ProtocolEngine factory."""
from opentrons.hardware_control import HardwareControlAPI
from opentrons.hardware_control.types import DoorState
from opentrons.config import feature_flags

from .protocol_engine import ProtocolEngine
from .resources import DeckDataProvider
from .state import StateStore, EngineConfigs
from .state import Config, StateStore


async def create_protocol_engine(
hardware_api: HardwareControlAPI,
configs: EngineConfigs = EngineConfigs(),
config: Config,
) -> ProtocolEngine:
"""Create a ProtocolEngine instance.

Arguments:
hardware_api: Hardware control API to pass down to dependencies.
configs: Protocol Engine configurations.
config: ProtocolEngine configuration.
"""
# TODO(mc, 2020-11-18): check short trash FF
deck_data = DeckDataProvider()
deck_definition = await deck_data.get_deck_definition()
deck_fixed_labware = await deck_data.get_deck_fixed_labware(deck_definition)
# TODO(mc, 2021-09-22): figure out a better way to load deck data that
# can more consistently handle Python vs JSON vs legacy differences

is_door_blocking = (
feature_flags.enable_door_safety_switch()
and hardware_api.door_state is DoorState.OPEN
)

state_store = StateStore(
config=config,
deck_definition=deck_definition,
deck_fixed_labware=deck_fixed_labware,
configs=configs,
is_door_blocking=is_door_blocking,
is_door_open=hardware_api.door_state is DoorState.OPEN,
)

return ProtocolEngine(state_store=state_store, hardware_api=hardware_api)
4 changes: 2 additions & 2 deletions api/src/opentrons/protocol_engine/execution/equipment.py
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ async def load_module(
assigned to the requested location.
"""
# TODO(mc, 2022-02-09): validate module location given deck definition
use_virtual_modules = self._state_store.get_configs().use_virtual_modules
use_virtual_modules = self._state_store.config.use_virtual_modules

if not use_virtual_modules:
attached_modules = [
Expand Down Expand Up @@ -275,7 +275,7 @@ def get_module_hardware_api(

def get_module_hardware_api(self, module_id: str) -> Optional[AbstractModule]:
"""Get the hardware API for a given module."""
use_virtual_modules = self._state_store.get_configs().use_virtual_modules
use_virtual_modules = self._state_store.config.use_virtual_modules
if use_virtual_modules:
return None

Expand Down
4 changes: 2 additions & 2 deletions api/src/opentrons/protocol_engine/execution/run_control.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,13 @@ def __init__(

async def wait_for_resume(self) -> None:
"""Issue a PauseAction to the store, pausing the run."""
if not self._state_store.get_configs().ignore_pause:
if not self._state_store.config.ignore_pause:
self._action_dispatcher.dispatch(PauseAction(source=PauseSource.PROTOCOL))
await self._state_store.wait_for(
condition=self._state_store.commands.get_is_running
)

async def wait_for_duration(self, seconds: float) -> None:
"""Delay protocol execution for a duration."""
if not self._state_store.get_configs().ignore_pause:
if not self._state_store.config.ignore_pause:
await asyncio.sleep(seconds)
9 changes: 8 additions & 1 deletion api/src/opentrons/protocol_engine/protocol_engine.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from opentrons.protocols.models import LabwareDefinition
from opentrons.hardware_control import HardwareControlAPI
from opentrons.hardware_control.modules import AbstractModule as HardwareModuleAPI
from opentrons.hardware_control.types import PauseType as HardwarePauseType

from .resources import ModelUtils, ModuleDataProvider
from .commands import Command, CommandCreate
Expand Down Expand Up @@ -59,6 +60,7 @@ def __init__(
This constructor does not inject provider implementations.
Prefer the `create_protocol_engine()` factory function.
"""
self._hardware_api = hardware_api
self._state_store = state_store
self._model_utils = model_utils or ModelUtils()

Expand Down Expand Up @@ -107,14 +109,19 @@ def play(self) -> None:
PlayAction(requested_at=requested_at)
)
self._action_dispatcher.dispatch(action)
self._queue_worker.start()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this an intentional removal of self._queue_worker.start()?

Copy link
Contributor

Choose a reason for hiding this comment

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

@SyntaxColoring @mcous I saw it the tests that instead we are now calling hardware_api.resume(HardwarePauseType.PAUSE)
hardware_api.pause(HardwarePauseType.PAUSE)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is intentional; queue_worker.start is called in __init__ so all subsequent calls will no-op


if self._state_store.commands.get_is_door_blocking():
self._hardware_api.pause(HardwarePauseType.PAUSE)
TamarZanzouri marked this conversation as resolved.
Show resolved Hide resolved
else:
self._hardware_api.resume(HardwarePauseType.PAUSE)

def pause(self) -> None:
"""Pause executing commands in the queue."""
action = self._state_store.commands.validate_action_allowed(
PauseAction(source=PauseSource.CLIENT)
)
self._action_dispatcher.dispatch(action)
self._hardware_api.pause(HardwarePauseType.PAUSE)

def add_command(self, request: CommandCreate) -> Command:
"""Add a command to the `ProtocolEngine`'s queue.
Expand Down
6 changes: 3 additions & 3 deletions api/src/opentrons/protocol_engine/state/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

from .state import State, StateStore, StateView
from .state_summary import StateSummary
from .config import Config
from .commands import CommandState, CommandView, CommandSlice, CurrentCommand
from .labware import LabwareState, LabwareView
from .pipettes import PipetteState, PipetteView, HardwarePipette, CurrentWell
Expand All @@ -19,15 +20,15 @@
)
from .geometry import GeometryView, TipGeometry
from .motion import MotionView, PipetteLocationData
from .configs import EngineConfigs


__all__ = [
# top level state value and interfaces
"State",
"StateStore",
"StateView",
"StateSummary",
# static engine configuration
"Config",
# command state and values
"CommandState",
"CommandView",
Expand Down Expand Up @@ -60,5 +61,4 @@
# computed motion state
"MotionView",
"PipetteLocationData",
"EngineConfigs",
]
18 changes: 14 additions & 4 deletions api/src/opentrons/protocol_engine/state/commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
)
from ..types import EngineStatus
from .abstract_store import HasState, HandlesActions
from .config import Config


class QueueStatus(str, Enum):
Expand Down Expand Up @@ -152,11 +153,17 @@ class CommandStore(HasState[CommandState], HandlesActions):

_state: CommandState

def __init__(self, is_door_blocking: bool = False) -> None:
def __init__(
self,
*,
config: Config,
is_door_open: bool,
) -> None:
"""Initialize a CommandStore and its state."""
self._config = config
self._state = CommandState(
queue_status=QueueStatus.SETUP,
is_door_blocking=is_door_blocking,
is_door_blocking=is_door_open and config.block_on_door_open,
run_result=None,
running_command_id=None,
all_command_ids=[],
Expand Down Expand Up @@ -327,8 +334,11 @@ def handle_action(self, action: Action) -> None: # noqa: C901
self._state.run_completed_at = action.completed_at

elif isinstance(action, HardwareEventAction):
if isinstance(action.event, DoorStateNotification):
if action.event.blocking:
if (
isinstance(action.event, DoorStateNotification)
and self._config.block_on_door_open
):
if action.event.new_state == DoorState.OPEN:
self._state.is_door_blocking = True
if self._state.queue_status != QueueStatus.SETUP:
self._state.queue_status = QueueStatus.PAUSED
Expand Down
11 changes: 11 additions & 0 deletions api/src/opentrons/protocol_engine/state/config.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
"""Top-level ProtocolEngine configuration options."""
from dataclasses import dataclass


@dataclass(frozen=True)
class Config:
"""ProtocolEngine configuration options."""

ignore_pause: bool = False
use_virtual_modules: bool = False
mcous marked this conversation as resolved.
Show resolved Hide resolved
block_on_door_open: bool = False
10 changes: 0 additions & 10 deletions api/src/opentrons/protocol_engine/state/configs.py

This file was deleted.

2 changes: 1 addition & 1 deletion api/src/opentrons/protocol_engine/state/modules.py
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ class ModuleView(HasState[ModuleState]):

_state: ModuleState

def __init__(self, state: ModuleState, virtualize_modules: bool) -> None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Argument was completely unused

def __init__(self, state: ModuleState) -> None:
"""Initialize the view with its backing state value."""
self._state = state

Expand Down
Loading