refactor(api): Split UpdateCommandAction

api/src/opentrons/protocol_engine/actions/__init__.py

@@ -14,7 +14,8 @@
     FinishAction,
     HardwareStoppedAction,
     QueueCommandAction,
-    UpdateCommandAction,
+    RunCommandAction,
+    SucceedCommandAction,
     FailCommandAction,
     AddLabwareOffsetAction,
     AddLabwareDefinitionAction,
@@ -40,7 +41,8 @@
     "FinishAction",
     "HardwareStoppedAction",
     "QueueCommandAction",
-    "UpdateCommandAction",
+    "RunCommandAction",
+    "SucceedCommandAction",
     "FailCommandAction",
     "AddLabwareOffsetAction",
     "AddLabwareDefinitionAction",

api/src/opentrons/protocol_engine/actions/actions.py

@@ -121,16 +121,35 @@ class QueueCommandAction:
 
 
 @dataclass(frozen=True)
-class UpdateCommandAction:
-    """Update a given command."""
+class RunCommandAction:
+    """Mark a given command as running.
+
+    The command must be queued immediately prior to this action.
+    """
+
+    command_id: str
+    started_at: datetime
+
+
+@dataclass(frozen=True)
+class SucceedCommandAction:
+    """Mark a given command as succeeded.
+
+    The command must be running immediately prior to this action.
+    """
 
     command: Command
+    """The command in its new succeeded state."""
+
     private_result: CommandPrivateResult
 
 
 @dataclass(frozen=True)
 class FailCommandAction:
-    """Mark a given command as failed."""
+    """Mark a given command as failed.
+
+    The command must be running immediately prior to this action.
+    """
 
     command_id: str
     error_id: str
@@ -211,7 +230,8 @@ class SetPipetteMovementSpeedAction:
     HardwareStoppedAction,
     DoorChangeAction,
     QueueCommandAction,
-    UpdateCommandAction,
+    RunCommandAction,
+    SucceedCommandAction,
     FailCommandAction,
     AddLabwareOffsetAction,
     AddLabwareDefinitionAction,

api/src/opentrons/protocol_engine/execution/command_executor.py

@@ -22,7 +22,12 @@
     CommandPrivateResult,
     Command,
 )
-from ..actions import ActionDispatcher, UpdateCommandAction, FailCommandAction
+from ..actions import (
+    ActionDispatcher,
+    RunCommandAction,
+    SucceedCommandAction,
+    FailCommandAction,
+)
 from ..errors import RunStoppedError
 from ..errors.exceptions import EStopActivatedError as PE_EStopActivatedError
 from ..notes import CommandNote, CommandNoteTracker
@@ -140,7 +145,7 @@ async def execute(self, command_id: str) -> None:
         )
 
         self._action_dispatcher.dispatch(
-            UpdateCommandAction(command=running_command, private_result=None)
+            RunCommandAction(command_id=command.id, started_at=started_at)
         )
 
         try:
@@ -168,12 +173,15 @@ async def execute(self, command_id: str) -> None:
             )
 
             if notes_update:
-                command_with_new_notes = running_command.copy(update=notes_update)
-                self._action_dispatcher.dispatch(
-                    UpdateCommandAction(
-                        command=command_with_new_notes, private_result=None
-                    )
-                )
+                # FIX BEFORE MERGE
+                pass
+                # command_with_new_notes = running_command.copy(update=notes_update)
+                # self._action_dispatcher.dispatch(
+                #     # FIX BEFORE MERGE
+                #     SucceedCommandAction(
+                #         command=command_with_new_notes, private_result=None
+                #     )
+                # )
 
             self._action_dispatcher.dispatch(
                 FailCommandAction(
@@ -203,7 +211,7 @@ async def execute(self, command_id: str) -> None:
             }
             completed_command = running_command.copy(update=update)
             self._action_dispatcher.dispatch(
-                UpdateCommandAction(
+                SucceedCommandAction(
                     command=completed_command, private_result=private_result
                 ),
             )

api/src/opentrons/protocol_engine/state/addressable_areas.py

@@ -35,7 +35,7 @@
     DeckConfigurationType,
     Dimensions,
 )
-from ..actions import Action, UpdateCommandAction, PlayAction, AddAddressableAreaAction
+from ..actions import Action, SucceedCommandAction, PlayAction, AddAddressableAreaAction
 from .config import Config
 from .abstract_store import HasState, HandlesActions
 
@@ -182,7 +182,7 @@ def __init__(
 
     def handle_action(self, action: Action) -> None:
         """Modify state in reaction to an action."""
-        if isinstance(action, UpdateCommandAction):
+        if isinstance(action, SucceedCommandAction):
             self._handle_command(action.command)
         elif isinstance(action, AddAddressableAreaAction):
             self._check_location_is_addressable_area(action.addressable_area)

api/src/opentrons/protocol_engine/state/commands.py

@@ -13,13 +13,16 @@
 from opentrons.ordered_set import OrderedSet
 
 from opentrons.hardware_control.types import DoorState
-from opentrons.protocol_engine.actions.actions import ResumeFromRecoveryAction
+from opentrons.protocol_engine.actions.actions import (
+    ResumeFromRecoveryAction,
+    RunCommandAction,
+)
 from opentrons.protocol_engine.error_recovery_policy import ErrorRecoveryType
 
 from ..actions import (
     Action,
     QueueCommandAction,
-    UpdateCommandAction,
+    SucceedCommandAction,
     FailCommandAction,
     PlayAction,
     PauseAction,
@@ -261,41 +264,55 @@ def handle_action(self, action: Action) -> None:  # noqa: C901
             if action.request_hash is not None:
                 self._state.latest_command_hash = action.request_hash
 
-        # TODO(mc, 2021-12-28): replace "UpdateCommandAction" with explicit
-        # state change actions (e.g. RunCommandAction, SucceedCommandAction)
-        # to make a command's queue transition logic easier to follow
-        elif isinstance(action, UpdateCommandAction):
-            command = action.command
-            prev_entry = self._state.commands_by_id.get(command.id)
-
-            if prev_entry is None:
-                index = len(self._state.all_command_ids)
-                self._state.all_command_ids.append(command.id)
-                self._state.commands_by_id[command.id] = CommandEntry(
-                    index=index,
-                    command=command,
-                )
-            else:
-                self._state.commands_by_id[command.id] = CommandEntry(
-                    index=prev_entry.index,
-                    command=command,
-                )
+        elif isinstance(action, RunCommandAction):
+            prev_entry = self._state.commands_by_id[action.command_id]
+            assert prev_entry.command.status == CommandStatus.QUEUED
 
-            self._state.queued_command_ids.discard(command.id)
-            self._state.queued_setup_command_ids.discard(command.id)
+            running_command = prev_entry.command.copy(
+                update={
+                    "status": CommandStatus.RUNNING,
+                    "startedAt": action.started_at,
+                }
+            )
 
-            if command.status == CommandStatus.RUNNING:
-                self._state.running_command_id = command.id
-            elif self._state.running_command_id == command.id:
-                self._state.running_command_id = None
+            self._state.commands_by_id[action.command_id] = CommandEntry(
+                index=prev_entry.index, command=running_command
+            )
+
+            assert self._state.running_command_id is None
+            self._state.running_command_id = action.command_id
+
+            self._state.queued_command_ids.discard(action.command_id)
+            self._state.queued_setup_command_ids.discard(action.command_id)
+
+        elif isinstance(action, SucceedCommandAction):
+            prev_entry = self._state.commands_by_id[action.command.id]
+            assert prev_entry.command.status == CommandStatus.RUNNING
+
+            succeeded_command = action.command
+            assert succeeded_command.status == CommandStatus.SUCCEEDED
+
+            self._state.commands_by_id[action.command.id] = CommandEntry(
+                index=prev_entry.index,
+                command=succeeded_command,
+            )
+
+            assert self._state.running_command_id == action.command.id
+            self._state.running_command_id = None
+
+            self._state.queued_command_ids.discard(succeeded_command.id)
+            self._state.queued_setup_command_ids.discard(succeeded_command.id)
 
         elif isinstance(action, FailCommandAction):
+            prev_entry = self._state.commands_by_id[action.command_id]
+            assert prev_entry.command.status == CommandStatus.RUNNING
+
             error_occurrence = ErrorOccurrence.from_failed(
                 id=action.error_id,
                 createdAt=action.failed_at,
                 error=action.error,
             )
-            prev_entry = self._state.commands_by_id[action.command_id]
+
             # TODO(mc, 2022-06-06): add new "cancelled" status or similar
             self._update_to_failed(
                 command_id=action.command_id,

api/src/opentrons/protocol_engine/state/labware.py

@@ -52,7 +52,7 @@
 )
 from ..actions import (
     Action,
-    UpdateCommandAction,
+    SucceedCommandAction,
     AddLabwareOffsetAction,
     AddLabwareDefinitionAction,
 )
@@ -152,7 +152,7 @@ def __init__(
 
     def handle_action(self, action: Action) -> None:
         """Modify state in reaction to an action."""
-        if isinstance(action, UpdateCommandAction):
+        if isinstance(action, SucceedCommandAction):
             self._handle_command(action.command)
 
         elif isinstance(action, AddLabwareOffsetAction):

api/src/opentrons/protocol_engine/state/modules.py

@@ -54,7 +54,7 @@
     temperature_module,
     thermocycler,
 )
-from ..actions import Action, UpdateCommandAction, AddModuleAction
+from ..actions import Action, SucceedCommandAction, AddModuleAction
 from .abstract_store import HasState, HandlesActions
 from .module_substates import (
     MagneticModuleSubState,
@@ -195,7 +195,7 @@ def __init__(
 
     def handle_action(self, action: Action) -> None:
         """Modify state in reaction to an action."""
-        if isinstance(action, UpdateCommandAction):
+        if isinstance(action, SucceedCommandAction):
             self._handle_command(action.command)
 
         elif isinstance(action, AddModuleAction):

api/src/opentrons/protocol_engine/state/pipettes.py

@@ -56,7 +56,7 @@
 from ..actions import (
     Action,
     SetPipetteMovementSpeedAction,
-    UpdateCommandAction,
+    SucceedCommandAction,
 )
 from .abstract_store import HasState, HandlesActions
 
@@ -150,7 +150,7 @@ def __init__(self) -> None:
 
     def handle_action(self, action: Action) -> None:
         """Modify state in reaction to an action."""
-        if isinstance(action, UpdateCommandAction):
+        if isinstance(action, SucceedCommandAction):
             self._handle_command(action.command, action.private_result)
         elif isinstance(action, SetPipetteMovementSpeedAction):
             self._state.movement_speed_by_id[action.pipette_id] = action.speed

api/src/opentrons/protocol_engine/state/tips.py

@@ -6,7 +6,7 @@
 from .abstract_store import HasState, HandlesActions
 from ..actions import (
     Action,
-    UpdateCommandAction,
+    SucceedCommandAction,
     ResetTipsAction,
 )
 from ..commands import (
@@ -64,7 +64,7 @@ def __init__(self) -> None:
 
     def handle_action(self, action: Action) -> None:
         """Modify state in reaction to an action."""
-        if isinstance(action, UpdateCommandAction):
+        if isinstance(action, SucceedCommandAction):
             if isinstance(action.private_result, PipetteConfigUpdateResultMixin):
                 pipette_id = action.private_result.pipette_id
                 config = action.private_result.config