Skip to content

Conversation

@yfyf
Copy link
Contributor

@yfyf yfyf commented Jan 13, 2026

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:

  • /flex endpoint WS clients can issue the same Driver commands as for the /senso backend. All commands except for firmware update are supported.
  • the Driver command protocol (see websocket/command.go) is updated to optionally include USB device metadata in DeviceInfo JSONs (in addition to discovered mDNS service metadata).
  • the Driver gains support for sending Broadcast messages to all connected endpoint clients. Currently /flex endpoint 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.
  • A /flex client can now indicate it wants to manually control the device connection by passing an WS subprotocol manual-connect. This disables automatic scan+connect, requiring the client to manually issue Discover and Connect commands. 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.
  • Raw device serial data can now be recorded using tools/record-flex-serial (relies on strace, so Linux-only)
  • Driver can now be run in test mode (-test-mode flag), which enables vitrual/mock serial device registration via a POST /flex/mock request (used in tests and below)
  • tools/replay-flex now 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)
  • Driver has two new Flex device (serial) readers: Sensitronics (for the new devices) and a special Passthru reader (for various dev/testing purposes)
  • Proper unit tests for the /flex endpoint
  • Added raw serial and WS data recordings for all current Flex devices

Changes:

  • go-serial is updated to a forked version that enables extra USB descriptors: bcdDevice, product and manufacturer. Tested to work on Linux and macOS. Windows support blindly implemented, not tested.
  • Driver used to (implicitly) sleep for 2 seconds on serial device disconnect/error. This is now removed, if a client aggressively attempts to reconnect without any backoff, so will the driver.
  • Avoid a temporary disconnect due to a race condition when two clients connect to the same /flex device at once

Not implemented, but considered (sorted by most->least important):

  • Driver does not send "derived" device metadata (e.g. 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.
  • As before, Driver does not attempt to distinguish between Flex v4 and v5 and relies on client commands to switch between 8bit vs 12bit. This logic could be moved to the Driver in a backwards compatible way.
  • Recordings have no metadata, so replay-flex does not know what device to emulate, it has to be specified manually, which can be quite error prone
  • Recordings do not capture commands sent to the device (hence there's no way to know how the device was configured during the recording)
  • Would be good to replace USB device detection with udev or fsnotify on Linux at least, instead of manual sleep+polling. TBD in a separate PR.
  • the Discover commands 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.
  • the Driver command protocol could be revised further and the implementation generalized/refactored even more, but this is PR is already too big.

Potential issues:

  • Windows support for the extra USB descriptors in go-serial is untested, as mentioned above
  • No way to record raw serial data in macOS, diminished dev experience. I see no easy/good way to achieve this, see Readme and 1d5cdf4 for extra details. Should not be a practical blocker, since for Sensitronics serial data ~= WS binary data.
  • Variance in sample timing in the serial recordings and replays, due to strace and more layers being involved. This can probably be adjusted by chunking the recording instead of replaying every read. I would say it's not a problem right now, since we don't rely on precise timing during replays.

Changes should be 100% backwards compatible for both /senso and /flex clients, 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

Checklist

  • Changelog updated
  • Code documented
  • Decide on a way to identify Flex/Sensitronics devices going forward. Currently Driver checks manufacturer == Sensitronics, which is maybe good enough, but need to set it in stone + document.

yfyf added 30 commits January 12, 2026 13:31
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
@yfyf yfyf requested a review from knuton January 13, 2026 13:10
@yfyf yfyf added the reviewable Ready for initial or iterative review. label Jan 13, 2026
@socket-security
Copy link

socket-security bot commented Jan 13, 2026

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addednpm/​commander@​12.1.09810010084100
Updatednpm/​ws@​5.2.4 ⏵ 8.18.39910010090100

View full report

@yfyf
Copy link
Contributor Author

yfyf commented Jan 14, 2026

I removed all the Flex recordings for now to avoid repo/diff bloat. We can either:

  1. Keep the recording files in the repo, but introduce .gitattributes marking .dat and .rec files as binary, so that git(hub) does not attempt to text-diff
  2. Move them to a separate repo and add it as a git submodule
  3. Move them to some cloud storage, e.g. GDrive

I vote option 1 or 2. Option 3 is inconvenient, because you can't easily deal with the files while developing.

@yfyf
Copy link
Contributor Author

yfyf commented Jan 14, 2026

Added .gitattributes and explicitly reverted the commits that modify / add recordings.

@knuton
Copy link
Member

knuton commented Jan 14, 2026

  • go-serial is updated to a forked version that enables extra USB descriptors: bcdDevice, product and manufacturer. Tested to work on Linux and macOS. Windows support blindly implemented, not tested.

I browsed the upstream repo, and while it's maintained, it seems the maintainer is burnt out on/uninterested in bringing back the metadata. What about the sysfs-ignore, is that worth trying to upstream or a hack? If we stick with the fork, shall we move it to the dividat org?

@@ -0,0 +1,143 @@
// Disclaimer: this is 90% vibe-coded
Copy link
Member

Choose a reason for hiding this comment

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

It's your code and I will review it as your code. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not trying to disown it, just hinting at the fact that since VirtualDevice and VirtualSerialPort modules are purely testing tools, it does matter much how they are implemented, as long as they satisfy the testing needs and do not have strange loopholes.

@yfyf
Copy link
Contributor Author

yfyf commented Jan 15, 2026

I browsed the upstream repo, and while it's maintained, it seems the maintainer is burnt out on/uninterested in bringing back the metadata. What about the sysfs-ignore, is that worth trying to upstream or a hack? If we stick with the fork, shall we move it to the dividat org?

Yeah, my impression is also that any extra development is "on hold" in go-serial.

The Driver code and tests no longer rely on the sysfs-ignore actually, that is a left-over from an earlier approach where I was trying to create a "real" serial device that could be discovered by go-serial/enumerator. I have abandoned this path in favor of explicitly registered test devices, because it was messy and you still had to pass all the device metadata via a side-channel.

I'll move the fork to dividat org and remove the "sysfs-ignore" commit for clarity.

@yfyf
Copy link
Contributor Author

yfyf commented Jan 19, 2026

Changed to dividat fork, but will need a version/ref bump once dividat/go-serial#1 is merged.

EDIT: bumped.

Intention is to drop Windows support entirely, TBD in a separate PR.

Copy link
Member

@knuton knuton left a comment

Choose a reason for hiding this comment

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

Very good work, still working through it, but submitting some feedback mostly about code structure.

Will perform manual testing with physical Senso next.

} else if message.Discovered != nil {
serviceEntry := message.Discovered.TcpDeviceInfo
msg := struct {
Type string `json:"type"`
Copy link
Member

Choose a reason for hiding this comment

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

Should we consider adding an additional device tag field that explicitly encodes which kind of device was discovered?

It took me a while to understand what's happening here.

Maybe branch on which of TCP/USB is defined and don't "mix" definitions, even if a little more verbose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 for adding the extra device tag that the client can use to simplify parsing.

But not entirely sure how you propose to branch/not "mix", while still being backwards compatible.

Currently Play receives Discover replies (for the Senso) that look like:

{ "type: "Discovered", "service": <TcpDeviceInfo>, "ip": [ "1.1.1.1", ... ] } 

do you propose to re-use the service field, i.e.:

{ "type: "Discovered", "device": "senso", "service": <TcpDeviceInfo>, "ip": [ "1.1.1.1", ... ] } 
{ "type: "Discovered", "device": "flex", "service": <UsbDeviceInfo> }

This would still "mix" things, as e.g. the IP is present for Senso, but not Flex devices.

Or do you mean moving service/ip to a sub-field, i.e. making things not backwards compatible? I really wanted to avoid having to make any kind of changes to the Senso part in Play to minimize the surface. I think it's better to tackle these rougher corners if/once we attempt to fully unify the device backends.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking to keep the format backwards compatible and flat as you made it, but use the device tag to make explicit what it should be parsed as.

  • No device tag: Implicitly parse only for TCP
  • device = "senso": Parse only for TCP
  • device = "flex" (or device = "serial"?): Parse only for USB

If there are superfluous fields for a given device tag, they can simply be ignored (this is the default behaviour in most JSON parsing).

Copy link
Contributor Author

@yfyf yfyf Jan 23, 2026

Choose a reason for hiding this comment

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

I went a slightly different way and duplicated the DeviceInfo into a separate device field of the Status reply. Otherwise, it would mean we have two different serialization paths and hence two different parsing paths for Status vs Discovered messages. Also enforced safe/correct creation of DeviceInfo for the different device types.

Re naming: went for flex instead of serial, since otherwise it's a strange taxonomical mix in the enum ("senso" and "serial").

More details in the commit: ec74088

Copy link
Contributor Author

Choose a reason for hiding this comment

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

P.S. This breaks the PoC implementation in Play, will fix it after I'm done with other parts.

Copy link
Member

Choose a reason for hiding this comment

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

Nit: "util" is underselling the importance of these modules, and util/websocket is very vague. "device"/"devices" or "protocol"/"client protocol"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, was thinking about this as well. Maybe transport/websocket? Or.. just websocket/?

Copy link
Member

Choose a reason for hiding this comment

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

Something emblematic of why I think websocket is not a very helpful name is code like this:

func (backend *DeviceBackend) GetStatus() websocket.Status {
	return websocket.Status{
		Address: backend.address,
	}
}

This is really not a WebSocket status at all, and also not a WebSocket address. So one needs to override intuitive reading with background knowledge completely in this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. I think the protocol part deserves it's own namespace, so maybe it can even become two modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Converted websocket/command.go to a protocol module (8e31859) and moved websocket to the top-level (2eab988). At first I did transport/websocket, but I don't think it makes much sense to have another layer since 1) there's only one transport currently; 2) the module does more than just handle transport; and 3) the top-level is anyway messy with modules that should not probably be top-level (e.g. service, logging and firmware).

Comment on lines +172 to +174
reader := deviceToReader(device)
// should not happen
if reader == nil {
Copy link
Member

Choose a reason for hiding this comment

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

I might have missed something, but to me it looks like the enumerator only gave us a list of all Teensys, so this could very well happen?

Could we "parse, don't validate" and look up the reader earlier when checking for supported devices?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good point. Will maybe just move the reader-choice closer to enumeration, but need to think a bit.

yfyf added 9 commits January 23, 2026 10:45
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

reviewable Ready for initial or iterative review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants