nuttxpr commented on PR #16130: URL: https://github.com/apache/nuttx/pull/16130#issuecomment-2775518159
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR appears to meet the basic NuttX requirements, but could be improved. Here's why and how: **Strengths:** * **Clear Summary:** The summary explains the "what" and "why" of the change and mentions a quantifiable performance improvement. * **Impact Section Mostly Addressed:** The impact section answers most questions, particularly highlighting that the feature is off by default. * **Testing Information Provided:** Build host, target architecture, and configuration are specified, along with a general description of the performance improvement observed. **Weaknesses and Suggested Improvements:** * **Missing Issue References:** If this relates to a specific issue in either the NuttX or NuttX-apps repositories, those should be linked in the summary. Even if it doesn't directly address an existing issue, it might be worth creating one to track the desire for this optimization. * **Impact - Build:** While you say "No" to build impact, you *do* mention it needs to be enabled. Clarify *how* it's enabled (e.g., a Kconfig option). This constitutes a build process change, even if minor. So, the answer should likely be "YES" with an explanation of the configuration option. * **Impact - Hardware:** Be more specific than "cortex-r52." State the exact board or SoC you tested on. Also, explicitly state that this optimization *only* applies to ARM architectures with the CRC32 extension. The implication is there, but it should be stated directly. * **Impact - Documentation:** If this new feature requires documentation updates (e.g., how to enable the optimization in the configuration), state that documentation is included/updated. If not needed, explicitly say "NO - Documentation updates are not required for this change." * **Testing Logs:** The "before" and "after" logs are missing the actual timing data. Include the specific measurements (e.g., output of a timing utility) showing the 118us vs. 14us difference. This provides concrete evidence of the improvement. Don't just describe it; show it. By addressing these points, the PR will be significantly stronger and better adhere to the NuttX guidelines. -- 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
