nuttxpr commented on PR #15956: URL: https://github.com/apache/nuttx/pull/15956#issuecomment-2708764920
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) The PR mostly meets the NuttX requirements, but is missing some key information. Here's a breakdown: **Strengths:** * **Clear Summary of "Why":** The PR explains the problem (gpio example not working on nrf5340-dk) and the proposed solution. * **Specific Functional Change:** It identifies the affected code (board-level initialization, similar to xiao-nrf52840). * **How it Works:** Explains how LEDs and buttons are configured as GPIOs. * **Testing Evidence:** Provides a picture showing the LEDs lit, indicating some level of testing. **Weaknesses (and how to fix them):** * **Missing Issue References:** Are there any related NuttX or NuttX Apps issues? If not, consider creating one before submitting the PR, as this helps with tracking and context. * **Incomplete Impact Assessment:** * **Impact on User:** While stating it only impacts board-level initialization implies minimal user impact, explicitly stating "NO" or providing a brief explanation would be clearer. * **Impact on Build:** Does enabling `CONFIG_DEV_GPIO` change the build process in any way (e.g., new dependencies)? Clarify. * **Impact on Hardware:** Specify that this *only* affects the nrf5340-dk board. Explicitly state "NO" for other architectures/boards. * **Impact on Documentation:** Does this require documentation updates? If not, state "NO". If so, explain what needs to be updated or if the updates are included in the PR. * **Impact on Security, Compatibility:** Explicitly state "NO" if there are no impacts. Don't leave these blank. * **Insufficient Testing Details:** * **Build Host Details:** Provide details about the build host OS, CPU, compiler, and versions. Example: "Linux (Ubuntu 22.04), x86_64, GCC 11.2.0" * **Target Details:** Be more specific: "nrf5340-dk: default configuration" or specify any configuration changes. * **Missing Logs:** The "Testing logs before change" and "Testing logs after change" sections are empty. These should show the output of relevant commands demonstrating the issue before the change and the successful operation after the change. For example, show attempts to access `/dev/gpio*` before and after the change. The picture is helpful, but insufficient. Consider a short video demonstrating the functionality if the interaction is complex. **Example of improved Testing section:** ``` Testing I confirm that changes are verified on local setup and works as intended: * Build Host(s): Linux (Ubuntu 22.04), x86_64, GCC 11.2.0 * Target(s): nrf5340-dk: default configuration Testing logs before change: ``` $ ls /dev/gpio* ls: cannot access '/dev/gpio*': No such file or directory $ gpioinfo gpioinfo: command not found # Or equivalent error if gpioinfo is available ``` Testing logs after change: ``` $ ls /dev/gpio* /dev/gpio0 /dev/gpio1 /dev/gpio2 /dev/gpio3 /dev/gpio4 /dev/gpio5 /dev/gpio6 /dev/gpio7 $ gpioinfo GPIO0: ... (output, LED 1) GPIO1: ... (output, LED 2) ... GPIO4: ... (input, Button 1) ... ``` (Also include the picture/video here) ``` By addressing these weaknesses, the PR will be much stronger and more likely to be accepted. Remember, clarity and completeness are key to a successful 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: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
