Fix how USB queue overflow is handled in chibios. #12576
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit reverts PR 12472 (commit c823fe2), 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.
This was referenced Apr 13, 2021
stapelberg
reviewed
Apr 14, 2021
stapelberg
approved these changes
Apr 14, 2021
stapelberg
reviewed
Apr 14, 2021
drashna
approved these changes
Apr 20, 2021
…re 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
tzarc
approved these changes
Apr 25, 2021
makenova
pushed a commit
to makenova/qmk_firmware
that referenced
this pull request
Apr 26, 2021
* Fix how USB queue overflow is handled in chibios. This commit reverts PR 12472 (commit c823fe2), 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
|
Thanks for this! I think this PR might be what resolved #11389 for me. Cheers! |
rizo
pushed a commit
to rizo/qmk_firmware
that referenced
this pull request
May 10, 2021
* Fix how USB queue overflow is handled in chibios. This commit reverts PR 12472 (commit c823fe2), 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
toddyamakawa
pushed a commit
to toddyamakawa/qmk_firmware
that referenced
this pull request
May 19, 2021
* Fix how USB queue overflow is handled in chibios. This commit reverts PR 12472 (commit c823fe2), 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
HokieGeek
pushed a commit
to HokieGeek/qmk_firmware
that referenced
this pull request
Jul 11, 2021
* Fix how USB queue overflow is handled in chibios. This commit reverts PR 12472 (commit c823fe2), 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
BorisTestov
pushed a commit
to BorisTestov/qmk_firmware
that referenced
this pull request
May 23, 2024
* Fix how USB queue overflow is handled in chibios. This commit reverts PR 12472 (commit c823fe2), 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This commit reverts PR #12472 (commit c823fe2),
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:
and drop data, even if hid_listen was running. (For example having matrix debug
enabled just didn't work right at all)
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.
Types of Changes
Issues Fixed or Closed by This PR
Checklist