nuttxpr commented on PR #16075: URL: https://github.com/apache/nuttx/pull/16075#issuecomment-2751764770
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR does **not** fully meet the NuttX requirements. While it provides a good explanation of the problem and solution, it lacks crucial information for proper review and acceptance. Here's a breakdown of what's missing: * **Summary:** While the *what* and *how* are explained well, the *why* is slightly weak. Instead of just saying it increases performance and flash life, quantify it. How much faster? What's the estimated impact on flash lifespan (e.g., estimated increase in write cycles)? A related NuttX issue should be created and linked if one doesn't exist. * **Impact:** While the author claims no impact other than performance, this needs more thorough investigation. * **Impact on user:** Even performance improvements can be considered user impact. Describe if any user-facing configuration or behavior might change. * **Impact on build:** Explicitly state NO. * **Impact on hardware:** Specify the affected architecture(s) (e.g., those using the at45db driver). State NO for other architectures. * **Impact on documentation:** If the performance improvement is significant, documentation (perhaps the at45db driver documentation) should be updated to reflect this. So likely YES. * **Impact on security:** Explicitly state NO. * **Impact on compatibility:** This is crucial. Does this change break compatibility with any existing at45db configurations or usage patterns? Thoroughly investigate and justify the answer. * **Testing:** This is the biggest deficiency. Testing on an older branch is insufficient. While porting to `main` might be a large task, testing on the latest NuttX release is *essential* for proper integration. The maintainers need to see evidence that the change works correctly in the current codebase. The provided "testing logs" are placeholders and offer no actual verification. Include real logs demonstrating the double erase before the change and the single erase after the change, including timings to quantify the improvement. If possible, provide a simple test case that others can reproduce. **Recommendations for the PR author:** 1. **Quantify improvements:** Provide concrete numbers for performance gains and flash life expectancy improvements. 2. **Test on latest NuttX:** Bite the bullet and test on the latest release. This is unavoidable for proper review. If porting the entire system is impossible, consider creating a minimal test case within the latest NuttX to isolate and demonstrate the changes. 3. **Provide real test logs:** Include actual logs showing the double/single erase behavior and timings. 4. **Address all impact points explicitly:** Don't assume; state clearly whether each impact area is affected or not, and provide justification. 5. **Create a NuttX issue:** If one doesn't exist, create an issue to track this problem and link it to the PR. By addressing these points, the PR will have a much higher chance of being accepted. -- 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
