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

Reduce the chances of xon/xoff causing comms to wedge #1369

Merged
merged 2 commits into from
Dec 13, 2023

Conversation

obra
Copy link
Member

@obra obra commented Dec 12, 2023

As previously implemented, it's possible that our xon/off flow control could send XOFF, do a read, and then...not send XON, leading to a stuck pipe

could send XOFF, do a read, and then...not send XON, leading to a stuck
pipe
@obra obra requested a review from tlyu December 12, 2023 22:41
@tlyu
Copy link
Collaborator

tlyu commented Dec 12, 2023

I was still getting XOFF with no XON with one of my test cases (63 A followed by a newline, sent via tio hexadecimal mode).

I added the additional patch here, which fixed it. The extra calls to flush() are probably also a good idea.

--- a/plugins/Kaleidoscope-FocusSerial/src/kaleidoscope/plugin/FocusSerial.cpp
+++ b/plugins/Kaleidoscope-FocusSerial/src/kaleidoscope/plugin/FocusSerial.cpp
@@ -39,11 +39,13 @@ void FocusSerial::manageFlowControl() {
   if (xon == true) {
     if (avail > RECV_BUFFER_THRESHOLD) {
       Runtime.serialPort().write(XOFF);  // Send XOFF to stop data
+      Runtime.serialPort().flush();
       xon = false;
     }
   } else {
     if (avail < RECV_BUFFER_RESUME) {
       Runtime.serialPort().write(XON);  // Send XON to resume data
+      Runtime.serialPort().flush();
       xon = true;
     }
   }
@@ -65,6 +67,7 @@ EventHandlerResult FocusSerial::afterEachCycle() {
       break;
     }
     c = Runtime.serialPort().read();
+    manageFlowControl();
     // Don't store the separator; just stash it
     if (c == SEPARATOR) {
       break;
@@ -82,6 +85,7 @@ EventHandlerResult FocusSerial::afterEachCycle() {
   Runtime.onFocusEvent(input_);
   while (Runtime.serialPort().available()) {
     c = Runtime.serialPort().read();
+    manageFlowControl();
     if (c == NEWLINE) {
       // newline serves as an end-of-command marker
       // don't drain the buffer past there

@tlyu
Copy link
Collaborator

tlyu commented Dec 12, 2023

Also, not a blocker for this change, but I think that if we can get the USB-level flow control to work with the new ZLP patches, we shouldn't add complexity by adding XON/XOFF flow control. Or maybe implement it at the Serial abstraction, if we really have to, but that has its own risks.

@obra
Copy link
Member Author

obra commented Dec 13, 2023

Just to confirm, I'd be delighted if it turned out we didn't need xon/xoff flow control.

…followed by a newline, sent via tio hexadecimal mode).

I added the additional patch here, which fixed it. The extra calls to flush() are probably also a good idea.
@obra obra merged commit db675ec into master Dec 13, 2023
15 checks passed
@obra obra deleted the f/flow-control-after-read branch December 13, 2023 06:24
@tlyu tlyu linked an issue Dec 14, 2023 that may be closed by this pull request
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

Successfully merging this pull request may close these issues.

xon/xoff flow control may be stalling out waiting for more input.
2 participants