boot: zephyr: migrate CDC ACM serial recovery to USB_DEVICE_STACK_NEXT#2635
boot: zephyr: migrate CDC ACM serial recovery to USB_DEVICE_STACK_NEXT#2635JayToltTech wants to merge 1 commit intomcu-tools:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request migrates CDC ACM serial recovery from the legacy USB device stack to the new USB_DEVICE_STACK_NEXT in MCUboot. The key improvement is lazy USB initialization - USB is only initialized when serial recovery is actually triggered, avoiding interference with application USB on normal boots.
Changes:
- Adds self-contained USB CDC ACM module with configurable descriptors via Kconfig
- Implements lazy USB initialization in serial recovery path with watchdog feeding during USB host wait
- Enlarges nrf52840 boot partitions (64KB → 72KB) to accommodate larger USB stack
- Removes legacy USB stack configuration options from board files
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/readme-zephyr.md | Documents new Kconfig options for USB descriptor configuration |
| boot/zephyr/usbd_cdc_serial.h | Header for new USB CDC ACM module |
| boot/zephyr/usbd_cdc_serial.c | Self-contained USB stack setup with lazy initialization |
| boot/zephyr/usb_cdc_acm_recovery.conf | Adds product string override and flash-saving options |
| boot/zephyr/usb_cdc_acm_log_recovery.conf | Same configuration for logging variant |
| boot/zephyr/serial_adapter.c | Updates to use new USB module with lazy init and watchdog feeding |
| boot/zephyr/main.c | Adds USB cleanup on boot with -EALREADY handling for lazy init |
| boot/zephyr/boards/nrf52840dongle_nrf52840_big.overlay | New overlay with enlarged boot partition for dongle |
| boot/zephyr/boards/nrf52840dongle_nrf52840.conf | Removes legacy USB stack config options |
| boot/zephyr/boards/nrf52840_big.overlay | Enlarges boot partition from 64KB to 72KB |
| boot/zephyr/Kconfig.serial_recovery | Adds new Kconfig options for USB descriptor customization |
| boot/zephyr/CMakeLists.txt | Conditionally compiles usbd_cdc_serial.c |
| boot/zephyr/sample.yaml | Updates test configuration to use new overlay for dongle |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1bdfcc1 to
41b043d
Compare
|
Same as with #2346, we cannot merge this without also migrating the USB DFU support the device_next. Otherwise, you will have a mix of dependencies, some of the MCUBoot code depending on the legacy stack, other on the new stack. |
|
I am going to ask for consideration to review and approve the CDC-ACM work while I make progress on the DFU USBD port. I am just fine with DNM (I cannot apply this myself, would someone else please do?) while we get both sets of changes in place. This is a larger change and I am a relatively new contributor, so I would like to build the experience of finalizing a larger PR with few pages of code as well as to manage the scope of work by developing smaller pieces one at a time. |
There was a problem hiding this comment.
think there is some confusion here, the CDC ACM porting work was already done in https://github.com/mcu-tools/mcuboot/pull/2346/files, that commit should be used as-is, the additional part that is needed is USB DFU support which needs to be ported (comments below were left before posting this message, so can be ignored, just pull in the existing commit from that PR, which I assume works?)
| config BOOT_SERIAL_CDC_ACM_SELF_POWERED | ||
| bool "USB self-powered attribute for CDC ACM serial recovery" | ||
| depends on BOOT_SERIAL_CDC_ACM | ||
| help | ||
| Set the Self-powered attribute in the USB configuration descriptor. | ||
| When enabled, the device reports that it does not draw power from | ||
| the USB bus. Only enable this if the device is powered by an |
There was a problem hiding this comment.
actually bit confused on all these options, they already exist in zephyr? CDC_ACM_SERIAL_SELF_POWERED, CDC_ACM_SERIAL_MANUFACTURER_STRING, CDC_ACM_SERIAL_PRODUCT_STRING, CDC_ACM_SERIAL_VID, CDC_ACM_SERIAL_PID?
There was a problem hiding this comment.
It is correct that these are defined in https://github.com/zephyrproject-rtos/zephyr/blob/main/subsys/usb/device_next/app/Kconfig.cdc_acm_serial, but I am not sure if these are to be used outside the Zephyr tree? The comment in https://github.com/zephyrproject-rtos/zephyr/blob/main/subsys/usb/device_next/app/cdc_acm_serial.c seems to indicate that their usage is limited:
/* * This is intended for use with cdc-acm-snippet or as a default serial backend * only in applications where no other USB features are required, configured, * and enabled. This code only registers the first CDC-ACM instance. */
There was a problem hiding this comment.
When it comes to USB DFU, MCUboot will have to define similar Kconfigs on its own anyway (and reviewing this without seeing how these two "sets" of Kconfig options will co-exist is difficult at best).
There was a problem hiding this comment.
Right, if you look in that source, you see that CDC_ACM_SERIAL_SELF_POWERED is conditionally included if CDC_ACM_SERIAL_INITIALIZE_AT_BOOT.
This design does not use CDC_ACM_SERIAL_INITIALIZE_AT_BOOT because that causes the USBD enable and disable code to be run, even in the happy case when mcuboot runs and sees that slot0 is valid and recovery/dfu are not needed.
There was a problem hiding this comment.
We could start with a refactoring of Zephyr's USBD CDC_ACM_SERIAL code, in order to cleanly support lazy initialization better, but...
There was a problem hiding this comment.
I honestly think MCUboot should implement this on the application layer itself.
There was a problem hiding this comment.
The description seems apt for MCUboot, it would only use one instance of it, but self powered being gated on initialising at boot seems a bit of a random condition
|
Thank you for the review @nordicjm .
I would recommend against that. There are a number of issues with that PR which I fixed when cherry picking those files into this branch, including:
And probably one of two other small things I fixed along the way. I will address the rest of your feedback once we have consensus that we are going to move this PR forward instead of go back to #2346. |
Neither #2346 nor this PR can be merged as they are incomplete in the sense that USB DFU was not ported yet. I think reviewing this without seeing how USB DFU would be integrated along side the CDC-ACM changes brings little value, as we have already seen the major part of this work in #2346? |
Please mark this PR DNM (I cannot) and we can remove the DNM tag once both CDC-ACM and DFU are updated. I am committed to providing both sets of functionality with the caveat that I'm not forced to maintain a fork of mcuboot for my own project because I cannot integrate the USBD CDC-ACM port I have completed and tested. As we can see, we have a number of things to work through along the way. So I will start my DFU work once we have a clear path to approving the CDC-ACM work. I am requesting that we work through the CDC-ACM issues and reach resolution. Thank you. |
41b043d to
6646409
Compare
d1ff913 to
1ab3b73
Compare
|
@nordicjm I went ahead and addressed your prior round of review feedback. I will pause here and will join the mcuboot tech forum call next Thursday to be available for discussion. |
Replace the legacy USB device stack with USB_DEVICE_STACK_NEXT for CDC-ACM serial recovery. USB is initialized lazily in boot_uart_fifo_init() only when serial recovery is triggered, avoiding interference with application USB on normal boots. Add self-contained USBD CDC ACM module (usbd_cdc_serial.c/.h) that manages device context, descriptors, and configuration registration. Expose configurable USB descriptors (VID, PID, manufacturer, product, self-powered, max power) through new Kconfig options under BOOT_SERIAL_CDC_ACM. Feed the watchdog while waiting for USB host connection to prevent hardware watchdog timeout before the serial recovery loop is reached. Update nrf52840 board files: enlarge boot partitions for the larger USB stack, remove stale legacy USB Kconfig options, and add flash- saving options (disable BOS, vendor requests, deferred messages). Signed-off-by: Jay Beavers <jay@tolttechnologies.com>
1ab3b73 to
9a39b38
Compare
Summary
USB_DEVICE_STACK_NEXTfor CDC-ACM serial recoveryboot_uart_fifo_init()only when serial recovery is triggered, avoiding interference with application USB on normal bootsusbd_cdc_serial.c/.h) with configurable USB descriptors (VID, PID, manufacturer, product, self-powered, max power) via new Kconfig optionsSupersedes #2634 (narrowed scope to CDC-ACM only, removed unrelated logging/SWO changes).
Builds on the approach from #2346 with key improvements:
CDC_ACM_SERIAL_INITIALIZE_AT_BOOTSTRUCT_SECTION_GETindexTest plan
usb_cdc_acm_log_recovery.conf(92.49% flash)usb_cdc_acm_recovery.conf(99.71% flash)