nuttxpr commented on PR #15927: URL: https://github.com/apache/nuttx/pull/15927#issuecomment-2693732698
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) No, this PR does not meet the NuttX requirements. The provided information is far too brief and lacks critical details. Here's why: * **Summary:** "usb host support more cdcacm composite device descriptor" is vague. *Which* composite device descriptors are added? *How* is support being added? What problem does this solve (i.e., *why* is this change necessary)? A related NuttX issue number should be included if applicable. * **Impact:** Simply stating "usb host cdcacm" is insufficient. Each impact category (user, build, hardware, documentation, security, compatibility) needs a YES/NO answer along with a description if the answer is YES. For example, even if the answer is NO for most, it needs to be explicitly stated. * **Testing:** "selftest in sim" is not enough. Specify the *host OS*, the simulator being used (e.g., QEMU), the *specific selftest(s)* run. Critically, there are no "before" and "after" logs provided to demonstrate the change's effect. These logs are essential for reviewers to understand what the change accomplishes. **Example of how to improve this PR description:** ## Summary This PR adds support for the composite device descriptor used by [Specific Device Manufacturer and Model] USB CDC/ACM devices. Currently, NuttX does not correctly enumerate these devices due to [explain the specific issue]. This change adds parsing logic for the specific descriptor format used by these devices, enabling proper enumeration and driver attachment. Fixes #[NuttX issue number]. ## Impact * Is new feature added? YES (Support for [Specific Device Manufacturer and Model] USB CDC/ACM devices) * Impact on user (will user need to adapt to change)? NO * Impact on build (will build process change)? NO * Impact on hardware (will arch(s) / board(s) / driver(s) change)? NO * Impact on documentation (is update required / provided)? YES (The USB host documentation will be updated to reflect the newly supported device.) * Impact on security (any sort of implications)? NO * Impact on compatibility (backward/forward/interoperability)? NO * Anything else to consider? This change has been tested with a physical device as well as in simulation. ## Testing I confirm that changes are verified on local setup and works as intended: * Build Host(s): Linux (Ubuntu 20.04), x86_64, GCC 9.4.0 * Target(s): sim:qemu-arm Testing logs before change: ``` $ ./tools/configure.sh sim:qemu-arm ... $ make ... $ nshl nsh> usbhost Attached Devices: None nsh> dmesg | grep usbhost ... (showing no device detected) ... ``` Testing logs after change: ``` $ ./tools/configure.sh sim:qemu-arm ... $ make ... $ nshl nsh> usbhost Attached Devices: /dev/ttyACM0 nsh> dmesg | grep usbhost ... (showing device detected and driver attached) ... ``` By providing specific details and evidence, reviewers can efficiently assess the PR and provide constructive feedback. This level of detail is crucial for a successful contribution to the NuttX project. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
