nuttxpr commented on PR #15450: URL: https://github.com/apache/nuttx/pull/15450#issuecomment-2574807030
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR generally meets the NuttX requirements, but could be improved. Specifically, it's missing details in the Impact and Testing sections. Here's a breakdown of what's good and what needs improvement: **Good:** * **Clear Summary:** The summary explains the "why" (error handling improvements), "what" (checking return values), and "how" (aborting init on error, returning error codes). * **Concise Description:** The writing is clear and to the point. **Needs Improvement:** * **Impact - Be More Specific:** While you mention the impact, you should be more explicit. Instead of just saying "Gives possibility to react...", describe *how* the user would react. For example: "Applications using the KSZ9477 driver can now check the return value of the initialization function and handle failures appropriately, such as retrying initialization or logging the error. Previously, failures would go unnoticed, potentially leading to network connectivity issues." Similar detail is needed for the `mpfs_phyinit` change. Answer all the "Impact on..." questions explicitly (even with "NO"). * **Testing - Insufficient Detail:** * **Build Host:** Specify your OS, CPU architecture, and compiler version (e.g., "Linux, x86_64, GCC 12.2.1"). * **Target:** Be very specific about your target. "Custom mpfs board" isn't enough. What's the SoC? What's the board configuration? (e.g., "Microchip MPFS-ICICLE-KIT with KSZ9477 configured via I2C"). * **Logs:** You *must* provide logs. "Tested with..." is not sufficient. Show logs demonstrating the previous behavior (errors being ignored) and the new behavior (errors being handled). Even simple logs showing successful initialization after the change are better than nothing. Ideally, you should also try to *induce* failures (e.g., disconnect the KSZ9477) to demonstrate that your error handling works as intended. **Example Improvement for Testing Section:** ``` Testing I confirm that changes are verified on local setup and works as intended: * Build Host(s): Linux, x86_64, GCC 12.2.1 * Target(s): Microchip MPFS-ICICLE-KIT with KSZ9477 configured via I2C. Testing logs before change (KSZ9477 simulated failure - cable unplugged): ``` [date/time] Network initialization... [date/time] KSZ9477 initialization complete (no error indication, despite failure). [date/time] Network connection failed. ``` Testing logs after change (KSZ9477 simulated failure - cable unplugged): ``` [date/time] Network initialization... [date/time] KSZ9477 initialization failed: -EIO (I/O error). [date/time] Network initialization aborted. ``` Testing logs after change (successful initialization): ``` [date/time] Network initialization... [date/time] KSZ9477 initialization complete. [date/time] Network connection successful. ``` ``` By adding these details, the PR will be much stronger and easier for reviewers to evaluate. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
