nuttxpr commented on PR #3024: URL: https://github.com/apache/nuttx-apps/pull/3024#issuecomment-2717490414
[**\[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 some clarifications could strengthen it. **Strengths:** * **Clear Summary:** The summary explains the "why," "what," and "how" of the changes. The reference to the related documentation PR is excellent. * **Impact Acknowledged:** The impact section addresses most points, particularly highlighting the breaking change and justifying it. * **Testing Description:** The testing description covers various scenarios and provides context (platform used). **Areas for Improvement (to make it even stronger):** * **Summary - Issue Reference:** If there's a related NuttX issue, include a link. Even if the change is internally driven, creating an issue to track it can be beneficial. * **Impact - User Impact (Elaborate):** While it states "no one is using it," providing more detail about *how* users would adapt (e.g., regenerate images with the new format) is helpful, even if the user base is small. * **Impact - Build Impact:** Clarify if the build process itself changes (e.g., new build options, changes to existing build scripts). Even if the answer is "NO," explicitly stating it is good practice. * **Impact - Hardware Impact:** Specify the affected architecture(s) (SAMv7 is mentioned in testing, but reiterate it in the impact section). * **Impact - Security Impact:** While you might not expect any security implications, explicitly stating "NO" and briefly explaining why (e.g., "No new security vulnerabilities introduced") is a good defensive practice. * **Impact - Compatibility:** Elaborate on the "breaking change." How will existing images be handled? Are there migration steps? This ties in with user impact. * **Testing - Be More Specific:** "Various scenarios" is good, but adding concise bullet points for each scenario and the *expected result* would improve the testing section significantly. For example: * **Revert after update:** The bootloader successfully boots the original primary slot image. * **Confirm after update:** The bootloader boots the updated image and marks it as permanent. * **Testing - Logs (Consider):** While the template requests logs, they might not be strictly necessary for all changes. However, if readily available, including snippets of relevant log output (especially for error conditions or unusual behavior) can be very helpful. If logs are omitted, briefly explain why (e.g., "Logs are not included as they are voluminous and don't provide additional insight beyond the described test cases"). By addressing these points, the PR would be even more robust and easier for reviewers to assess. -- 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
