nuttxpr commented on PR #15583: URL: https://github.com/apache/nuttx/pull/15583#issuecomment-2596412226
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR mostly meets the NuttX requirements, but could be improved. **Strengths:** * **Clear Summary:** Explains the *what* and *why* of the change, referencing the STM32H7 as a basis. The specific differences related to `stm32_uid.c` and `ETH_SEL_PHY` are helpful. The note about ICACHE/DCACHE is also relevant. * **Impact is identified:** Correctly highlights the impact is on the STM32H5 architecture. * **Testing information provided:** Includes hardware, software, configuration, and procedure used for testing. The link to the test log is helpful but the logs themselves should be inline if possible (see below). **Weaknesses & Suggestions for Improvement:** * **Summary: Missing Issue References:** Even if there isn't a directly related issue, consider creating one to track the addition of STM32H5 Ethernet support. This helps with traceability and allows others to follow the development process. * **Impact: Needs more detail:** While "STM32H5 Architecture" is correct, it's too general. Specifically mention: * **Impact on user:** YES - Users will now be able to use Ethernet on STM32H5 platforms. Briefly describe how to configure it (mentioning the similarity to STM32H7 configuration). * **Impact on build:** YES - New driver code added, build system changes likely required. Briefly describe the changes, e.g., new Kconfig options, board configuration updates. * **Impact on hardware:** YES - Specifies RMII is used. Mention any specific hardware requirements/dependencies. * **Impact on documentation:** YES - Documentation needs to be updated to describe the new driver and its configuration. Confirm whether documentation updates are included in the PR or planned for a separate update. * **Impact on security, compatibility:** Explicitly state NO if there are no impacts. * **Testing: Inline logs (if reasonable size):** While the external link is acceptable, including the relevant snippets of the logs directly in the PR description improves readability and makes it easier for reviewers to assess the changes. If the logs are very long, summarize the key parts inline and keep the full log as an external link. Indicate *what* the logs demonstrate (e.g., successful ping, MDIO register reads/writes). By addressing these weaknesses, the PR will be more complete and easier for maintainers to review and merge. -- 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
