nuttxpr commented on PR #15798: URL: https://github.com/apache/nuttx/pull/15798#issuecomment-2646788330
[**\[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 could be improved with more detail. Here's a breakdown of what's good and what's missing: **Strengths:** * **Clear Summary:** The summary explains the "what" (adding XIAO nRF52840 support) and the "how" (four initial configurations). * **User Impact is Addressed:** The impact section mentions the new ability to use NuttX on the board. * **Testing Instructions Provided:** Clear steps for building and accessing the shell are given. * **Testing Output Shown:** Example output from `uname -a` verifies basic functionality. **Weaknesses & Missing Information:** * **Missing "Why":** The summary doesn't explain *why* this change is necessary. Is there a demand for this board? Does it offer unique capabilities? * **Functional Change Detail:** The summary mentions configurations but lacks specifics about code changes. Which files were modified? Were new drivers or board-specific logic added? * **Incomplete Impact Assessment:** The impact section is too brief. Consider these points: * **Build Impact:** Did adding this board require changes to the build system (e.g., new Kconfig options, board directory structure)? * **Hardware Impact:** Specify the architecture (ARM Cortex-M4) and any board-specific hardware used (e.g., UART, USB peripheral). * **Documentation Impact:** Does this PR include updates to the documentation (e.g., board configuration instructions)? If not, it should. * **Other Impacts:** The remaining impact categories (security, compatibility, etc.) are simply omitted. Even if the answer is "NO," explicitly state it for each. * **Insufficient Testing:** The testing section only demonstrates basic shell access. More comprehensive testing is needed to validate the different configurations (nsh, usbnsh, leds, gpio). Show output from the `leds` and `gpio` examples. Test different peripherals. Consider testing on multiple build hosts if feasible. * **Missing "Before" Logs:** The template requests "Testing logs before change," which are absent. While not always strictly necessary for new board support, it's good practice to show that the board *didn't* work before your changes. Even a simple build failure log would suffice. **Recommendations for Improvement:** 1. **Expand the Summary:** Explain the motivation for adding support for this board. 2. **Detail Code Changes:** Be more specific about the files modified and the code added. 3. **Complete the Impact Assessment:** Address *all* impact categories, even if the answer is "NO." Provide specifics for "YES" answers. 4. **Enhance Testing:** Provide testing output for all configurations, demonstrating the functionality of the `leds` and `gpio` examples. Test other peripherals if applicable. If possible, test on different build hosts. 5. **Include "Before" Logs:** Even a simple build failure log demonstrating that the board wasn't previously supported would be helpful. By addressing these points, the PR will be significantly stronger and more likely to be accepted. -- 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
