Skip to content

boot/nxboot: Enhancements to add progress messages and copy-to-RAM #3068

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TimJTi
Copy link
Contributor

@TimJTi TimJTi commented May 2, 2025

Summary

This PR has been created after email-list discussions mainly with with @Cynerd and @michallenc

Details

Progress Reporting

This was driven as a result of the inevitable time that firmware upgrades can take. The progress is reported via syslog, but not all embedded systems use this and, in the case (such as mine) where a user LCD is available, it is beneficial to allow progress to be displayed duringh booting and upgrades - especially should things go wrong.

To achieve this there is a new Kconfig choice - NXBOOT_PRINTF_PROGRESS which is disabled by defailt - to allow messages to be printed to stdout.

Since other users may wish to treat NXboot as a library, and perhaps handle the messages differently, the feature is implemented by calls to a specific nxboot_progress function using new enums to indicate the type of message: info, error, or that a potentially lengthy procedure has started, stopped or is in progress using the "." character printed periodically.

A user wishing to implement this in another way would use the existing top-level public function nxboot_main and would only need to modify this to report errors in a different way.

Enabling this feature adds approximately 1.5KiB to the size of the binary, in my case.

RAM copy

In my case, the ultimate boot_image function needs to copy the validated and bootable image to SDRAM. Although this could be handled by the board-specific logic, the NXboot header does not conatin the size of the actual image to be copied meaning an image copy can only be of the entire NXboot "slot" size. This means it is a little slower then it could be and it made sense to have that implemented by a new function nxboot_ramcopy before calling the board boot function via BOARDIOC_BOOT_IMAGE

This is enabled by a new Kconfig choice - NXBOOT_COPY_TO_RAM - that is disabled by default

The address to copy the image to is also configured via Kconfig - BUT there is no way that I can see for NXboot to determine the validity or otherwise of the configured address.

Impact

Since the new features are selected via Kconfig, defaulting to off, the is no intended impact to existing implementations. Lower level functions have been edited carefully, and I can confirm that NXboot still behaves for me as it did before the changes, with the added benefit of progress messages if required.

The copy-to-ram function also works, for me, as required.

Testing

This has been tested on my custom board using a SAMA5D27C-D1G processor, equipped with a partitioned 256Mbyte NOR flash that for the NXboot slots, and an 800x40 TFT for stdout courtesy of the FBCON example application

@TimJTi TimJTi force-pushed the NXBoot-Add-Progress-Reporting-and-copy-to-ram branch from d23e18a to 76a5818 Compare May 2, 2025 14:25
@nuttxpr
Copy link

nuttxpr commented May 2, 2025

[Experimental Bot, please feedback here]

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.

@TimJTi TimJTi force-pushed the NXBoot-Add-Progress-Reporting-and-copy-to-ram branch 2 times, most recently from 0750b52 to 4d613e4 Compare May 2, 2025 16:12
@TimJTi TimJTi changed the title apps/boot/nxboot: Enhancements to add progress messages and copy-to-RAM boot/nxboot: Enhancements to add progress messages and copy-to-RAM May 2, 2025
@TimJTi TimJTi force-pushed the NXBoot-Add-Progress-Reporting-and-copy-to-ram branch from 4d613e4 to 66572be Compare May 5, 2025 11:33
@TimJTi TimJTi marked this pull request as ready for review May 6, 2025 11:27
@@ -64,6 +64,17 @@ config NXBOOT_BOOTLOADER

if NXBOOT_BOOTLOADER

config NXBOOT_COPY_TO_RAM
bool "Copy bootable image to RAM before calling board boot-image function"
default n
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please include a ---help--- explaining when and why to use it, etc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

@TimJTi TimJTi force-pushed the NXBoot-Add-Progress-Reporting-and-copy-to-ram branch from 66572be to 02dde6f Compare May 6, 2025 13:50
@TimJTi TimJTi force-pushed the NXBoot-Add-Progress-Reporting-and-copy-to-ram branch from 02dde6f to f263c7c Compare May 6, 2025 19:55
@TimJTi TimJTi marked this pull request as draft May 7, 2025 11:45
@TimJTi
Copy link
Contributor Author

TimJTi commented May 7, 2025

Found a couple message enum inconsistencies so I'm sorting those out and put back to draft while I do so

…AM feature

This adds Kconfig-enabled progress messages that are output to stdout.

It also adds Kconfig-enabled option to copy the validated and bootable image to RAM

Signed-off by: Tim Hardisty <[email protected]>
@TimJTi TimJTi force-pushed the NXBoot-Add-Progress-Reporting-and-copy-to-ram branch from 7d96e90 to 43bf41b Compare May 7, 2025 12:01
@TimJTi TimJTi marked this pull request as ready for review May 7, 2025 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants