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(api, app): add state change information to rpc #5512

Merged
merged 7 commits into from
May 5, 2020
Merged
Show file tree
Hide file tree
Changes from all 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
17 changes: 17 additions & 0 deletions api/src/opentrons/api/dev_types.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
from typing_extensions import Literal, TypedDict
amitlissack marked this conversation as resolved.
Show resolved Hide resolved

State = Literal[
'loaded', 'running', 'finished', 'stopped', 'paused', 'error', None]


class StateInfo(TypedDict, total=False):
message: str
#: A message associated with the state change by the system for display
changedAt: float
#: The time at which the state changed, relative to the startTime
estimatedDuration: float
#: If relevant for the state (e.g. 'paused' caused by a delay() call) the
#: duration estimated for the state
userMessage: str
#: If provided by the mechanism that changed the state, a message from the
#: user
51 changes: 42 additions & 9 deletions api/src/opentrons/api/session.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
from functools import reduce, wraps
import logging
from time import time, sleep
from typing import List, Dict, Any, Optional
from typing import List, Dict, Any, Optional, Set, TYPE_CHECKING
from uuid import uuid4
from opentrons.drivers.smoothie_drivers.driver_3_0 import SmoothieAlarm
from opentrons.drivers.rpi_drivers.gpio_simulator import SimulatingGPIOCharDev
Expand All @@ -27,9 +27,13 @@

from opentrons.legacy_api.containers import get_container, location_to_list

if TYPE_CHECKING:
from .dev_types import State, StateInfo

log = logging.getLogger(__name__)

VALID_STATES = {'loaded', 'running', 'finished', 'stopped', 'paused', 'error'}
VALID_STATES: Set['State'] = {
'loaded', 'running', 'finished', 'stopped', 'paused', 'error'}


def _motion_lock(func):
Expand Down Expand Up @@ -241,7 +245,10 @@ def __init__(self, name, protocol, hardware, loop, broker, motion_lock):
self._simulating_ctx = ProtocolContext.build_using(
self._protocol, loop=self._loop, broker=self._broker)

self.state = None
self.state: 'State' = None
#: The current state
self.stateInfo: 'StateInfo' = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

See comment about statusInfo vs stateInfo. Also, is this only ever going to be used for pause? Should it be pauseInfo?

Copy link
Member Author

Choose a reason for hiding this comment

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

It can be used for any state; many of the functions will only be used in pause for now, but e.g. changedAt and estimatedDuration could be used for more.

#: A message associated with the current state
self.commands = []
self.command_log = {}
self.errors = []
Expand All @@ -256,7 +263,7 @@ def __init__(self, name, protocol, hardware, loop, broker, motion_lock):
self.modules = None
self.protocol_text = protocol.text

self.startTime = None
self.startTime: Optional[float] = None
self._motion_lock = motion_lock

def _hw_iface(self):
Expand Down Expand Up @@ -448,15 +455,20 @@ def stop(self):
self.set_state('stopped')
return self

def pause(self):
def pause(self,
reason: str = None,
user_message: str = None,
duration: float = None):
if self._use_v2:
self._hardware.pause()
# robot.pause in the legacy API will publish commands to the broker
# use the broker-less execute_pause instead
else:
robot.execute_pause()

self.set_state('paused')
self.set_state(
'paused', reason=reason,
user_message=user_message, duration=duration)
return self

def resume(self):
Expand All @@ -476,7 +488,9 @@ def on_command(message):
if message['$'] == 'before':
self.log_append()
if message['name'] == command_types.PAUSE:
self.set_state('paused')
self.set_state('paused',
reason='The protocol paused execution',
user_message=message['payload']['userMessage'])
if message['name'] == command_types.RESUME:
self.set_state('running')

Expand Down Expand Up @@ -553,13 +567,31 @@ def run(self):
self._broker.set_logger(self._default_logger)
return self

def set_state(self, state):
log.debug("State set to {}".format(state))
def set_state(self, state: 'State',
reason: str = None,
user_message: str = None,
duration: float = None):
if state not in VALID_STATES:
raise ValueError(
'Invalid state: {0}. Valid states are: {1}'
.format(state, VALID_STATES))
self.state = state
if user_message:
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be easier to just create a new self.stateInfo object here? No need to clear previous values, just overwrite instance with a new one.

Copy link
Member Author

Choose a reason for hiding this comment

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

It would definitely be cleaner if I wanted to have all the keys there all the time but I kind of like the idea of removing keys and making sure they always have values if present

Copy link
Contributor

Choose a reason for hiding this comment

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

Have it your way, but I'll make one last statement. Up to you. It's not that big a deal.

So... I am not a fan of having several if / else clauses that do almost exactly the same thing. You can filter out the None values when creating payload below.

self.stateInfo['userMessage'] = user_message
else:
self.stateInfo.pop('userMessage', None)
if reason:
self.stateInfo['message'] = reason
else:
self.stateInfo.pop('message', None)
if duration:
self.stateInfo['estimatedDuration'] = duration
else:
self.stateInfo.pop('estimatedDuration', None)
if self.startTime:
self.stateInfo['changedAt'] = now()-self.startTime
else:
self.stateInfo.pop('changedAt', None)
self._on_state_changed()

def log_append(self):
Expand Down Expand Up @@ -593,6 +625,7 @@ def _snapshot(self):

payload = {
'state': self.state,
'stateInfo': self.stateInfo,
'startTime': self.startTime,
'errors': self.errors,
'lastCommand': last_command
Expand Down
3 changes: 2 additions & 1 deletion api/src/opentrons/commands/commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -533,7 +533,8 @@ def pause(msg):
return make_command(
name=command_types.PAUSE,
payload={
'text': text
'text': text,
'userMessage': msg,
}
)

Expand Down
16 changes: 16 additions & 0 deletions api/tests/opentrons/api/test_session.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import pytest
import base64

from opentrons.api import session
from opentrons.api.session import (
_accumulate, _dedupe)
from tests.opentrons.conftest import state
Expand Down Expand Up @@ -132,6 +133,21 @@ def test_set_state(run_session):
run_session.set_state('impossible-state')


def test_set_state_info(run_session, monkeypatch):
assert run_session.stateInfo == {}
run_session.set_state('paused',
reason='test1',
user_message='cool message',
duration=10)
assert run_session.stateInfo == {'message': 'test1',
'userMessage': 'cool message',
'estimatedDuration': 10}
run_session.startTime = 300
monkeypatch.setattr(session, 'now', lambda: 350)
run_session.set_state('running')
assert run_session.stateInfo == {'changedAt': 50}


def test_error_append(run_session):
foo = Exception('Foo')
bar = Exception('Bar')
Expand Down
12 changes: 10 additions & 2 deletions app/src/components/RunLog/CommandList.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@ import { SessionAlert } from './SessionAlert'
import { Portal } from '../portal'
import styles from './styles.css'

import type { SessionStatus } from '../../robot'
import type { SessionStatus, SessionStatusInfo } from '../../robot'

export type CommandListProps = {|
commands: Array<any>,
sessionStatus: SessionStatus,
sessionStatusInfo: SessionStatusInfo,
showSpinner: boolean,
onResetClick: () => mixed,
|}
Expand All @@ -23,7 +24,13 @@ export class CommandList extends React.Component<CommandListProps> {
}

render() {
const { commands, sessionStatus, showSpinner, onResetClick } = this.props
const {
commands,
sessionStatus,
sessionStatusInfo,
showSpinner,
onResetClick,
} = this.props
const makeCommandToTemplateMapper = depth => command => {
const {
id,
Expand Down Expand Up @@ -88,6 +95,7 @@ export class CommandList extends React.Component<CommandListProps> {
{!showSpinner && (
<SessionAlert
sessionStatus={sessionStatus}
sessionStatusInfo={sessionStatusInfo}
onResetClick={onResetClick}
className={styles.alert}
/>
Expand Down
21 changes: 17 additions & 4 deletions app/src/components/RunLog/SessionAlert.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,27 @@ import * as React from 'react'
import { useSelector } from 'react-redux'
import { AlertItem } from '@opentrons/components'
import { getSessionError } from '../../robot/selectors'
import type { SessionStatus } from '../../robot'
import type { SessionStatus, SessionStatusInfo } from '../../robot'
import styles from './styles.css'

const buildPauseMessage = (message: ?string): string =>
message ? `: ${message}` : ''

const buildPause = (message: ?string): string =>
`Run paused${buildPauseMessage(message)}`

const buildPauseUserMessage = (message: ?string) =>
message && <div className={styles.pause_user_message}>{message}</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

<p> is more appropriate for copy than a <div>


export type SessionAlertProps = {|
sessionStatus: SessionStatus,
sessionStatusInfo: SessionStatusInfo,
className?: string,
onResetClick: () => mixed,
|}

export function SessionAlert(props: SessionAlertProps) {
const { sessionStatus, className, onResetClick } = props
const { sessionStatus, sessionStatusInfo, className, onResetClick } = props
const sessionError = useSelector(getSessionError)

switch (sessionStatus) {
Expand All @@ -36,8 +47,10 @@ export function SessionAlert(props: SessionAlertProps) {
className={className}
type="info"
icon={{ name: 'pause-circle' }}
title="Run paused"
/>
title={buildPause(sessionStatusInfo.message)}
>
{buildPauseUserMessage(sessionStatusInfo.userMessage)}
</AlertItem>
)

case 'stopped':
Expand Down
4 changes: 3 additions & 1 deletion app/src/components/RunLog/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,15 @@ import {
import { CommandList } from './CommandList'

import type { State, Dispatch } from '../../types'
import type { SessionStatus } from '../../robot'
import type { SessionStatus, SessionStatusInfo } from '../../robot'
import type { CommandListProps } from './CommandList'

export { ConfirmCancelModal } from './ConfirmCancelModal'

type SP = {|
commands: Array<any>,
sessionStatus: SessionStatus,
sessionStatusInfo: SessionStatusInfo,
showSpinner: boolean,
|}

Expand All @@ -33,6 +34,7 @@ function mapStateToProps(state: State): SP {
return {
commands: robotSelectors.getCommands(state),
sessionStatus: robotSelectors.getSessionStatus(state),
sessionStatusInfo: robotSelectors.getSessionStatusInfo(state),
showSpinner:
robotSelectors.getCancelInProgress(state) ||
robotSelectors.getSessionLoadInProgress(state),
Expand Down
8 changes: 8 additions & 0 deletions app/src/components/RunLog/styles.css
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,14 @@
/* stylelint-disable selector-class-pattern, font-family-no-missing-generic-family-keyword */
@import '@opentrons/components';

.pause_user_message {
margin-left: 0.5rem;
margin-right: 0.5rem;
font-style: italic;
overflow-y: scroll;
max-height: 4rem;
}

.run_page {
position: relative;
}
Expand Down
7 changes: 7 additions & 0 deletions app/src/robot/api-client/client.js
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,13 @@ export function client(dispatch) {
clearRunTimerInterval()
}

update.statusInfo = {
message: apiSession.stateInfo?.message ?? null,
userMessage: apiSession.stateInfo?.userMessage ?? null,
changedAt: apiSession.stateInfo?.changedAt ?? null,
estimatedDuration: apiSession.stateInfo?.estimatedDuration ?? null,
}

// both light and full updates may have the errors list
if (apiSession.errors) {
update.errors = apiSession.errors.map(e => ({
Expand Down
9 changes: 9 additions & 0 deletions app/src/robot/reducer/session.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import type {
Mount,
Slot,
SessionStatus,
SessionStatusInfo,
} from '../types'

import type { InvalidProtocolFileAction } from '../../protocol/types'
Expand All @@ -25,6 +26,7 @@ type Request = {
export type SessionState = {
sessionRequest: Request,
state: SessionStatus,
statusInfo: SessionStatusInfo,
errors: Array<{|
timestamp: number,
line: number,
Expand Down Expand Up @@ -72,6 +74,12 @@ const INITIAL_STATE: SessionState = {
// loading a protocol
sessionRequest: { inProgress: false, error: null },
state: '',
statusInfo: {
message: null,
changedAt: null,
estimatedDuration: null,
userMessage: null,
},
errors: [],
protocolCommands: [],
protocolCommandsById: {},
Expand Down Expand Up @@ -196,6 +204,7 @@ function handleSessionUpdate(
return {
...state,
state: sessionState,
statusInfo: action.payload.statusInfo,
remoteTimeCompensation,
startTime,
protocolCommandsById,
Expand Down
5 changes: 5 additions & 0 deletions app/src/robot/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import type {
LabwareCalibrationStatus,
LabwareType,
SessionStatus,
SessionStatusInfo,
SessionModule,
TiprackByMountMap,
} from './types'
Expand Down Expand Up @@ -83,6 +84,10 @@ export function getSessionStatus(state: State): SessionStatus {
return session(state).state
}

export function getSessionStatusInfo(state: State): SessionStatusInfo {
return session(state).statusInfo
}

export function getSessionIsLoaded(state: State): boolean {
return getSessionStatus(state) !== ('': SessionStatus)
}
Expand Down
Loading