nuttxpr commented on PR #15598: URL: https://github.com/apache/nuttx/pull/15598#issuecomment-2599568935
[**\[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 some sections could be expanded for clarity. **Strengths:** * **Clear Summary:** The summary explains the "why," "what," and "how" of the change. The link to the pico-sdk PR provides valuable context. * **Impact Addressed:** The impact section covers several key areas (user, hardware, compatibility). * **Testing Provided:** Testing results are included, although they could be more robust. **Areas for Improvement:** * **Summary:** While the summary is good, explicitly mentioning the new configuration option added to `board.h` would be beneficial. For example: "This PR adds a `CONFIG_RP2040_XOSC_STARTUP_DELAY_MS` configuration option in `board.h` to allow boards to specify a longer startup delay for the XOSC." * **Impact:** While the impact on the user, hardware, and compatibility is mentioned, several sections are marked as "NO" without explanation. Even if there's no impact, explicitly stating "NO - No change to the build process" is better for clarity. The impact on documentation should be addressed – does this new config option need to be documented? Security and forward/backward compatibility implications should also be explicitly addressed, even if the answer is "NO - No security implications." * **Testing:** While testing is provided, it's limited. Ideally, you'd provide logs demonstrating the failure *before* the change and the successful boot *after* the change. The current testing description suggests the initial board works, making the test less convincing. Find a board that consistently exhibits the failure to showcase the fix effectively. Include specifics of the build host (OS, compiler version). Clearly state which target architecture and board configurations were tested. **Example of Improved Testing Section:** ``` Testing: Build Host(s): macOS Ventura 13.5, clang version 14.0.6 Target(s): arm, adafruit-feather-rp2040:usbnsh, seeed-xiao:nsh Testing logs before change (adafruit-feather-rp2350 - Note: This board will be added in a separate PR): ``` <Insert log showing boot failure here> ``` Testing logs after change (adafruit-feather-rp2350): ``` <Insert log showing successful boot here> ``` Testing logs before/after change (adafruit-feather-rp2040, seeed-xiao): (Note: These boards did not exhibit the failure consistently but were tested to ensure the change did not introduce regressions) ``` <Insert relevant portions of logs, focusing on XOSC initialization> ``` ``` By addressing these points, the PR will be even stronger and easier for reviewers to assess. -- 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
