nuttxpr commented on PR #15812: URL: https://github.com/apache/nuttx/pull/15812#issuecomment-2650481790
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR appears to meet the NuttX requirements, although it could be improved with more specific information. Here's a breakdown of what's good and what could be better: **Strengths:** * **Clear Summary:** The summary explains the problem, the solution, and why the previous behavior was undesirable. * **Impact Description:** Addresses the user impact (correct error handling) and implicitly acknowledges the lack of other impacts. * **Testing Information:** Provides target architecture, board, and specific flash chips used for testing. Mentions different behavior between single and dual-die flashes, suggesting thorough testing. **Weaknesses:** * **Missing References:** No NuttX issue references are provided. Even if there isn't a directly related issue, it's good practice to create one to track bugs or feature requests. * **Vague Testing Logs:** "Testing logs before/after change" sections are empty. Include actual log output demonstrating the incorrect behavior before the change and the correct behavior afterward. Be specific about what commands/tests were run. * **Limited Build Host Info:** Only the target is specified. Include details about the build host OS, CPU architecture, and compiler version. This is crucial for reproducibility. * **Lack of Explicit "NO" Answers:** For the impact section, explicitly state "NO" for the categories where there's no impact. This improves clarity and shows you considered each point. Example: "Impact on build (will build process change)? NO" * **Missing Detail on "How it Works":** The summary briefly mentions the fix, but the "How does the change exactly work" part of the requirements could be expanded. Briefly describe the code changes made to fix the error handling and wait for operation completion (e.g., "Modified `w25qxxxjv_erase` to check the return code of `w25qxxxjv_erase_sector` and retry until success or timeout. Removed the EBUSY return code and implemented a wait loop using the flash's timing parameters."). **Recommendation:** Add the missing information and details described above to strengthen the PR. Clear, concise, and complete information makes it much easier for reviewers to understand and approve the changes. -- 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
