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(app): Run timer stays at 00:00:00 if you reconnect in the middle of a delay #7841

Closed
wants to merge 11 commits into from
37 changes: 34 additions & 3 deletions app/src/redux/robot/reducer/session.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,18 @@ export type SessionState = {
remoteTimeCompensation: number | null,
startTime: ?number,
runTime: number,
/**
* The total number of seconds that this protocol has spent paused, across all steps executed so far.
* todo: hacky in that it is managed locally. Not part of the start-time fix,
* but partially fixes issue of pausing without pausing the timer. Keep?
*/
pausedDuration: number,
/**
* When paused it is the time at which protocol execution pause was started. When not paused it is null.
* todo: hacky in that it is managed locally. Not part of the start-time fix,
* but partially fixes issue of pausing without pausing the timer. Keep?
*/
pausedTime: number | null,
apiLevel: [number, number] | null,
capabilities: Array<string>,
}
Expand Down Expand Up @@ -104,6 +116,8 @@ const INITIAL_STATE: SessionState = {
remoteTimeCompensation: null,
startTime: null,
runTime: 0,
pausedTime: null,
pausedDuration: 0,
apiLevel: null,
}

Expand Down Expand Up @@ -222,6 +236,7 @@ function handleSessionUpdate(
function handleSessionInProgress(state: SessionState): SessionState {
return {
...state,
pausedDuration: 0,
runTime: 0,
startTime: null,
remoteTimeCompensation: null,
Expand Down Expand Up @@ -262,7 +277,11 @@ function handleInvalidFile(
}

function handleRun(state: SessionState, action: any): SessionState {
return { ...state, runTime: 0, runRequest: { inProgress: true, error: null } }
return {
...state,
runTime: Date.now(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think calls to anything non-idempotent should be strictly avoided in Redux reducers. Which makes it very embarrassing to realize that handleTickRunTime below has been sitting there for 4 years with a call to Date.now() that I definitely wrote...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, don't be too hard on yourself. It's probably one of my edits :)

runRequest: { inProgress: true, error: null },
}
}

function handleRunResponse(state: SessionState, action: any): SessionState {
Expand All @@ -280,7 +299,12 @@ function handleTickRunTime(state: SessionState, action: any): SessionState {
}

function handlePause(state: SessionState, action: any): SessionState {
return { ...state, pauseRequest: { inProgress: true, error: null } }
return {
...state,
pausedTime: Date.now(),
pauseRequest: { inProgress: true, error: null },
runTime: Date.now(),
}
}

function handlePauseResponse(state: SessionState, action: any): SessionState {
Expand All @@ -294,7 +318,14 @@ function handlePauseResponse(state: SessionState, action: any): SessionState {
}

function handleResume(state: SessionState, action: any): SessionState {
return { ...state, resumeRequest: { inProgress: true, error: null } }
const pausedDuration = state.pausedTime ? Date.now() - state.pausedTime : 0
return {
...state,
pausedDuration: state.pausedDuration + pausedDuration,
pausedTime: null,
resumeRequest: { inProgress: true, error: null },
runTime: Date.now(),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In the process of looking for and testing the bug found that pause does not pause the timer. The solution is hacky in that refresh wipes the slate clear. But I think for the most common use case it is better than the way it currently works or doesn't work depending on how you look at it.

So if you:

  1. Start running a protocol.
  2. Get through a bunch of pauses and delays in the protocol, amounting to 15 minutes.
  3. Disconnect from the robot or quit the app.
  4. Reconnect to the robot or reopen the app.

Then the run timer will jump up by 15 minutes between steps 3 and 4, since the app will no longer have any knowledge of pausedDuration?

If this is right, I don't think we should attempt to pause the timer during pauses at all. The timer being predictable and consistent is more important than it excluding pause time.

Copy link
Contributor

@SyntaxColoring SyntaxColoring May 28, 2021

Choose a reason for hiding this comment

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

Also:

When you're authoring a protocol, there's a distinction between a "delay" ("this scientific process requires our science juice to marinate for exactly 15 minutes before we move on to the next step") and a "pause" ("I want to wait here until the user clicks 'Resume'").

By the time these events reach the Opentrons App, I think (but I'm definitely not sure) that they get conflated into a single "pause" event.

It wouldn't be right for the timer to stop running during one of those deliberate 15-minute delays. But if we fix the timer running during pauses, we might accidentally do that?

Copy link
Contributor Author

@celsasser celsasser May 28, 2021

Choose a reason for hiding this comment

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

Yes, that's exactly right. Okay, then if we do want support for it then we would have to go deeper and make the robot-server or lower track pauses. But sounds like we should the hacky fix out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh dear, I've lost the ability to type proper sentences. Okay, I'll pull it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also:

When you're authoring a protocol, there's a distinction between a "delay" ("this scientific process requires our science juice to marinate for exactly 15 minutes before we move on to the next step") and a "pause" ("I want to wait here until the user clicks 'Resume'").

By the time these events reach the Opentrons App, I think (but I'm definitely not sure) that they get conflated into a single "pause" event.

It wouldn't be right for the timer to stop running during one of those deliberate 15-minute delays. But if we fix the timer running during pauses, we might accidentally do that?

I may have been mistaken about what you were saying. It's a little confusing. The pause I am addressing is not the delay in the protocol. It's 100% the Pause button in the app. When they press pause I am essentially stopping the timer from ticking and measuring the time between when the initiated pause and resumed it. And upon resume the timer ticks again.

I am not doing anything with delays scripted into protocols.

Does that change anything? The motivation for doing it was that we are conditioned to see things pause when we pause them. Maybe I spent too much time in the media industry.

Copy link
Contributor

@SyntaxColoring SyntaxColoring Jun 1, 2021

Choose a reason for hiding this comment

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

Sorry, I think you can disregard that—I think I'm just confusing myself. 🤐

}

function handleResumeResponse(state: SessionState, action: any): SessionState {
Expand Down
25 changes: 12 additions & 13 deletions app/src/redux/robot/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -203,12 +203,13 @@ export const getSessionError: State => string | null = createSelector(

const getStartTimeMs = (state: State): number | null => {
const { startTime, remoteTimeCompensation } = session(state)

if (startTime == null || remoteTimeCompensation === null) {
if (startTime == null) {
return null
}

return startTime + remoteTimeCompensation
// [7740] ce: 5/26/2021 - upon reload `remoteTimeCompensation` is null.
// will look to see why, but in the meantime this solves the start-time
// issue on reload but may create other issues?
return startTime + remoteTimeCompensation || 0
}

export const getStartTime: (state: State) => string | null = createSelector(
Expand All @@ -218,15 +219,13 @@ export const getStartTime: (state: State) => string | null = createSelector(
}
)

export const getRunSeconds: State => number = createSelector(
getStartTimeMs,
(state: State) => session(state).runTime,
(startTime: ?number, runTime: ?number): number => {
return runTime && startTime && runTime > startTime
? Math.floor((runTime - startTime) / 1000)
: 0
}
)
export function getRunSeconds(state: State): number {
const startTime = getStartTimeMs(state)
const { runTime, pausedDuration } = session(state)
return runTime && startTime
? Math.max(0, Math.floor((runTime - startTime - pausedDuration) / 1000))
: 0
}

export function formatSeconds(runSeconds: number): string {
const hours = padStart(`${Math.floor(runSeconds / 3600)}`, 2, '0')
Expand Down
40 changes: 27 additions & 13 deletions app/src/redux/robot/test/selectors.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ describe('robot selectors', () => {
const state = {
robot: {
session: {
pausedDuration: 0,
startTime: null,
runTime: 42,
},
Expand All @@ -227,28 +228,41 @@ describe('robot selectors', () => {
expect(getRunTime(state)).toEqual('00:00:00')
})

it('getRunTime with no remoteTimeCompensation', () => {
const state = {
robot: {
session: {
remoteTimeCompensation: null,
startTime: 40,
runTime: 42,
it('getRunTime without remoteTimeCompensation', () => {
const testGetRunTime = (seconds, expected) => {
const stateWithRunTime = {
robot: {
session: {
pausedDuration: 0,
remoteTimeCompensation: null,
startTime: 40,
runTime: 42 + 1000 * seconds,
},
},
},
}

expect(getRunTime(stateWithRunTime)).toEqual(expected)
}

expect(getRunTime(state)).toEqual('00:00:00')
testGetRunTime(0, '00:00:00')
testGetRunTime(1, '00:00:01')
testGetRunTime(59, '00:00:59')
testGetRunTime(60, '00:01:00')
testGetRunTime(61, '00:01:01')
testGetRunTime(3599, '00:59:59')
testGetRunTime(3600, '01:00:00')
testGetRunTime(3601, '01:00:01')
})

it('getRunTime', () => {
it('getRunTime with remoteTimeCompensation', () => {
const testGetRunTime = (seconds, expected) => {
const stateWithRunTime = {
robot: {
session: {
remoteTimeCompensation: 0,
startTime: 42,
runTime: 42 + 1000 * seconds,
pausedDuration: 0,
remoteTimeCompensation: 1000,
startTime: 40,
runTime: 1042 + 1000 * seconds,
},
},
}
Expand Down
13 changes: 11 additions & 2 deletions app/src/redux/robot/test/session-reducer.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ describe('robot reducer - session', () => {
startTime: null,
runTime: 0,
apiLevel: null,
pausedDuration: 0,
pausedTime: null,
})
})

Expand Down Expand Up @@ -113,6 +115,7 @@ describe('robot reducer - session', () => {
remoteTimeCompensation: null,
startTime: null,
runTime: 0,
pausedDuration: 0,
})
})

Expand Down Expand Up @@ -198,15 +201,14 @@ describe('robot reducer - session', () => {
it('handles RUN action', () => {
const state = {
session: {
runTime: now,
runRequest: { inProgress: false, error: new Error('AH') },
},
}
const action = { type: actionTypes.RUN }

expect(reducer(state, action).session).toEqual({
runRequest: { inProgress: true, error: null },
runTime: 0,
runTime: now,
})
})

Expand Down Expand Up @@ -248,7 +250,9 @@ describe('robot reducer - session', () => {
const action = { type: actionTypes.PAUSE }

expect(reducer(state, action).session).toEqual({
pausedTime: now,
pauseRequest: { inProgress: true, error: null },
runTime: now,
})
})

Expand Down Expand Up @@ -281,13 +285,18 @@ describe('robot reducer - session', () => {
it('handles RESUME action', () => {
const state = {
session: {
pausedDuration: 0,
pausedTime: now - 1000,
resumeRequest: { inProgress: false, error: new Error('AH') },
},
}
const action = { type: actionTypes.RESUME }

expect(reducer(state, action).session).toEqual({
pausedDuration: 1000,
pausedTime: null,
resumeRequest: { inProgress: true, error: null },
runTime: now,
})
})

Expand Down