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 #20986

Merged
merged 1 commit into from
Nov 14, 2024

Conversation

mguetschow
Copy link
Contributor

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

@github-actions github-actions bot added Area: USB Area: Universal Serial Bus Area: sys Area: System labels Nov 13, 2024
@mguetschow mguetschow added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 13, 2024
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Now that's a very straightforward fix, neat!

@riot-ci
Copy link

riot-ci commented Nov 14, 2024

Murdock results

✔️ PASSED

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

Success Failures Total Runtime
10251 0 10251 19m:06s

Artifacts

@benpicco benpicco added this pull request to the merge queue Nov 14, 2024
Merged via the queue into RIOT-OS:master with commit 70aa1ec Nov 14, 2024
27 checks passed
@mguetschow mguetschow deleted the stdio-cdc-acm-miss branch November 14, 2024 11:47
@mguetschow
Copy link
Contributor Author

Thanks!

@benpicco
Copy link
Contributor

benpicco commented Nov 14, 2024

Should we also backport this to the 2024.10 release?

@mguetschow
Copy link
Contributor Author

May be good, if its not too much hassle :)

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants