nuttxpr commented on PR #15855: URL: https://github.com/apache/nuttx/pull/15855#issuecomment-2664771979
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR appears to mostly meet the NuttX requirements, but is missing some key information. Here's a breakdown of what's good and what needs improvement: **Strengths:** * **Clear Summary:** The summary explains the "why," "what," and "how" of the change. The link to the ESP-IDF guide provides good context. * **Concise Impact Description:** The impact description is brief but gets the point across. * **Testing Mentions Targets:** The testing section identifies specific targets. **Weaknesses:** * **Missing Issue References:** If this relates to any existing NuttX issues, those should be linked in the Summary. * **Impact Section Incomplete:** Several "Impact" fields are left blank (build, hardware, documentation, security, compatibility). Even if the answer is "NO," it should be explicitly stated. The current description focuses solely on the user impact and ignores potential impacts on other aspects. * **Insufficient Testing Detail:** While targets are mentioned, there are no actual logs provided. "Before" and "after" logs demonstrating the change are crucial. Furthermore, just stating "build against" isn't sufficient. What was the observed behavior? Did the change have the intended effect? Quantify the improvement if possible. The rationale for skipping ESP32S2 should be in the Impact section (hardware impact). * **Build Host Information Missing:** The testing section lacks information about the build host used (OS, CPU, compiler). * **Missing Contributing Guidelines Adherence Confirmation:** While the note is there, there's no explicit statement confirming adherence to the guidelines. **Recommendations for Improvement:** 1. **Complete the Impact Section:** Explicitly address *all* impact categories, even if the answer is "NO." Justify the "NO" if necessary. For example, if there's no documentation impact, state "Impact on documentation: NO. This change does not require documentation updates." Explain the ESP32S2 exclusion under hardware impact. 2. **Provide Detailed Testing Logs:** Include actual "before" and "after" logs demonstrating the effect of the change. Quantify the improvement in startup time if possible. Show the commands used for building and testing. 3. **Include Build Host Information:** Specify the OS, CPU, and compiler version used for the build. 4. **Link Related Issues (if any):** If no issues exist, explicitly state that. 5. **Explicitly Confirm Adherence to Contributing Guidelines:** Add a statement like "I confirm that this PR adheres to the NuttX Contributing Guidelines." By addressing these points, the PR will be significantly stronger and easier to review. -- 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