nuttxpr commented on PR #16079: URL: https://github.com/apache/nuttx/pull/16079#issuecomment-2755197859
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR appears to meet the NuttX requirements, although it could be slightly improved. Here's a breakdown: **Strengths:** * **Clear Summary:** The summary explains the problem, the solution, and the affected code area. * **Impact Description:** The impact on the user is clearly stated. * **Testing Provided:** Evidence of testing is included, mentioning the specific target and the observed improvement. **Areas for Improvement (Minor):** * **Missing Issue References:** While not strictly required if no related issue exists, it's generally good practice to create one and link it. This helps with tracking and searchability. * **Incomplete Testing Details:** The "Testing" section could be more robust. While the target is mentioned, providing more context about the *Build Host* would be helpful (OS, compiler, etc.). Also, the "logs" are empty. While full logs might be excessive, a snippet demonstrating the change in behavior (e.g., reading a GPIO value before and after the change) would be beneficial. * **Missing "NO" in Impact Sections:** Explicitly stating "NO" for the unaffected impact categories (build process, hardware changes *other than GPIOD usage*, documentation, security, compatibility) would improve clarity and completeness. **Example of improved "Impact" section:** ``` ## Impact * Is new feature added? NO * Is existing feature changed? YES (GPIO input functionality corrected) * 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)? YES (SAMv7 GPIOD peripheral usage will now function correctly without IRQ configuration) * Impact on documentation (is update required / provided)? NO * Impact on security (any sort of implications)? NO * Impact on compatibility (backward/forward/interoperability)? NO * Anything else to consider? NO ``` **Example of improved "Testing" section:** ``` ## 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): SAMV7, Custom Board (board_config), sim Testing logs before change (CONFIG_SAMV7_GPIOD_IRQ=n): ``` GPIOD pin X read: -1 (or some error indicating failure) ``` Testing logs after change (CONFIG_SAMV7_GPIOD_IRQ=n): ``` GPIOD pin X read: 1 ``` ``` By addressing these minor points, the PR would be even stronger and easier to review. -- 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]
