feat(app,app-shell): usb request via app-shell #12292

brenthagen · 2023-03-14T21:44:35Z

This (large) PR enables http communication between the opentrons desktop app and an OT-3 robot via usb serial port connection. It adds a new package usb-bridge/node-client and uses it as follows:

app detects OT-3 usb device connection
app-shell fetches OT-3 serial port path associated with OT-3
app-shell creates a custom serial port http agent singleton
discovery client polls health for assigned hostname opentrons-usb using serial port http agent
for opentrons-usb hostname, app passes electron two-way IPC invoke requestor function to ApiHostProvider host context
api-client uses provided requestor function for http requests
app-shell handles IPC http requests, makes axios requests using serial port http agent

a handful of work left for this or follow-on PRs:

handle usb device removal correctly, so robot remains in unavailable list (currently it is removed entirely)
pass response status, ok via IPC
download robot logs (electron-dl) doesn't work with a custom agent - either inactivate for a usb connection or refactor how app-shell downloads
unit testing coverage
usb-bridge/node-client documentation
remove some cruft

Adds a silly node client that uses https://www.npmjs.com/package/agent-base to build an http agent that redirects everything through a node serialport stream https://serialport.io/docs/api-serialport including working through node-fetch (although right now you have to mess with the url). Many things to continue working on but this works shockingly well

makes a GET /instruments request from the app device details page via serial cable connection. facilitated via promise-based electron IPC communication and custom usb HTTP agent created in discovery-client

codecov · 2023-03-14T21:52:38Z

Codecov Report

Merging #12292 (c3db0de) into edge (fe13f95) will decrease coverage by 0.11%.
The diff coverage is 7.86%.

@@            Coverage Diff             @@
##             edge   #12292      +/-   ##
==========================================
- Coverage   73.26%   73.15%   -0.11%     
==========================================
  Files        1508     1488      -20     
  Lines       49453    48997     -456     
  Branches     3006     2903     -103     
==========================================
- Hits        36230    35843     -387     
+ Misses      12764    12698      -66     
+ Partials      459      456       -3

Flag	Coverage Δ
shared-data	`76.35% <ø> (+0.93%)`	⬆️
step-generation	`88.20% <ø> (ø)`
system-server	`96.07% <ø> (ø)`
update-server	`65.65% <ø> (ø)`
usb-bridge	`81.35% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
app-shell/src/buildroot/update.ts	`0.00% <0.00%> (ø)`
app-shell/src/http.ts	`25.71% <0.00%> (ø)`
app-shell/src/main.ts	`0.00% <ø> (ø)`
...alibration/useAllPipetteOffsetCalibrationsQuery.ts	`0.00% <0.00%> (ø)`
...rc/calibration/useAllTipLengthCalibrationsQuery.ts	`0.00% <0.00%> (ø)`
...lient/src/calibration/useCalibrationStatusQuery.ts	`0.00% <0.00%> (ø)`
...aintenance_runs/useCreateMaintenanceRunMutation.ts	`77.77% <0.00%> (-11.12%)`	⬇️
react-api-client/src/networking/useWifiQuery.ts	`0.00% <0.00%> (ø)`
...-client/src/protocols/useCreateProtocolMutation.ts	`76.92% <0.00%> (-7.70%)`	⬇️
react-api-client/src/robot/useSetLightsMutation.ts	`0.00% <0.00%> (ø)`
... and 6 more

... and 21 files with indirect coverage changes

sfoster1

oh man this is so cool

shlokamin

this is fkn awesome! we should do a group review with @sfoster1 to discuss next steps. seems reasonable to merge into the prototyping branch though

shlokamin · 2023-03-27T20:13:27Z

app-shell/package.json

@@ -59,6 +59,7 @@
 "node-stream-zip": "1.8.2",
 "pump": "3.0.0",
 "semver": "5.5.0",
+ "serialport": "^10.5.0",


we prolly wanna pin this to a fixed version

shlokamin · 2023-03-27T20:17:19Z

app-shell/src/discovery.ts

+ .getSerialPorts?.()
+ .find(port => port.productId === DEFAULT_PRODUCT_ID)
+
+ // TODO: handle null case properly


if we can't find a serial port we prolly wanna just bail and do nothing for now

shlokamin · 2023-03-27T20:24:21Z

discovery-client/package.json

+ "devDependencies": {
+ "@opentrons/usb-bridge/node-client": "link:../usb-bridge/node-client"
+ },


this should be a normal dep right?

shlokamin · 2023-03-27T20:29:25Z

discovery-client/src/health-poller.ts

+ * TODO: poll via serial port if connection present and combine in same object if same robot?
+ * will poll serial port for every robot


yeah that makes sense to me cc @sfoster1

shlokamin · 2023-03-27T20:31:00Z

discovery-client/src/store/selectors.ts

@@ -54,6 +55,11 @@ export const getRobots: (
 }
 )

+export const getSerialPorts: (state: State) => PortInfo[] = createSelector(
+ state => state.serialPorts,
+ serialPorts => serialPorts


can either get rid of line 60 or not use reselect since this is just pulling from state

shlokamin · 2023-03-27T20:35:54Z

discovery-client/src/store/reducer.ts

 robotsByName: robotsByNameReducer,
 hostsByIp: hostsByIpReducer,
 manualAddresses: manualAddressesReducer,
+ serialPorts: serialPortsReducer,


can we somehow roll serial ports into hostsByIp (we'd have to rename it to be something else), all of the info should be the same it would just be a serial port instad of an ip address. hostsByIp returns a HostsByIpMap interface:

export interface HostsByIpMap { [ipAddress: string]: HostState } export interface HostState extends Address { /** Whether this IP has been seen via mDNS or HTTP while the client has been running */ seen: boolean /** How the last GET /health responded (null if no response yet) */ healthStatus: HealthStatus | null /** How the last GET /server/update/health responded (null if no response yet) */ serverHealthStatus: HealthStatus | null /** Error status and response from /health if last request was not 200 */ healthError: HealthErrorResponse | null /** Error status and response from /server/update/health if last request was not 200 */ serverHealthError: HealthErrorResponse | null /** Robot that this IP points to */ robotName: string /** the robot model advertised in mdns, if known */ advertisedModel: string | null }

vegano1 · 2023-05-12T19:15:47Z

This is so cool!

… usb module

shlokamin

this is awesome! tested it out and seems to work pretty smoothly. excited to get this into the next internal release. great job @brenthagen!

shlokamin · 2023-05-15T17:13:51Z

discovery-client/src/store/selectors.ts

+ // prefer ip hostname
+ const isIpSort = isIp(b.ip) ? 1 : -1


shlokamin · 2023-05-15T17:16:59Z

package.json

@@ -33,6 +34,7 @@
 "@babel/eslint-parser": "^7.12.1",
 "@babel/plugin-proposal-class-properties": "^7.12.1",
 "@babel/plugin-transform-modules-commonjs": "^7.12.1",
+ "@babel/plugin-transform-typescript": "^7.14.5",


we can already use the types syntax — how come we need this plugin?

this error happens when trying to use the declare keyword:

[shell] Module build failed (from ../node_modules/babel-loader/lib/index.js): [shell] SyntaxError: /Users/brenthagen/opentrons/usb-bridge/node-client/src/usb-agent.ts: TypeScript 'declare' fields must first be transformed by @babel/plugin-transform-typescript. [shell] If you have already enabled that plugin (or '@babel/preset-typescript'), make sure that it runs before any plugin related to additional class features: [shell] - @babel/plugin-proposal-class-properties [shell] - @babel/plugin-proposal-private-methods [shell] - @babel/plugin-proposal-decorators [shell] 140 | } = { path: '' } [shell] 141 | [shell] > 142 | declare totalSocketCount: number [shell] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

so i inserted @babel/plugin-transform-typescript above @babel/plugin-proposal-class-properties in our babel config

shlokamin · 2023-05-15T17:27:06Z

usb-bridge/node-client/README.md

+## overview
+
+## api
+
+```
+
+```


mmm we should either fill these sections out or delete em

shlokamin · 2023-05-15T17:34:18Z

usb-bridge/node-client/bin/index.js

+#!/usr/bin/env node
+'use strict'
+require('../lib/cli')


not following why this is necessary. if our ts config is set up correctly (which i think it is?) we shouldn't have to do this

this is boilerplate i believe seth copied over from the discovery-client to enable cli requests. I left that in, do we want to take it out?

shlokamin · 2023-05-15T17:35:12Z

usb-bridge/node-client/package.json

+ "dependencies": {
+ "serialport": "10.5.0",
+ "yargs": "15.4.0",
+ "node-fetch": "^2.6.0",


we should probably pin this to a specific version to avoid surprises

seems reasonable to pin to 2.6.7 in app-shell, discovery-client, and here

shlokamin

this is awesome! tested it out and seems to work pretty smoothly. excited to get this into the next internal release. great job @brenthagen!

shlokamin · 2023-05-15T17:41:03Z

no idea how the last review got duplicated... and i cant seem to delete them

sfoster1 and others added 8 commits February 16, 2023 13:24

usb-bridge package boilerplate

94c482d

build usb http agent in discovery-client w/port hardcode

df55727

add serialport native node module to app-shell

889fd44

add async usb listener to app-shell

72e1695

add api-client request function that uses provided requestor function

c27c39a

add api-client, react-api-client instruments query boilerplate

1f84e69

feat(app,app-shell): usb request via app-shell POC

b4886fa

makes a GET /instruments request from the app device details page via serial cable connection. facilitated via promise-based electron IPC communication and custom usb HTTP agent created in discovery-client

brenthagen requested review from shlokamin and sfoster1 March 15, 2023 15:25

build http agent on usb detection WIP

5ee0b6a

sfoster1 reviewed Mar 16, 2023

View reviewed changes

brenthagen added 10 commits March 20, 2023 16:33

add/remove usb listener on usb device add/remove

ac242d7

destroy http agent on usb device removal

83ffc30

discovery client tsconfig boilerplate

c6aae9a

app shell request default to axios requestor

1aaaeaa

add fetchSerialPortList in usb-bridge

fbc6a2e

fix system-info action ts check

013fa0e

add discovery-client serial ports reducer, actions, selector

12352d1

fetchSerialPortList in discovery-client health poller

27a5ed7

create health poller with discovered serial port

5949ccc

create http agent with fetched serial port path

e0f1708

shlokamin approved these changes Mar 27, 2023

View reviewed changes

brenthagen added 6 commits April 10, 2023 16:58

WIP: poll health via usb

d4eb2fb

WIP: working subclassed http.Agent, shared by app and discovery-client

68575e1

WIP: make all usb react-query requests with app-shell requestor

86a3480

close serialport on finish

dc432a2

light refactor, remove excessive logging, add some typescript

f84fbf1

Merge branch 'edge' into app_app-shell-usb-request-POC

12080a4

brenthagen added 7 commits May 10, 2023 15:07

clean up constants and dependencies

02d04b1

add back empty tsconfig references

31bfa1e

Merge branch 'edge' into app_app-shell-usb-request-POC

8ff3892

prefer ip hostname in discovery-client address sort

87a02e9

retry if no serial port found on device connect

3de4534

remove unnecessary discovery-client diff

c2d404e

Merge branch 'edge' into app_app-shell-usb-request-POC

ce6aefa

brenthagen marked this pull request as ready for review May 12, 2023 19:05

brenthagen requested review from a team as code owners May 12, 2023 19:05

brenthagen requested review from b-cooper and removed request for a team May 12, 2023 19:05

brenthagen added 4 commits May 15, 2023 10:53

add usb requests start and stop actions

c9367cc

migrate app-shell usb request responsibility from discovery module to…

234442e

… usb module

redirect from device details to devices page when no robot found

4602f4d

Merge branch 'edge' into app_app-shell-usb-request-POC

8497258

shlokamin approved these changes May 15, 2023

View reviewed changes

brenthagen added 6 commits May 15, 2023 14:33

pin node-fetch to 2.6.7

9f0a405

pass dispatch to fetchSerialPortList retry

933a1f4

Merge branch 'edge' into app_app-shell-usb-request-POC

4032a42

add port opening condition to buildUSBAgent for cli

cb531f1

Merge branch 'edge' into app_app-shell-usb-request-POC

abcb4d5

optional chain usb rxjs response map

c3db0de

brenthagen merged commit f32f618 into edge May 15, 2023

brenthagen deleted the app_app-shell-usb-request-POC branch May 15, 2023 22:18

brenthagen mentioned this pull request Aug 25, 2023

fix(app,app-shell): download robot logs from from app not app-shell #13388

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(app,app-shell): usb request via app-shell #12292

feat(app,app-shell): usb request via app-shell #12292

brenthagen commented Mar 14, 2023 •

edited

Loading

codecov bot commented Mar 14, 2023 •

edited

Loading

sfoster1 left a comment

shlokamin left a comment

shlokamin Mar 27, 2023

shlokamin Mar 27, 2023

shlokamin Mar 27, 2023

shlokamin Mar 27, 2023

shlokamin Mar 27, 2023

shlokamin Mar 27, 2023

vegano1 commented May 12, 2023

shlokamin left a comment

shlokamin May 15, 2023

shlokamin May 15, 2023

brenthagen May 15, 2023

shlokamin May 15, 2023

shlokamin May 15, 2023

brenthagen May 15, 2023

shlokamin May 15, 2023

brenthagen May 15, 2023

shlokamin left a comment

shlokamin commented May 15, 2023

		* TODO: poll via serial port if connection present and combine in same object if same robot?
		* will poll serial port for every robot

feat(app,app-shell): usb request via app-shell #12292

feat(app,app-shell): usb request via app-shell #12292

Conversation

brenthagen commented Mar 14, 2023 • edited Loading

codecov bot commented Mar 14, 2023 • edited Loading

Codecov Report

sfoster1 left a comment

Choose a reason for hiding this comment

shlokamin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vegano1 commented May 12, 2023

shlokamin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlokamin left a comment

Choose a reason for hiding this comment

shlokamin commented May 15, 2023

brenthagen commented Mar 14, 2023 •

edited

Loading

codecov bot commented Mar 14, 2023 •

edited

Loading