[riot-notifications] [RIOT-OS/RIOT] nrfusb: Fix data stage handling of control writes (#12269)
notifications at github.com
Wed Sep 18 09:08:26 CEST 2019
chrysn approved this pull request.
As it's probably my first review that I'm requested for that I do on time, I'll try to play this by the book:
* Fundamentals: The PR does make sense in that it addresses a protocol situation previously not considered that breaks the old code's behavior. I don't have an overview of the whole CP0 handling code but the fix looks reasonably placed and compact; it is explainable but due to its position in the USB protocol handling unfeasible to run in any standalone version (so that's OK).
* Code: Changes are small enough not to be eligible for duplicate checking. Slight increases in text size (well justified), no increased stack requirements expected (but even if so...). API usage looks OK, no error handling is indicated here (the only external calls are via `usbdev_ep_event_cb_t` handlers that are infallible). I don't understand all semantic implications of the USB state machine handling, but it looks consistent with the rest of the code.
* Test: The latest #11085 version, in a combined test with CDC-ACM and CDC-ECM on nrf52840-dongle, produced working CDC-ACM output but no CDC-ECM device; this PR fixes this. I expect theses tests' outcome to depend on the user's operating system to some extent. My tests were run from Linux 5.0.0, I'd appreciate additional feedback from a MacOS user.
* Coding conventions: I'm no expert there myself, but it matches the surrounding code, and uncrustify didn't complain.
* This change does not warrant any documentation updates, as it only changes particular implementation case handlings.
I'd like to give MacOS testers a day to chime in with test results, but give it a Go unless it breaks things for them.
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the notifications