Revert or fix broken webserial support

[alexrudd2]

@jedahan said:

By manually reverting the webserial changes, I got it to work correctly.

I'm conflicted about this.
On the one hand, multiple people have reported problems with the webserial implementation. (I removed it myself and found things improved!) It's clearly problematic.
On the other, nornagon explicitly doesn't want to revert it. I've been hoping to keep as as little of a diff with him as possible to upstream our changes.

I'm leaning towards revert it so we can iterate on a working system. When nornagon is available again, we can try to upstream what's relevant. I don't want a hard fork.

[jedahan]

I agree - revert for now, so that we actually have working software, then figure out what is needed to upstream once possible. Mozilla's current position is that WebSerial in its current form is harmful (discussion), though there are addons that add support which I have not tested.

Since we already were building separate bundles with IS_WEB, I think to keep the most amount of current work relevant, we would need to split out more from the critical path.

[jedahan]

soooo... i havent tested webserial support with the latest changes, but I can confirm that my brushless axidraw is happy plotting with #94 . So maybe we don't need to revert?

[jedahan]

I am wondering if the node serial upgrade did the trick

[alexrudd2]

I am wondering if the node serial upgrade did the trick

Possibly serialport/bindings-cpp#115?

[jedahan]

Hard to tell - I would expect with that bug to not be able to connect at all, but the behavior I got was lots of bad moves, then a few normal moves plotting, then a bunch of bad moves.

Bad move = pen would just wiggle < 2mm movement and servo would sing different pitches. Now it seems smooth.

[jedahan]

And its back :(

[alexrudd2]

Copying from another issue:

See how there are 5 redundant websocket (NOT webserial) messages at once. I've seen this for months and never been able to figure it out

These lines look suspicious to me, as connect() is called multiple times:
https://github.com/alexrudd2/saxi/blob/main/src/ui.tsx#L211C1-L216C4

[alexrudd2]

OK, I'm getting closer. I think the problem is React hooks are instancing Driver multiple times.

Namely, the useEffect() calls that add event handlers are causing React to call Root() again and therefore re-initialize the Driver. I made them only fire once, which helps but didn't solve everything.

Another solution is to wrap the driver connection code with a flag so only one attempt is made.

[jedahan]

Given that we have a separate IS_WEB(serial) build, I wonder if we can avoid having to attach the handlers in an useEffect hook. The rerenders should really only happen when needed.

[jedahan]

I also started investigating https://github.com/robtaussig/react-use-websocket#example-implementation to see if it handles certain things better.

For example, our server uses the entire WebSocket object for identity, so I am unsure if it actually is filtering out when a tab is closed. We should generate an ID client-side and just pass that in as a query parameter, so we can gracefully remove connections that have been terminated.

[alexrudd2]

Given that we have a separate IS_WEB(serial) build, I wonder if we can avoid having to attach the handlers in an useEffect hook. The rerenders should really only happen when needed.

Why don't you give #113 a try? It works for me. (EDIT: Well, it runs for me and appears to eliminate the redundant EBB connections. I can't reproduce the crashes you experience)

I agree with your suggestion that it's possible to avoid useEffect altogether. However, I think the intention was to allow for switching devices on the fly?

[alexrudd2]

I agree with your suggestion that it's possible to avoid useEffect altogether. However, I think the intention was to allow for switching devices on the fly?

OK so the useEffect for device is needed to handle connect/disconnect although I'm not sure how it works.
The useEffect for the clipboard event handlers appears to be only used for teardown.

[alexrudd2]

Closed by #113.

There may be further improvements possible, which I split out into #121