nuttxpr commented on PR #15105: URL: https://github.com/apache/nuttx/pull/15105#issuecomment-2530511358
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR appears to **mostly** meet the NuttX requirements, but could be improved with more specific information in the Testing section. Here's a breakdown: **Strengths:** * **Clear Summary:** The summary explains the *why* (continuation of previous work to eliminate `spin_lock_irqsave(NULL)`), the *what* (adding `g_` prefix to RTC spinlock), and the *how* (implicitly, by renaming the spinlock). The related commit hash provides valuable context. * **Impact:** While "N/A" is acceptable if there's truly no impact beyond the specific code change, it's always better to be explicit. For example, stating "No impact on users, build process, hardware, documentation, security, or compatibility" demonstrates that these aspects were considered. * **References previous commit:** This helps reviewers understand the context and rationale for the change. **Weaknesses:** * **Testing:** "ci-check" is insufficient. While CI is crucial, the PR should include specific details about the testing environment and results. This should include: * **Build Host:** OS, CPU architecture, compiler version used for building NuttX. * **Target:** The specific architecture and board configuration tested on. Was this tested on real hardware or a simulator? * **Logs (or equivalent):** "Before" and "after" logs are ideal, but if the change is subtle, a description of the expected behavior and confirmation that it was observed is acceptable. For driver changes, demonstrating correct driver function before and after is important. Even a simple statement like "RTC functionality verified on simulator before and after change" adds valuable information. **Recommendation:** To fully meet the requirements, expand the Testing section. Provide specific details about the test environment and, if feasible, include relevant logs or a clear description of the observed behavior before and after the change. Even a brief confirmation that the RTC continues to function correctly after the change is preferable to simply stating "ci-check". -- 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]
