Some kind of serial buffering fix #212
Conversation
|
IMO the alternative to doing it this 'stupid' way is to do something like what's in https://github.com/micropython/micropython/blob/d68e3b03b1053a6de0c7eb28f5989132c138364b/ports/samd/mphalport.c |
| if (actual_length) { | ||
| memcpy(ptr, buf, min(len, actual_length)); | ||
| return actual_length; | ||
|
|
There was a problem hiding this comment.
This avoids the following issues:
- calling read more than once before cdc_task happened would give us the same data
- not calling read before cdc_task happened would silently drop data
- inability to read '\0'
| #else | ||
| read(0, line, 256); | ||
| char *read_loc = &serial_buffer[serial_buffer_index]; |
There was a problem hiding this comment.
This elaborate buffering dance handles arbitrary amounts of input at a time and echos the input immediately. I've been successfully using:
picocom /dev/ttyACM0 --imap lfcrlf
wryun
force-pushed
the
serial-buffer-fix
branch
2 times, most recently
from
7dc893d
to
caa1cba
Compare
| tud_cdc_n_write_flush(0); | ||
| return len; | ||
|
|
||
| while (hri_usbdevice_get_CTRLA_ENABLE_bit(USB) && written == 0) { |
There was a problem hiding this comment.
Previously, if you tried to write something large it would write whatever it could (tud_cdc_n_write) and then claim it had written the whole buffer. Now we return how much we wrote.
Unfortunately, https://github.com/bminor/newlib/blob/5192d5ea51dee786ba1cb97b216bd40115dbbf20/newlib/libc/stdio/fflush.c#L225 demands that we actually write something otherwise it's an error, hence there's a busy loop here waiting for the usb buffer to clear. i.e. it's a blocking operation. This is presumably horribly inefficient, but I'm not sure how to do it the right way, or clear on what the right way is (a blocking call that doesn't spin, or an internal buffer that we periodically try to clear? But I guess the internal buffer only postpones the problem).
There was a problem hiding this comment.
So, experimenting with do both USB CDC and mass storage at the same time, this blocking operation causes us to freeze up if we spam debug statements in the USB mass storage callbacks (probably because we're already busy with mass storage and can't write CDC things at the same time).
Not sure whether the lesson is 'don't do silly things in callbacks' or 'we need an intermediate buffer here' or ...
I don't really understand what's going on here, but this works better than it did before.
wryun
force-pushed
the
serial-buffer-fix
branch
from
caa1cba
to
524bf5c
Compare
| // away any data in the buffer. | ||
| // Note that it's fine not to write all the bytes, it's just | ||
| // that we have to write _something_. | ||
| while (tud_cdc_connected() && written == 0) { |
There was a problem hiding this comment.
I'm now wondering if it's a reasonable approach to just try to do this for some amount of time then fail to avoid deadlocks. Of course, I could put an intermediate buffer in, but tiny USB already has a buffer here (which we could increase...).
|
@WesleyAC - I realise this is not right, but I think it's better than the current situation (where the serial is IMO close to unusable). I'd moved it out of draft in hope :) Because serial is only on when you have it plugged in and not in the watch casing, I think the possible downsides are limited. |
|
Closed since #344 fixes this properly. |
Addresses #117
Removes the intermediate buffer entirely and make movement.c responsible for calling read. I'm not sure if anything bad happens if you don't call tud_cdc_n_read, and this PR has problems with the initial display of '$' that I'm not sure how to work around well.
I'm raising it as a draft for comment since I lack confidence in its sanity.