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

sys/usb_cdc_acm_stdio: only submit and flush for non-empty buffer [backport 2024.10] #20987

Open
wants to merge 1 commit into
base: 2024.10-branch
Choose a base branch
from

Conversation

benpicco
Copy link
Contributor

Backport of #20986

Contribution description

There seems to be some problem the implementation of stdio over usbus_cdc_acm where printing an empty string sometimes leads to the next character being missed in the printout. I've managed to pin the problem down to the call to usbus_cdc_acm_flush(&cdcacm); in sys/usb/usbus/cdc/acm/cdc_acm_stdio.c and, more specifically, to _ep_xmit in cpu/nrf5x_common/periph/usbdev.c, but since I have no non-nRF hardware at hand, I cannot tell whether it is a nRF specific issue.

In any case, skipping the flushing (and submitting) in the first place seems to fix the issue for now. If anyone has an idea how to investigate this further and maybe fix the root cause somewhere deeper in the usb driver, please tell me, I'm happy to test more.

Testing procedure

  1. apply the following diff:
diff --git a/tests/sys/usbus_cdc_acm_stdio/main.c b/tests/sys/usbus_cdc_acm_stdio/main.c
index c603882121..50c0b9a9d8 100644
--- a/tests/sys/usbus_cdc_acm_stdio/main.c
+++ b/tests/sys/usbus_cdc_acm_stdio/main.c
@@ -21,6 +21,7 @@
 #include <stdlib.h>
 
 #include "shell.h"
+#include "fmt.h"
 
 static int cmd_text(int argc, char **argv)
 {
@@ -30,7 +31,7 @@ static int cmd_text(int argc, char **argv)
         return -1;
     }
     int n = atoi(argv[1]);
-    if (n <= 0) {
+    if (n < 0) {
         puts(usage);
         return -1;
     }
@@ -40,7 +41,9 @@ static int cmd_text(int argc, char **argv)
         }
         putc('0' + (i % 10), stdout);
     }
-    puts("");
+    print_str("");
+    print_str("x");
+    print_str("\n");
     return 0;
 }
  1. pick any (nrf-based?) board using stdio_cdc_acm such as feather-nrf52840-sense
  2. make -C tests/sys/usbus_cdc_acm_stdio BOARD=feather-nrf52840-sense flash term
  3. repeatedly enter text 0

On master, this sometimes skips over the x in the printout. With this PR, I never managed to reproduce the issue.

Issues/PRs references

Found while investigating failing tests for #20980

@benpicco benpicco added Area: sys Area: System Area: USB Area: Universal Serial Bus CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: release backport Integration Process: The PR is a release backport of a change previously provided to master labels Nov 14, 2024
@benpicco benpicco added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Nov 14, 2024
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the backport!

@riot-ci
Copy link

riot-ci commented Nov 14, 2024

Murdock results

✔️ PASSED

ed00724 sys/usb_cdc_acm_stdio: only submit and flush for non-empty buffer

Success Failures Total Runtime
10215 0 10215 19m:58s

Artifacts

@benpicco benpicco added this pull request to the merge queue Nov 14, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: sys Area: System Area: USB Area: Universal Serial Bus CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: release backport Integration Process: The PR is a release backport of a change previously provided to master Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants