Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent individual writes from being broken up by implementation #123

Open
marciot opened this issue Feb 24, 2021 · 6 comments
Open

Prevent individual writes from being broken up by implementation #123

marciot opened this issue Feb 24, 2021 · 6 comments

Comments

@marciot
Copy link

marciot commented Feb 24, 2021

Prior to my use of the Web Serial API, I had used the Electron framework to write some code that would flash firmware the Atmel SAM microcontroller. I recently modified this code to work with the Web Serial API and I found that it failed in unexpected ways. Using a serial snooping tool, I found that the exact same bytes were being written to the device, yet the code using Web Serial API would fail to write certain pages, while the Electron Serial Library always succeeded.

To make a long story short, I found out that the bootloader on those boards requires that once you initiate writing a page, all 256 bytes must be delivered contiguously and uninterrupted, otherwise the request fails. This is possibly a timeout feature to prevent the board from locking up on incomplete commands.

I found out that in the Chromium implementation, the failures were related to the "bufferSize" option to the Web Serial API "open()" method. By counting how many bytes were written to the serial port, I could exactly predict when a command would fail. I suspect Chromium is using the buffer as a circular queue and will break up a write into two when it straddles that boundary.

I understand that this is an implementation specific concern, and I have reported it as such:

https://bugs.chromium.org/p/chromium/issues/detail?id=1181852

However, technically there isn't anything wrong with their implementation unless something in the specifications recommends that writes not be broken up, so I am submitting it here for consideration.

I can only imagine it would save others some grief, as this was a very tricky problem to track down.

@reillyeon
Copy link
Collaborator

This is another interesting question for @domenic to ponder w.r.t. the Streams API. I'm not sure it is explicitly stated by the specification but for generic streams chunks are atomic, while for byte streams it seems like chunks could be split or merged arbitrarily by the implementation. What are your thoughts here?

I've added some additional notes on the implementation details to the Chromium issue. I personally think the bootloader is making some sketchy assumptions about how USB serial drivers work but they are assumptions that other APIs don't break.

@marciot
Copy link
Author

marciot commented Feb 25, 2021

I personally think the bootloader is making some sketchy assumptions about how USB serial drivers work but they are assumptions that other APIs don't break.

Yes. I've come across much sketchiness in this line of work :)

I don't think Atmel intended anyone to write tools to flash to their chips. There are no public documents on the protocol except the handful of people who have reverse engineered it using serial sniffers. What really got me about this issue is that two implementations that generated the exact same byte stream were giving different results -- it was quite maddening!

Right now I have solved the problem by reading back the data for each block from the chip prior to issuing the command to save it to flash. It's a perfectly acceptable solution that fixes the issue, but it is a bit slower than before.

@domenic
Copy link
Collaborator

domenic commented Feb 26, 2021

This is another interesting question for @domenic to ponder w.r.t. the Streams API. I'm not sure it is explicitly stated by the specification but for generic streams chunks are atomic, while for byte streams it seems like chunks could be split or merged arbitrarily by the implementation. What are your thoughts here?

From the streams spec perspective, chunks are atomic all the way through... until they get to the underlying source logic. The underlying source logic then gets an atomic chunk, and can decide what to do with it---which might include breaking it up, or buffering it, or whatever.

Generally on the web once things disappear into the underlying source we've been splitting them up a good bit. E.g. when a response or request body stream crosses a service worker boundary, the chunks could get split or combined arbitrarily.

I see the serial spec's underlying source is already pretty clear on this; it says

Invoke the operating system to write bytes to the port. Alternately, store the chunk for future coalescing.

although I guess it isn't clear here that chunks could be split, just that they could be combined.

@reillyeon
Copy link
Collaborator

I could see us rewriting the write steps so that the underlying sink makes a best-effort attempt to avoid splitting chunks. The only case in which that is inevitable is if the underlying operating system only accepts a partial chunk and we have to retry with the remainder.

I need to think through the performance and memory usage consequences.

@marciot
Copy link
Author

marciot commented Feb 26, 2021

although I guess it isn't clear here that chunks could be split, just that they could be combined.

@reillyeon: I'll add that the Atmel bootloader implementation is so broken that even combining chunks is forbidden. I tried sending the command string and data payload as one and it rejected the command as well. It requires a single USB transaction for the command and a single USB transaction for the data. No combining or splitting allowed.

The original implementation issued a "flush" the command string and another after the data payload. On your suggestion, I am closing Writer stream and getting another at each flush. This is working beautifully.

@reillyeon
Copy link
Collaborator

reillyeon commented Feb 26, 2021

Having had some time to mull over the performance implications of not coalescing chunks I think the biggest issue comes from the WritableStream issue mentioned in the specification which is why we explicitly allows coalescing in the first place: There is no way for the underlying sink to know that there are multiple chunks queued because the specification requires the Promise returned by the previous call to write() to resolve before the next call is made. Without this restriction multiple parallel calls to write() could be in flight at once, something the underlying operating system can handle perfectly fine. With only a single call allowed at a time there would be significant delay added by the round-trip time required to notify the stream that the previous write completed so that the next can be dispatched.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants