nuttxpr commented on PR #3068: URL: https://github.com/apache/nuttx-apps/pull/3068#issuecomment-2847343712
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR description is a good start but could be improved to better meet the NuttX requirements. Here's a breakdown of what's good and what needs improvement: **Strengths:** * **Clear explanation of the "why":** The rationale behind the changes (progress reporting and RAM copy) is well-articulated. * **Detailed "how":** The implementation details, including the new functions and Kconfig options, are described sufficiently. * **Testing context provided:** You've specified your target hardware and mentioned the FBCON example application, which provides some context. * **Mention of community involvement:** Referencing the email-list discussion with @Cynerd and @michallenc is good practice. **Weaknesses and Suggested Improvements:** * **Conciseness:** The summary is a bit too verbose. Try to be more concise and focus on the key changes. Instead of explaining *why* progress reporting is needed in detail, simply state that it *is* added. The *why* is better placed in the commit message itself. * **Summary: missing functional parts affected:** Specify the affected modules (e.g., boot, drivers/mtd). * **Impact: incomplete "YES" answers:** While you've marked some sections as "YES," you haven't always provided sufficient detail. Expand on the impact of Kconfig changes on the build. Mention size increases precisely. * **Testing: Lack of concrete logs:** Providing actual log output is crucial. Showing "before" and "after" logs demonstrates the changes in behavior. Even small snippets are better than none. Consider using a simplified test case for the logs if your current logs are too verbose. * **Testing: Limited scope:** Testing on a single, custom board is insufficient. Ideally, test on a common, supported board (e.g., QEMU) to demonstrate wider applicability. If you can only test on your custom board, explain why. * **Impact on Documentation:** Even if no documentation changes are *required*, if the features are user-facing, consider whether documentation updates are *beneficial* and mark "YES" accordingly. * **Clarity on RAM copy address validation:** The concern about the inability to validate the RAM address should be highlighted more prominently. Perhaps suggest a future improvement or mitigation strategy. * **Kconfig option names:** Mention the exact names of the new Kconfig options. This will make it easier for reviewers to locate and understand their purpose. **Revised Example (Partial - focusing on improvements):** ## Summary This PR adds progress reporting and a RAM copy feature to NXboot. The `nxboot` module is primarily affected. Progress is reported via stdout (configurable via `CONFIG_NXBOOT_PROGRESS`) and can be integrated with user displays. A new function, `nxboot_ramcopy` (enabled by `CONFIG_NXBOOT_RAMCOPY`), copies the validated image to a configurable RAM address. This addresses the need for feedback during lengthy boot/upgrade processes and facilitates RAM-based booting. Related email-list discussion with @Cynerd and @michallenc. ## Impact * **New feature added?** YES (Progress reporting, RAM copy) * **Impact on user?** YES (Users can enable progress reporting and RAM copy via new Kconfig options. See documentation updates for usage.) * **Impact on build?** YES (New Kconfig options added: `CONFIG_NXBOOT_PROGRESS`, `CONFIG_NXBOOT_RAMCOPY`. Enabling `CONFIG_NXBOOT_PROGRESS` increases binary size by approximately 1.5KiB on SAMA5D27C-D1G). * **Impact on hardware?** NO * **Impact on documentation?** YES (Documentation updated to explain the new Kconfig options and their usage. Example provided for integrating progress reporting with a user display) * ... (rest of the Impact section) ## Testing **Build Host:** Linux, x86_64, GCC 9.4.0 **Target(s):** SAMA5D27C-D1G (custom board), QEMU-riscv32 (sim - basic functionality test) **Testing logs before change (QEMU-riscv32):** ``` NuttShell (NSH)> boot Booting... ... (previous boot logs) ``` **Testing logs after change (QEMU-riscv32, `CONFIG_NXBOOT_PROGRESS` enabled):** ``` NuttShell (NSH)> boot Booting... NXBoot: Starting boot process NXBoot: Validating image... NXBoot: Image validated ... (other progress messages) ``` **(Add similar before/after logs for your custom board, highlighting progress reporting on the display)** By addressing these points, your PR will be clearer, easier to review, and more likely to be accepted. Remember to fill out all sections completely and accurately. -- 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
