nuttxpr commented on PR #15757: URL: https://github.com/apache/nuttx/pull/15757#issuecomment-2635295271
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) The provided PR description does not fully meet the NuttX requirements. Here's a breakdown and how it could be improved: **Missing/Insufficient Information:** * **Summary:** * Lacks clarity on *why* the existing behavior was problematic. Explain the consequences of not checking PHYID1/2. (e.g., Network initialization fails silently, leading to difficult debugging, application errors, etc.) * Doesn't specify the exact files changed. (e.g., `drivers/net/ethernet/stm32_eth.c`) * **Impact:** * Needs to explicitly answer all the "Impact on..." questions with NO/YES and explanations where applicable. The current description only covers the user impact. Pay particular attention to: * **Impact on build:** Likely NO, but confirm. * **Impact on hardware:** Specify the affected architecture (STM32H7). Mentioning the specific board (ARKV6X) is helpful but the focus should be on the MCU. * **Impact on documentation:** If no documentation changes are needed, explicitly state "NO". If the bug fix warrants a documentation update (even a minor one), consider adding it. * **Impact on security:** Likely NO, but confirm. * **Impact on compatibility:** Important! Does this change break any existing configurations or introduce any compatibility concerns with older versions of NuttX or specific PHY chips? If no, explicitly state so and explain why. * **Testing:** * **Build Host(s):** Missing operating system, compiler version, etc. Be specific. Example: `Linux, Ubuntu 22.04, GCC 11.3.0` * **Target(s):** Needs more detail than just "ARKV6X". Specify `STM32H7, ARKV6X` or similar. Crucially, mention the NuttX configuration used (e.g., `nsh`). * **Testing logs:** The provided logs only show the *after* state (the fix working and preventing initialization). You **must** provide logs demonstrating the *before* behavior (the original bug where initialization proceeds despite the missing PHY). This shows that the change actually fixes the identified problem. **Example of an Improved PR Description:** ## Summary This PR fixes a bug in the STM32 Ethernet driver where network initialization would succeed even if no PHY was connected. This was caused by the driver not verifying the PHY ID registers (PHYID1 and PHYID2) during initialization. The absence of this check meant that the driver could erroneously report a successful initialization, leading to application errors and difficult debugging. This change adds the missing PHY ID checks to `drivers/net/ethernet/stm32_eth.c`, ensuring that initialization fails if no valid PHY is detected. Fixes #[NuttX Issue Number, if applicable]. ## Impact * Is new feature added? NO * Is existing feature changed? YES, the STM32 ethernet driver now correctly detects the absence of a PHY. * Impact on user: YES. Users will no longer experience silent failures during network initialization when no PHY is present. Error messages will now be logged, aiding debugging. * Impact on build: NO * Impact on hardware: YES. This change specifically affects the STM32H7 architecture (e.g., ARKV6X board). * Impact on documentation: NO. * Impact on security: NO. * Impact on compatibility: NO. This fix corrects an error condition and does not introduce any backward compatibility issues. It is expected to work with a wider range of PHY chips than the previous, unverified behavior. * Anything else to consider: None. ## Testing I confirm that changes are verified on local setup and works as intended: * Build Host(s): Linux, Ubuntu 22.04, GCC 11.3.0 * Target(s): STM32H7, ARKV6X, nsh configuration Testing logs before change (without PHY connected): ``` [SHOW LOGS OF FAILED INITIALIZATION *WITHOUT* THE FIX] (e.g., showing initialization succeeding despite no PHY) ``` Testing logs after change (without PHY connected): ``` [PASTE THE PROVIDED LOGS SHOWING THE CORRECT ERROR REPORTING] ``` Testing logs after change (with PHY connected): ``` [SHOW LOGS OF SUCCESSFUL INITIALIZATION WITH THE FIX APPLIED AND A PHY CONNECTED] This demonstrates that the fix does not break normal functionality. ``` By providing comprehensive information, you significantly increase the chances of your PR being reviewed and merged quickly. Reviewers will appreciate the clarity and completeness, making their job much easier. Remember to replace the bracketed placeholders with the actual information. -- 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
