nuttxpr commented on PR #15720: URL: https://github.com/apache/nuttx/pull/15720#issuecomment-2622243977
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) __Fill In The Commit Message:__ This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary. The PR summary is okay, but could be improved by mentioning the specific Ethernet controller used (e.g., "Adds support for the Xilinx GEM Ethernet controller on the Zynq MPSOC"). The impact section needs significant improvement. Here's a breakdown: * **Impact on user:** You state "Users may need to reconfigure their UART settings..." This is completely unrelated to Ethernet and suggests a problem with the PR. Remove this. The actual user impact is that they can now use the Ethernet interface on the ZCU111 board. Be specific: "Users can now configure and use the Ethernet interface on the ZCU111 board. This includes assigning IP addresses, using network applications like ping, etc." * **Impact on build:** More detail is needed. Mentioning which files are compiled is good, but also specify where they are located (e.g., "Adds `zynq_enet.c` to `arch/arm/src/zynqmp` and `zcu11_ethernet.c` to `boards/arm/zynqmp/zcu111/src`"). If there are Kconfig changes, mention them here. * **Impact on hardware:** Specify that it *only* impacts Zynq MPSOC and explicitly mention the ZCU111. "This impacts the Zynq UltraScale+ MPSoC, specifically enabling Ethernet functionality on the ZCU111 board. Other hardware is unaffected." * **Impact on documentation:** While you intend to add documentation later, it's best to include at least basic usage instructions in the initial PR. If that's absolutely impossible, then acknowledge that documentation is missing and will be addressed in a follow-up PR. "Documentation is incomplete and will be provided in a separate PR. However, the driver config option and its usage is..." * **Impact on security:** You say "NO" but provide no justification. Briefly explain why there are no security implications (e.g., "No new network services are introduced, and existing network security measures remain applicable.") * **Impact on compatibility:** Justify the "NO" (e.g., "This adds new functionality and does not break existing functionality.") The testing section is good, showing successful ping tests. However, more comprehensive testing is recommended. Consider including: * **Basic connectivity:** Pinging a gateway or another device on the network. * **Throughput tests:** `iperf` or similar tools. * **Different network configurations:** Static IP, DHCP. * **Error handling:** Testing behavior with unplugged cables, invalid IP addresses, etc. **Revised Impact Section Example:** ``` Impact on user: YES. Users can now configure and use the Ethernet interface on the ZCU111 board. This includes assigning IP addresses and using network applications. Impact on build: YES. Adds `zynq_enet.c` to `arch/arm/src/zynqmp` and `zcu11_ethernet.c` to `boards/arm/zynqmp/zcu111/src`. Adds Kconfig option `CONFIG_ZYNQMP_GEM` (describe usage). Impact on hardware: YES. This enables the Ethernet functionality on the Zynq UltraScale+ MPSoC, specifically on the ZCU111 board. Other hardware is unaffected. Impact on documentation: NO. Documentation will be provided in a separate PR. [Optional: Briefly describe how to enable the driver in menuconfig or via defconfig]. Impact on security: NO. This change does not introduce new network services and does not alter existing network security mechanisms. Impact on compatibility: NO. This adds new functionality and does not modify existing behavior. ``` By addressing these points, you'll create a much stronger and more informative PR. Reviewers will appreciate the clarity and completeness, leading to a faster and smoother review process. -- 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
