nuttxpr commented on PR #15974: URL: https://github.com/apache/nuttx/pull/15974#issuecomment-2714568560
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) No, this PR description does not fully meet the NuttX requirements. Here's why and how to fix it: **Summary:** * **Insufficient Detail:** While it states the change, it lacks crucial details. It needs to explain *how* the GPIO driver is implemented (e.g., "reuses the existing button/LED driver infrastructure," "implements a new driver from scratch," etc.). It should be more explicit about *what* changes in the code. * **Missing Links:** There's no mention of related NuttX or NuttX Apps issues. Even if there aren't any, explicitly stating "None" is helpful. **Impact:** * **Incomplete:** While it touches upon user impact, it's very vague. It needs to be more specific: * **User Impact:** "Users can now control the on-board LEDs and buttons through the standard GPIO driver interface, allowing for more flexible control within applications. This requires enabling `CONFIG_NRF52840DK_BTNLEDS_GPIO=y` in the configuration." * **Build Impact:** Does enabling this option change any dependencies or Kconfig options? Be specific. If no, state "NO." * **Hardware Impact:** Specify that this only impacts the nrf52840-dk board. If no other hardware is affected, explicitly state "NO" for other architectures/boards. * **Documentation:** Does this require documentation updates? If so, were they provided in the PR? * **Security:** Unlikely a security impact, but explicitly state "NO" for clarity. * **Compatibility:** Are there backward compatibility concerns? Does this break existing code? Will future changes be affected? Even if the answer is "NO," state it explicitly. * **Consider:** This section is meant for unusual or unexpected side effects. If none, state "None." **Testing:** * **Insufficient Detail:** This is the most critical deficiency. * **Build Host(s):** Provide specifics about the development environment: e.g., "Linux (Ubuntu 20.04), x86_64, GCC 9.4.0" * **Target(s):** Be precise: "nrf52840-dk with the `nsh` configuration, using the QEMU simulator" (or specify the actual hardware if used). * **Missing Logs:** The "before" and "after" logs are essential. They demonstrate that the change works as intended. Show commands used to test and their output. Examples: * **Before:** Show how the LEDs/buttons were controlled *before* this change (if applicable). If there was no previous way, state that. * **After:** Show commands using the GPIO interface to control the LEDs and buttons and the resulting behavior (e.g., `gpio -w /dev/gpio0 1` to turn on an LED). **Example of an improved Testing section:** ``` Testing: Build Host(s): Linux (Ubuntu 22.04), x86_64, GCC 11.2.0 Target(s): nrf52840-dk, nsh configuration, QEMU simulator Testing logs before change: Buttons and LEDs were controlled directly through the button and LED drivers. There was no GPIO interface for them. Testing logs after change: nsh> gpio -w /dev/gpio0 1 # Turn on LED 1 nsh> gpio -r /dev/gpio1 # Read the state of button 1 (0 = pressed, 1 = released) 0 nsh> gpio -w /dev/gpio1 # Configure button 1 as an output (for demonstration) nsh> gpio -w /dev/gpio1 1 # Set button 1 output high ``` By providing these details, the PR will be much easier to review and merge. It demonstrates that the change was thoroughly tested and considered. -- 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
