Skip to content

Commit

Permalink
fix(app): improve robot update via usb (#13849)
Browse files Browse the repository at this point in the history
Co-authored-by: Shlok Amin <[email protected]>
  • Loading branch information
mjhuff and shlokamin committed Nov 1, 2023
1 parent 692888c commit 85369e7
Show file tree
Hide file tree
Showing 5 changed files with 143 additions and 22 deletions.
45 changes: 45 additions & 0 deletions app-shell/src/usb.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import type { Action, Dispatch } from './types'

let usbHttpAgent: SerialPortHttpAgent | undefined
const usbLog = createLogger('usb')
let usbFetchInterval: NodeJS.Timeout

export function getSerialPortHttpAgent(): SerialPortHttpAgent | undefined {
return usbHttpAgent
Expand All @@ -43,6 +44,7 @@ export function createSerialPortHttpAgent(path: string): void {
keepAliveMsecs: 10000,
path,
logger: usbLog,
timeout: 100000,
})

usbHttpAgent = serialPortHttpAgent
Expand Down Expand Up @@ -110,6 +112,48 @@ async function usbListener(
}
}

function pollSerialPortAndCreateAgent(dispatch: Dispatch): void {
// usb poll already initialized
if (usbFetchInterval != null) {
return

Check warning on line 118 in app-shell/src/usb.ts

View check run for this annotation

Codecov / codecov/patch

app-shell/src/usb.ts#L118

Added line #L118 was not covered by tests
}
usbFetchInterval = setInterval(() => {

Check warning on line 120 in app-shell/src/usb.ts

View check run for this annotation

Codecov / codecov/patch

app-shell/src/usb.ts#L120

Added line #L120 was not covered by tests
// already connected to an Opentrons robot via USB
if (getSerialPortHttpAgent() != null) {
return

Check warning on line 123 in app-shell/src/usb.ts

View check run for this annotation

Codecov / codecov/patch

app-shell/src/usb.ts#L123

Added line #L123 was not covered by tests
}
usbLog.debug('fetching serialport list')
fetchSerialPortList()

Check warning on line 126 in app-shell/src/usb.ts

View check run for this annotation

Codecov / codecov/patch

app-shell/src/usb.ts#L125-L126

Added lines #L125 - L126 were not covered by tests
.then((list: PortInfo[]) => {
const ot3UsbSerialPort = list.find(

Check warning on line 128 in app-shell/src/usb.ts

View check run for this annotation

Codecov / codecov/patch

app-shell/src/usb.ts#L128

Added line #L128 was not covered by tests
port =>
port.productId?.localeCompare(DEFAULT_PRODUCT_ID, 'en-US', {
sensitivity: 'base',
}) === 0 &&
port.vendorId?.localeCompare(DEFAULT_VENDOR_ID, 'en-US', {
sensitivity: 'base',
}) === 0
)

if (ot3UsbSerialPort == null) {
usbLog.debug('no OT-3 serial port found')
return

Check warning on line 140 in app-shell/src/usb.ts

View check run for this annotation

Codecov / codecov/patch

app-shell/src/usb.ts#L139-L140

Added lines #L139 - L140 were not covered by tests
}

createSerialPortHttpAgent(ot3UsbSerialPort.path)

Check warning on line 143 in app-shell/src/usb.ts

View check run for this annotation

Codecov / codecov/patch

app-shell/src/usb.ts#L143

Added line #L143 was not covered by tests
// remove any existing handler
ipcMain.removeHandler('usb:request')
ipcMain.handle('usb:request', usbListener)

Check warning on line 146 in app-shell/src/usb.ts

View check run for this annotation

Codecov / codecov/patch

app-shell/src/usb.ts#L145-L146

Added lines #L145 - L146 were not covered by tests

dispatch(usbRequestsStart())

Check warning on line 148 in app-shell/src/usb.ts

View check run for this annotation

Codecov / codecov/patch

app-shell/src/usb.ts#L148

Added line #L148 was not covered by tests
})
.catch(e =>
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
usbLog.debug(`fetchSerialPortList error ${e?.message ?? 'unknown'}`)
)
}, 10000)
}

function startUsbHttpRequests(dispatch: Dispatch): void {
fetchSerialPortList()
.then((list: PortInfo[]) => {
Expand Down Expand Up @@ -150,6 +194,7 @@ export function registerUsb(dispatch: Dispatch): (action: Action) => unknown {
if (action.payload.usbDevices.find(isUsbDeviceOt3) != null) {
startUsbHttpRequests(dispatch)
}
pollSerialPortAndCreateAgent(dispatch)

Check warning on line 197 in app-shell/src/usb.ts

View check run for this annotation

Codecov / codecov/patch

app-shell/src/usb.ts#L197

Added line #L197 was not covered by tests
break
case USB_DEVICE_ADDED:
if (isUsbDeviceOt3(action.payload.usbDevice)) {
Expand Down
3 changes: 2 additions & 1 deletion app/src/redux/robot-api/http.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { map, switchMap, catchError } from 'rxjs/operators'
import mapValues from 'lodash/mapValues'
import toString from 'lodash/toString'
import omitBy from 'lodash/omitBy'
import inRange from 'lodash/inRange'

import { OPENTRONS_USB } from '../../redux/discovery'
import { appShellRequestor } from '../../redux/shell/remote'
Expand Down Expand Up @@ -68,7 +69,7 @@ export function fetchRobotApi(
body: response?.data,
status: response?.status,
// appShellRequestor eventually calls axios.request, which doesn't provide an ok boolean in the response
ok: response?.statusText === 'OK',
ok: inRange(response?.status, 200, 300),
}))
)
: from(fetch(url, options)).pipe(
Expand Down
67 changes: 61 additions & 6 deletions app/src/redux/robot-update/__tests__/epic.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -286,23 +286,78 @@ describe('robot update epics', () => {
})
})

it('issues error if begin request fails without 409', () => {
it('sends request to cancel URL if a non 409 occurs and reissues CREATE_SESSION', () => {
testScheduler.run(({ hot, cold, expectObservable, flush }) => {
const action = actions.createSession(robot, '/server/update/begin')

mockFetchRobotApi.mockReturnValueOnce(
cold('r', { r: Fixtures.mockUpdateBeginFailure })
)
mockFetchRobotApi
.mockReturnValueOnce(
cold<RobotApiResponse>('r', { r: Fixtures.mockUpdateBeginFailure })
)
.mockReturnValueOnce(
cold<RobotApiResponse>('r', { r: Fixtures.mockUpdateCancelSuccess })
)

const action$ = hot<Action>('-a', { a: action })
const state$ = hot<State>('a-', { a: state } as any)
const output$ = epics.createSessionEpic(action$, state$)

expectObservable(output$).toBe('-e', {
e: actions.unexpectedRobotUpdateError(
expectObservable(output$).toBe('-a', { a: action })
flush()
expect(mockFetchRobotApi).toHaveBeenCalledWith(robot, {
method: 'POST',
path: '/server/update/cancel',
})
})
})

it('Issues an error if cancelling a session fails after a 409 error occurs', () => {
testScheduler.run(({ hot, cold, expectObservable, flush }) => {
const action = actions.createSession(robot, '/server/update/begin')

mockFetchRobotApi
.mockReturnValueOnce(
cold<RobotApiResponse>('r', { r: Fixtures.mockUpdateBeginConflict })
)
.mockReturnValueOnce(
cold<RobotApiResponse>('r', { r: Fixtures.mockUpdateCancelFailure })
)

const action$ = hot<Action>('-a', { a: action })
const state$ = hot<State>('a-', { a: state } as any)
const output$ = epics.createSessionEpic(action$, state$)

expectObservable(output$).toBe('-a', {
a: actions.unexpectedRobotUpdateError(
'Unable to cancel in-progress update session'
),
})
flush()
})
})

it('Issues an error if cancelling a session fails after a non 409 error occurs', () => {
testScheduler.run(({ hot, cold, expectObservable, flush }) => {
const action = actions.createSession(robot, '/server/update/begin')

mockFetchRobotApi
.mockReturnValueOnce(
cold<RobotApiResponse>('r', { r: Fixtures.mockUpdateBeginFailure })
)
.mockReturnValueOnce(
cold<RobotApiResponse>('r', { r: Fixtures.mockUpdateCancelFailure })
)

const action$ = hot<Action>('-a', { a: action })
const state$ = hot<State>('a-', { a: state } as any)
const output$ = epics.createSessionEpic(action$, state$)

expectObservable(output$).toBe('-a', {
a: actions.unexpectedRobotUpdateError(
'Unable to start update session'
),
})
flush()
})
})

Expand Down
11 changes: 10 additions & 1 deletion app/src/redux/robot-update/epic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,16 @@ export const createSessionEpic: Epic = action$ => {
)
}

return of(unexpectedRobotUpdateError(UNABLE_TO_START_UPDATE_SESSION))
return fetchRobotApi(host, {
method: POST,
path: `${pathPrefix}/cancel`,
}).pipe(
map(cancelResp => {
return cancelResp.ok
? createSession(host, path)
: unexpectedRobotUpdateError(UNABLE_TO_START_UPDATE_SESSION)
})
)
})
)
}
Expand Down
39 changes: 25 additions & 14 deletions usb-bridge/node-client/src/usb-agent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ export function createSerialPortListMonitor(
return { start, stop }
}

const SOCKET_OPEN_RETRY_TIME = 10000
class SerialPortSocket extends SerialPort {
// allow node socket destroy
destroy(): void {}
Expand Down Expand Up @@ -195,10 +196,18 @@ class SerialPortHttpAgent extends http.Agent {

const socket = new SerialPortSocket({
path: this.options.path,
baudRate: 115200,
baudRate: 1152000,
})
if (!socket.isOpen && !socket.opening) {
socket.open()
socket.open(error => {
this.log(
'error',
`could not open serialport socket: ${error?.message}. Retrying in ${SOCKET_OPEN_RETRY_TIME} ms`
)
setTimeout(() => {
socket.open()
}, SOCKET_OPEN_RETRY_TIME)
})
}
if (socket != null) oncreate(null, socket)
}
Expand Down Expand Up @@ -230,6 +239,11 @@ function installListeners(
}
s.on('free', onFree)

function onError(err: Error): void {
agent.log('error', `CLIENT socket onError: ${err?.message}`)
}
s.on('error', onError)

function onClose(): void {
agent.log('debug', 'CLIENT socket onClose')
// This is the only place where sockets get removed from the Agent.
Expand All @@ -241,18 +255,15 @@ function installListeners(
s.on('close', onClose)

function onTimeout(): void {
agent.log('debug', 'CLIENT socket onTimeout')

// Destroy if in free list.
// TODO(ronag): Always destroy, even if not in free list.
const sockets = agent.freeSockets
if (
Object.keys(sockets).some(name =>
sockets[name]?.includes((s as unknown) as Socket)
)
) {
return s.destroy()
}
agent.log(
'debug',
'CLIENT socket onTimeout, closing and reopening the socket'
)

s.close()
setTimeout(() => {
s.open()
}, 3000)
}
s.on('timeout', onTimeout)

Expand Down

0 comments on commit 85369e7

Please sign in to comment.