nuttxpr commented on PR #15070: URL: https://github.com/apache/nuttx/pull/15070#issuecomment-2521940393
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR appears to meet the basic NuttX requirements, but it's missing some crucial details. While it addresses the summary and testing sections reasonably well, the impact section is insufficient. Here's a breakdown of what's missing and how to improve it: **Missing Information/Improvements Needed:** * **Impact - More Detail:** Simply stating "The boards has enabled usbmsc" isn't enough. You need to specify *which* boards are affected. Also, address *all* the impact points: * **Impact on user:** Will users need to change their configuration files or scripts to use this new feature? If not, explicitly state "NO". If yes, explain the changes required. * **Impact on build:** Does this change add any new dependencies or alter the build process in any way? Even if the answer is no, explicitly state "NO". * **Impact on hardware:** Be specific about the hardware affected (e.g., "This change affects the ESP32-S3 and LC823450 boards when configured with USB MSC support"). * **Impact on documentation:** Does the documentation need to be updated to reflect this change? If yes, confirm whether the PR includes those updates or if a separate documentation PR will be needed. If no, explicitly state "NO". * **Impact on security:** Consider if allowing runtime path binding for LUNs introduces any security vulnerabilities. Even if the answer is no, explicitly state "NO" and briefly justify why (e.g., "NO - No new attack surface introduced as access controls remain unchanged"). * **Impact on compatibility:** Does this change break backward compatibility? Will it work with older versions of NuttX or applications? Even if the answer is no, explicitly state "NO". * **Testing - More Detail:** While the testing description is a good start, more context would be helpful. * **Build Host(s):** Specify the OS, CPU architecture, and compiler used for your local testing. * **Testing Logs:** The placeholder "your testing logs here" needs to be replaced with actual log output. Focus on showing that the new functionality works as expected (e.g., successfully binding a path to a LUN at runtime). Demonstrate both successful and unsuccessful scenarios (e.g., trying to bind to an invalid path). **Example of Improved Impact Section:** ``` ## Impact * Is new feature added? YES. This adds the ability to set paths that bind to LUN at runtime for the USB MSC class driver. * Impact on user: YES. Users will need to configure the new options in their defconfig or menuconfig to specify the paths for LUN binding. Example configuration will be added to the documentation. * Impact on build: NO. * Impact on hardware: This change affects ESP32-S3 and LC823450 boards when configured with USB MSC support. * Impact on documentation: YES. Documentation will be updated to explain the new configuration options and provide usage examples. These updates are included in this PR. * Impact on security: NO. No new attack surface introduced as access controls remain unchanged. Runtime binding is subject to the same permissions as static configuration. * Impact on compatibility: NO. This change maintains backward compatibility. Existing configurations without the new options will continue to function as before. ``` By providing more complete information, you'll make it much easier for reviewers to understand and approve your PR. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
