Fix how USB queue overflow is handled in chibios. (#12576)0.12.39

* Fix how USB queue overflow is handled in chibios.

This commit reverts PR 12472 (commit c823fe2d3f23ed090e36ce39beed4c448298bd2f),
and it implements the original intent of the commit in a better way.
The original intent of the above mentioned commit was to not deadlock the
keyboard when console is enabled, and hid_listen is not started.

The above mentioned commit had a few drawbacks:
1) When a lot of data was printed to the console, the queue would get full,
and drop data, even if hid_listen was running. (For example having matrix debug
enabled just didn't work right at all)
2) I believe the function in which this was implemented is used by all other
USB endpoints, so with the above change, overflow, and data loss could
happen in other important functions of QMK as well.

This commit implements deadlock prevention in a slightly similar way to how
it's done on AVR. There is an additional static local variable, that memorizes
whether the console has timeouted before. If we are in the timeouted=false
state, then we send the character normally with a 5ms timeout. If it does
time out, then hid_listen is likely not running, and future characters should
not be sent with a timeout, but those characters should still be sent if there
is space in the queue. The difference between the AVR implementation and this
one is that the AVR implementation checks the queue state directly, but this
implementation instead attempts to write the character with a zero timeout.
If it fails, then we remain in the timeouted=true state, if it succeeds, then
hid_listen started removing data from the queue, so we can go out of the
timeouted=true state.

* Added comment explaining the timeouted logic to console flow control.

* Console flow control: refactor chibios flowcontrol code to make it more readable, and rename the timeouted variable to timed_out on both chibios and lufa. Changed comments to says timed_out is an approximation of listener_disconnected, to make it clear that it's not the same thing

* fix typo

3 files changed, 33 insertions, 21 deletions

diff --git a/tmk_core/protocol/chibios/usb_driver.c b/tmk_core/protocol/chibios/usb_driver.c
index 40bfb8eb2b..cc0ce7600f 100644
--- a/tmk_core/protocol/chibios/usb_driver.c
+++ b/tmk_core/protocol/chibios/usb_driver.c
@@ -80,19 +80,7 @@ static bool qmkusb_start_receive(QMKUSBDriver *qmkusbp) {
  * Interface implementation.
  */
 
-static size_t _write(void *ip, const uint8_t *bp, size_t n) {
-    output_buffers_queue_t *obqueue = &((QMKUSBDriver *)ip)->obqueue;
-    chSysLock();
-    const bool full = obqIsFullI(obqueue);
-    chSysUnlock();
-    if (full || bqIsSuspendedX(obqueue)) {
-        /* Discard any writes while the queue is suspended or full, i.e. the hidraw
-           interface is not open. If we tried to send with an infinite timeout, we
-           would deadlock the keyboard otherwise. */
-        return -1;
-    }
-    return obqWriteTimeout(obqueue, bp, n, TIME_INFINITE);
-}
+static size_t _write(void *ip, const uint8_t *bp, size_t n) { return obqWriteTimeout(&((QMKUSBDriver *)ip)->obqueue, bp, n, TIME_INFINITE); }
 
 static size_t _read(void *ip, uint8_t *bp, size_t n) { return ibqReadTimeout(&((QMKUSBDriver *)ip)->ibqueue, bp, n, TIME_INFINITE); }
 
diff --git a/tmk_core/protocol/chibios/usb_main.c b/tmk_core/protocol/chibios/usb_main.c
index 8adecfa719..0703cdc718 100644
--- a/tmk_core/protocol/chibios/usb_main.c
+++ b/tmk_core/protocol/chibios/usb_main.c
@@ -930,9 +930,32 @@ void send_consumer(uint16_t data) {
 #ifdef CONSOLE_ENABLE
 
 int8_t sendchar(uint8_t c) {
-    // The previous implmentation had timeouts, but I think it's better to just slow down
-    // and make sure that everything is transferred, rather than dropping stuff
-    return chnWrite(&drivers.console_driver.driver, &c, 1);
+    static bool timed_out = false;
+    /* The `timed_out` state is an approximation of the ideal `is_listener_disconnected?` state.
+     *
+     * When a 5ms timeout write has timed out, hid_listen is most likely not running, or not
+     * listening to this keyboard, so we go into the timed_out state. In this state we assume
+     * that hid_listen is most likely not gonna be connected to us any time soon, so it would
+     * be wasteful to write follow-up characters with a 5ms timeout, it would all add up and
+     * unncecessarily slow down the firmware. However instead of just dropping the characters,
+     * we write them with a TIME_IMMEDIATE timeout, which is a zero timeout,
+     * and this will succeed only if hid_listen gets connected again. When a write with
+     * TIME_IMMEDIATE timeout succeeds, we know that hid_listen is listening to us again, and
+     * we can go back to the timed_out = false state, and following writes will be executed
+     * with a 5ms timeout. The reason we don't just send all characters with the TIME_IMMEDIATE
+     * timeout is that this could cause bytes to be lost even if hid_listen is running, if there
+     * is a lot of data being sent over the console.
+     *
+     * This logic will work correctly as long as hid_listen is able to receive at least 200
+     * bytes per second. On a heavily overloaded machine that's so overloaded that it's
+     * unusable, and constantly swapping, hid_listen might have trouble receiving 200 bytes per
+     * second, so some bytes might be lost on the console.
+     */
+
+    const sysinterval_t timeout = timed_out ? TIME_IMMEDIATE : TIME_MS2I(5);
+    const size_t result = chnWriteTimeout(&drivers.console_driver.driver, &c, 1, timeout);
+    timed_out = (result == 0);
+    return result;
 }
 
 // Just a dummy function for now, this could be exposed as a weak function
diff --git a/tmk_core/protocol/lufa/lufa.c b/tmk_core/protocol/lufa/lufa.c
index f1908b3d07..85d71d0835 100644
--- a/tmk_core/protocol/lufa/lufa.c
+++ b/tmk_core/protocol/lufa/lufa.c
@@ -829,9 +829,10 @@ static void send_consumer(uint16_t data) {
  * FIXME: Needs doc
  */
 int8_t sendchar(uint8_t c) {
-    // Not wait once timeouted.
+    // Do not wait if the previous write has timed_out.
     // Because sendchar() is called so many times, waiting each call causes big lag.
-    static bool timeouted = false;
+    // The `timed_out` state is an approximation of the ideal `is_listener_disconnected?` state.
+    static bool timed_out = false;
 
     // prevents Console_Task() from running during sendchar() runs.
     // or char will be lost. These two function is mutually exclusive.
@@ -845,11 +846,11 @@ int8_t sendchar(uint8_t c) {
         goto ERROR_EXIT;
     }
 
-    if (timeouted && !Endpoint_IsReadWriteAllowed()) {
+    if (timed_out && !Endpoint_IsReadWriteAllowed()) {
         goto ERROR_EXIT;
     }
 
-    timeouted = false;
+    timed_out = false;
 
     uint8_t timeout = SEND_TIMEOUT;
     while (!Endpoint_IsReadWriteAllowed()) {
@@ -860,7 +861,7 @@ int8_t sendchar(uint8_t c) {
             goto ERROR_EXIT;
         }
         if (!(timeout--)) {
-            timeouted = true;
+            timed_out = true;
             goto ERROR_EXIT;
         }
         _delay_ms(1);