Refactor to enable Driver commands for /flex, new Flex device support, testing via socat/VirtualDevice
#162
Conversation
This is step 1 in introducing Driver commands to Flex: simply extracting the common (i.e. literally mostly identical) websocket handling code and encapsulating the device specific commands into a DeviceBackend interface. Adding some TODOs for trickier areas. Purposefully not introducing any new structure to keep the diff readable/minimal. At this stage this should be backwards compatible: Driver command handling is unchanged for the Senso, while Flex can now receive and respond to some of them, but keeps the backwards-compatible behaviour of auto-connecting on first websocket connection.
This adds proper Discover/Connect/Disconnect command handling to the Flex backend, while keeping the /flex endpoint backwards compatible with "auto-connect" behaviour. The auto-connect backwards compatibility is useful both for a seamless transition in Play and for allowing tools like `record-flex` to continue to "just work". By default, the Flex backend auto-connects to the first Flex-like serial device when a new WS connection is opened, as before. However, the client can pass the header `manual-connect: 1`, to indicate that it will set up the connection via Driver commands itself. If a previous client has already set up the connection, the header has no effect. Implementation change: this removes the 2 second sleep after serial connection closure (e.g. due to error). Blocking the thread seems like a strange way to implement backoff?
The fork provides extra USB device details on Linux. The go version bump is needed for basic generics support. The rest is due to `go mod tidy`
This extends the Driver to: - Support Flex device metadata in Driver message replies - Enable broadcasting messages to all connected clients Using it, the Flex backend implements a flow where it auto-connects to devices and informs about device changes by broadcasting a Status update. This eliminates the need for the client to periodically poll the Driver. The protocol changes are backwards compatible for the /senso endpoint.
- Handle port closure in the same context where it is opened - Move context cancelation to the caller - Remove explicit reader cancelation - it will automatically canceled when parent context is canceled
Even if device connection is not established, we still need to cancel the auto-connect if there are no subscribers remaining. Note: Disconnect already internally checks if `cancelCurrentConnection != nil`
Bumping node_modules/ws to be able to identify binary messages.
Also split up tests for checking different aspects.
The strace recording approach is a bit hacky, but the alternatives are even messier. If we want to both interact and observe, we have two choices: - multiplex the serial stream before it reaches the driver. This would mean circumventing the serial enumeration, registering a mock device just to record it, etc. Messy. - multiplex the serial stream once it reaches the driver, i.e. expose yet another binary stream from the driver. Complex and potentially dangerous code to have in production. Philosophical digression: with the new Flex protocol, Driver really becomes just a glorified chunker + WS<->USB/serial proxy. TODO: store metadata or at least a file suffix convention (, .v4.ws.dat, .v5.serial.dat) to help distinguish what type of data was recorded. Disclaimer: partially vibe-coded
This modifies the protocol to serialize DeviceInfo in both Status and Discovered messages in an identical way. This simplifies the parsing logic for the clients, since they can: 1) Rely on the `deviceType` field to determine what fields are present 2) Re-use the same parsing logic in Status and Discovered messages To prevent incorrectly constructed DeviceInfo structs, all the fields are made private and helper functions are provided for initialization. To maintain backwards compatibility, Senso device data is duplicated in the top-level fields in Status messages. Since this data is used at a low-volume, this should not pose any problems.
This ensures that devices returned by ListMatchingDevices() are all supported devices that we know how to connect to. Previously, the enumerator would only check the Teensy Vendor ID. Also adjusted the tests and extended to verify unknown devices are not Discovered. Note: purposefully not providing DeviceFamily to the client, since it would break the "passthru" concealment.
|
Off-topic: the "CORS and PNA test" is flaky: https://github.com/dividat/driver/actions/runs/21350862315/attempts/1 I think I've seen it fail a long time ago too. |
|
@knuton addressed all review comments as of now. Let me know if you want me to patch Play side for testing. I'd prefer to wait for feedback, finalize here, then return to Play side. |
knuton
left a comment
knuton
left a comment
There was a problem hiding this comment.
Very good!
I confirmed compatibility with an unaltered main-branch version of Play, including discovery, commands, firmware update.
I was also able to replay v5 recordings via passthru with the same version of Play.
Has flex replay also been tested on macOS?
test/flex/mock/VirtualDevice.js
Outdated
| @@ -0,0 +1,143 @@ | |||
| // Disclaimer: this is 90% vibe-coded | |||
There was a problem hiding this comment.
OK, I agree that testing tools are under different constraints than the production code.
I am not sure what to practically make of the comment going forward -- is it somehow like generated code, do I need to keep vibing when I want to edit it? Probably not? Origin is good to document (commit message?), but from now on it's just code.
There was a problem hiding this comment.
We don't really have codified JS code style in this repo, and you are adding a much more recent dialect than most of the existing files are written in (which is fine).
But maybe just drop semicolons, which is most obvious stylistic difference?
There was a problem hiding this comment.
Dropping the semicolons actually breaks a bunch of tests in mysterious ways. Is it really a good idea to default to no semicolons?
There was a problem hiding this comment.
Well, it's very possible and nowadays I think the norm to write JS that way ;-)
It's a bit unclear to me how things break, and I don't think it should be too hard. But I am OK with merging as is and standardizing JS in this repo separately.
knuton
added
changes requested
No yet, should be done. Is |
Dropping the `sensitronics` alias for the `v6`, but keeping the possibility to identify the prototype devices when Manufacturer == Sensitronics.
Since each DeviceBackend uses a separate broker instances, there is no need to use globally unique names.
This ensures that samples are produced with the same delays they were recorded (previously it was shifted-by-one).
Dev builds use a `debug` tag by default. When the build is tagged with `debug`, a functional MockDeviceRegistry is created. Otherwise, a noop stub is used.
I changed CI to run the tests on macOS and they seem to pass for Flex, but one Senso test fails (or is flaky): EDIT: it's the "Can discover mock Senso" test, so seems to be related to mDNS/bonjour, could be CI specific. Removing the CI change for now, we can add it / fix it in a separate PR. EDIT 2: confirmed by @terezka to work on her dev macOS machine locally, so it seems to be a CI / GH runner thing. |
knuton
removed
the
changes requested
2 participants
This PR materializes the idea of unifying Senso and Flex backends to have the same Driver command protocol, adds support for the new Sensitronics protocol/devices and revamps the testing workflow by introducing mocked serial devices (using socat).
Added functionality:
/flexendpoint WS clients can issue the same Driver commands as for the/sensobackend. All commands except for firmware update are supported.websocket/command.go) is updated to optionally include USB device metadata inDeviceInfoJSONs (in addition to discovered mDNS service metadata).Broadcastmessages to all connected endpoint clients. Currently/flexendpoint uses this to inform clients about change in device connectivity status. This eliminates the need to poll for changes from the client side. Not used in/senso./flexclient can now indicate it wants to manually control the device connection by passing an WS subprotocolmanual-connect. This disables automatic scan+connect, requiring the client to manually issueDiscoverandConnectcommands. Not used in Play, so can be thrown out, but this is a parity check that we can control Flex and Senso devices "in the same way" from Play.tools/record-flex-serial(relies on strace, so Linux-only)-test-modeflag), which enables vitrual/mock serial device registration via aPOST /flex/mockrequest (used in tests and below)tools/replay-flexnow uses the Driver for replays and can replay both raw serial data (emulating a serial device via socat) and WS binary data (by asking the Driver to run with a "passthru" reader, see below)/flexendpointChanges:
go-serialis updated to a forked version that enables extra USB descriptors:bcdDevice,productandmanufacturer. Tested to work on Linux and macOS. Windows support blindly implemented, not tested./flexdevice at onceNot implemented, but considered (sorted by most->least important):
deviceType=Flex, gen=v5) and so clients have to roll their own device identification logic. Mixed feelings about this. On the one hand, this is aligned with my (future) vision of the driver as merely a "dumb" chunking serial<->WS proxy, so that device firmware/protocol changes do not necessitate driver updates (e.g. if we add a new message type, the driver does not care). On the other hand, this requires duplicating the (USB -> device type/gen) identification logic in the client.replay-flexdoes not know what device to emulate, it has to be specified manually, which can be quite error proneudevorfsnotifyon Linux at least, instead of manual sleep+polling. TBD in a separate PR.Discovercommands remains awkwardly asynchronous in/flex(same as in in/senso) - the client has no way of knowing when the discovery is "finished", it can merely wait for the specified timeout to happen.Potential issues:
go-serialis untested, as mentioned aboveChanges should be 100% backwards compatible for both
/sensoand/flexclients, i.e. Play should be able to run with this Driver "as is". Older Play versions will simply not be able to use Driver commands/events for Flex or understand the Sensitronics protocol.Commits unsquashed, since they contain explanations about some of the choices. History is messy after the initial ~5 commits,so probably best to squash before merging.
Apologies for the huge diff.
Related PRs/code
/flexdevice metadata: https://github.com/yfyf/diviapps/tree/flex-discovery (fully working, but not yet rebased / cleaned up).Checklist
manufacturer == Sensitronics, which is maybe good enough, but need to set it in stone + document.