usb: device_next: implement USB DFU class for the new device support #79794

jfischer-no · 2024-10-14T10:51:04Z

Add USB DFU class implementation for the USB device support.
Add flash backend and sample.

carlescufi · 2024-10-14T11:24:44Z

subsys/usb/device_next/class/usbd_dfu_flash.c

@@ -0,0 +1,153 @@
+/*


@de-nordic @nordicjm could you review this file?

nordicjm

Testing on an nrf52840dk:

This allows downloading of slot data, but does not enable MCUboot, so this doesn't make sense (sysbuild.conf with SB_CONFIG_BOOTLOADER_MCUBOOT=y)
It seems to time out on the try, finishes, then locks up:

sudo dfu-util -d "2fe3:0005" -a 1 -U test.bin
dfu-util 0.11

Copyright 2005-2009 Weston Schmidt, Harald Welte and OpenMoko Inc.
Copyright 2010-2021 Tormod Volden and Stefan Schmidt
This program is Free Software and has ABSOLUTELY NO WARRANTY
Please report bugs to http://sourceforge.net/p/dfu-util/tickets/

Opening DFU capable USB device...
Device ID 2fe3:0005
Device DFU version 0110
Claiming USB DFU (Run-Time) Interface...
Setting Alternate Interface zero...
Determining device status...
DFU state(0) = appIDLE, status(0) = No error condition is present
Device really in Run-Time Mode, send DFU detach request...
Device will detach and reattach...
Opening DFU USB Device...
Claiming USB DFU Interface...
Setting Alternate Interface #1 ...
Determining device status...
DFU state(2) = dfuIDLE, status(0) = No error condition is present
DFU mode device DFU version 0110
Device returned transfer size 512
Copying data from DFU device to PC
Upload  [======================== ]  99%       351744 bytesdfu-util: 
Error during upload (LIBUSB_ERROR_TIMEOUT)
Upload  [======================== ]  99%       479232 bytes
Received a total of 479232 bytes

At this point, it won't work again:

sudo dfu-util -d "2fe3:0005" -a 1 -U test.bin
dfu-util 0.11

Copyright 2005-2009 Weston Schmidt, Harald Welte and OpenMoko Inc.
Copyright 2010-2021 Tormod Volden and Stefan Schmidt
This program is Free Software and has ABSOLUTELY NO WARRANTY
Please report bugs to http://sourceforge.net/p/dfu-util/tickets/

dfu-util: Broken LANGID string descriptor
Opening DFU capable USB device...
Device ID 2fe3:ffff
Device DFU version 0110
Claiming USB DFU Interface...
Setting Alternate Interface #1 ...
Determining device status...
dfu-util: error get_status: LIBUSB_ERROR_TIMEOUT

And after this the device name part fails:

sudo dfu-util -l
dfu-util 0.11

Copyright 2005-2009 Weston Schmidt, Harald Welte and OpenMoko Inc.
Copyright 2010-2021 Tormod Volden and Stefan Schmidt
This program is Free Software and has ABSOLUTELY NO WARRANTY
Please report bugs to http://sourceforge.net/p/dfu-util/tickets/

dfu-util: Failed to retrieve language identifiers
dfu-util: Failed to retrieve language identifiers
Found DFU: [2fe3:ffff] ver=0307, devnum=22, cfg=1, intf=0, path="3-3", alt=1, name="UNKNOWN", serial="UNKNOWN"
Found DFU: [2fe3:ffff] ver=0307, devnum=22, cfg=1, intf=0, path="3-3", alt=0, name="UNKNOWN", serial="UNKNOWN"

I only have one USB cable so it's possible that there is an error on the UART and I can't see it

Same story with the RAM disk:

sudo dfu-util -d "2fe3:0005" -a 0 -U test.bin
dfu-util 0.11

Copyright 2005-2009 Weston Schmidt, Harald Welte and OpenMoko Inc.
Copyright 2010-2021 Tormod Volden and Stefan Schmidt
This program is Free Software and has ABSOLUTELY NO WARRANTY
Please report bugs to http://sourceforge.net/p/dfu-util/tickets/

Opening DFU capable USB device...
Device ID 2fe3:ffff
Device DFU version 0110
Claiming USB DFU Interface...
Setting Alternate Interface #0 ...
Determining device status...
DFU state(2) = dfuIDLE, status(0) = No error condition is present
DFU mode device DFU version 0110
Device returned transfer size 512
Copying data from DFU device to PC
Upload  [======================== ]  99%          512 bytesdfu-util: 
Error during upload (LIBUSB_ERROR_TIMEOUT)
Upload  [======================== ]  99%        16384 bytes
Received a total of 16384 bytes

sudo dfu-util -d "2fe3:0005" -a 0 -U test.bin
dfu-util 0.11

Copyright 2005-2009 Weston Schmidt, Harald Welte and OpenMoko Inc.
Copyright 2010-2021 Tormod Volden and Stefan Schmidt
This program is Free Software and has ABSOLUTELY NO WARRANTY
Please report bugs to http://sourceforge.net/p/dfu-util/tickets/

dfu-util: Broken LANGID string descriptor
Opening DFU capable USB device...
Device ID 2fe3:ffff
Device DFU version 0110
Claiming USB DFU Interface...
Setting Alternate Interface #0 ...
Determining device status...
dfu-util: error get_status: LIBUSB_ERROR_TIMEOUT

nordicjm · 2024-10-15T12:56:05Z

Offsets make no sense, the file you get from the device, supposedly for slot0, has lots of 0xff's then at 0x1736c has "mutex_free called", if I read the code from the device using nrfjprog and search, I find one result at 0x2236c, which is a 0xb000 difference, this doesn't make sense. Can see here from the zephyr.dts file that the application starts at 0xc000:

                flash_controller: flash-controller@4001e000 {
                        compatible = "nordic,nrf52-flash-controller";
                        reg = < 0x4001e000 0x1000 >;
                        partial-erase;
                        #address-cells = < 0x1 >;
                        #size-cells = < 0x1 >;
                        flash0: flash@0 {
                                compatible = "soc-nv-flash";
                                erase-block-size = < 0x1000 >;
                                write-block-size = < 0x4 >;
                                reg = < 0x0 0x100000 >;
                                partitions {
                                        compatible = "fixed-partitions";
                                        #address-cells = < 0x1 >;
                                        #size-cells = < 0x1 >;
                                        boot_partition: partition@0 {
                                                label = "mcuboot";
                                                reg = < 0x0 0xc000 >;
                                        };
                                        slot0_partition: partition@c000 {
                                                label = "image-0";
                                                reg = < 0xc000 0x77000 >;
                                        };
                                        slot1_partition: partition@83000 {
                                                label = "image-1";
                                                reg = < 0x83000 0x75000 >;
                                        };
                                        storage_partition: partition@f8000 {
                                                label = "storage";
                                                reg = < 0xf8000 0x8000 >;
                                        };
                                };
                        };
                };

include/zephyr/usb/class/usbd_dfu.h

tmon-nordic · 2024-10-15T13:13:38Z

subsys/usb/device_next/class/usbd_dfu.c

+ * It is very unlikely that anyone would need more than one instance of the DFU
+ * class. Therefore, we make an exception here and do not support multiple
+ * instances, which allows us to have a much simpler implementation.


Unsure if this will bite us later or not. One example that comes to mind is device with two independent USB device controllers running separate instances of USB stack. The example may be a bit far stretched, but there are SoCs on the market with hardware capable for running two separate USB device stack instances at the same time.

I have thought about that. DFU is not a normal operating mode, I think it is quite unlikely that there will be a need to have two USB devices on the same SoC with DFU capabilities.

subsys/usb/device_next/class/Kconfig.dfu

subsys/usb/device_next/class/usbd_dfu.c

subsys/usb/device_next/class/usbd_dfu_flash.c

subsys/usb/device_next/usbd_class.c

jfischer-no · 2024-10-22T13:43:52Z

Testing on an nrf52840dk:

* This allows downloading of slot data, but does not enable MCUboot, so this doesn't make sense (sysbuild.conf with `SB_CONFIG_BOOTLOADER_MCUBOOT=y`)

Yes, MCUboot is just some bootloader library, never really integrated into the Zephyr project. You have built the example with CONFIG_BOOTLOADER_MCUBOOT=y if you want to try it.

* It seems to time out on the try, finishes, then locks up:

Yes, there is a bug in USBD controller driver.

nordicjm · 2024-10-22T14:08:32Z

Yes, MCUboot is just some bootloader library, never really integrated into the Zephyr project. You have built the example with CONFIG_BOOTLOADER_MCUBOOT=y if you want to try it.

This is completely false, it is integrated with zephyr, hence why doing west build -b <board> --sysbuild -- -DSB_CONFIG_BOOTLOADER_MCUBOOT=y will build the application and MCUboot, and sign the image for you for use with sysbuild

nordicjm

Issues:

Needs a sysbuild.conf file with SB_CONFIG_BOOTLOADER_MCUBOOT=y in
~~Does not work if USB cable is attached when application starts, it needs to be removed then re-inserted~~

jfischer-no · 2024-10-23T10:27:15Z

Issues:

* [ ]  Needs a `sysbuild.conf` file with `SB_CONFIG_BOOTLOADER_MCUBOOT=y` in

Why is it needed and where do you get these requirements from?

* [ ]  Does not work if USB cable is attached when application starts, it needs to be removed then re-inserted

What application? Please give me some more information about what you are trying to do.

nordicjm · 2024-10-23T10:42:16Z

Why is it needed and where do you get these requirements from?

You have a sample here that writes slot1 partition, so what use is building a sample that can't actually be used (i.e. without mcuboot)? You added slot1 to dfu, so doing an upload should work and be testable by users

What application? Please give me some more information about what you are trying to do.

The sample here? Connect nrf5340dk up using both ports, build and flash, then see after boot that there is no USB device detected on the host PC, then unplug only the nRF USB port and plug it back in, now it appears

jfischer-no · 2024-10-23T11:10:56Z

Why is it needed and where do you get these requirements from?

You have a sample here that writes slot1 partition, so what use is building a sample that can't actually be used (i.e. without mcuboot)? You added slot1 to dfu, so doing an upload should work and be testable by users

First of all, this is a sample, not an application, and MCUboot is just some bootloader library. There is no need to bind the USB DFU to a specific bootloader library. The user can simply test the upload/download of the slot 1 image or the RAMdisk using dfu-utils. The user can have similar functionality as to the legacy sample by using "CONFIG_BOOTLOADER_MCUBOOT=y".
I have to ask again, where did you get the requirement for "Needs a sysbuild.conf file with SB_CONFIG_BOOTLOADER_MCUBOOT=y"?

carlescufi · 2024-11-06T11:17:46Z

Does not work if USB cable is attached when application starts, it needs to be removed then re-inserted

This is a bug in the usbreg driver in nrfx that will be solved later and only affects nRF5340, nothing to do with the USB DFU class. Please use an nRF52840 instead.

tejlmand · 2024-11-06T11:53:43Z

First of all, this is a sample, not an application, and MCUboot is just some bootloader library.

Yes, but MCUboot is officially supported and integrated with Zephyr.

There is no need to bind the USB DFU to a specific bootloader library.

and adding a sysbuild.conf with SB_CONFIG_BOOTLOADER_MCUBOOT=y doesn't bind the USB DFU to a specific bootloader. It simply means that if you build this sample using sysbuild, then mcuboot will be enabled per default in the multi-image build, thus giving a much better end-user experience out-of-the box.

Users are still free to do:

build the sample without sysbuild, meaning MCUboot will not be enabled.
Note, this is default behavior for west build in Zephyr.
If using sysbuild, then users can still decide to not include MCUboot by doing:
west build ... --sysbuild -- -DSB_CONFIG_BOOTLOADER_NONE=y
(or enable another bootloader of their choice if they have made one such available in a downstream Zephyr module)

The user can simply test the upload/download of the slot 1 image or the RAMdisk using dfu-utils.

and nothing prevents the user from doing so, even if you include a sysbuild.conf.

I have to ask again, where did you get the requirement for "Needs a sysbuild.conf file with SB_CONFIG_BOOTLOADER_MCUBOOT=y"?

What's the problem in making a better out-of-the-box experience for users testing this sample by supporting automatic inclusion of MCUboot when building with sysbuild ?

A lot of good and valuable review comments cannot be traced back to a specific requirement, but they are still accepted by PR authors to provide an even better user experience.

Just like a lot of PRs in Zephyr itself are opened without a specific requirement for the feature proposed.

kartben · 2025-01-30T10:36:48Z

samples/subsys/usb/dfu-next/README.rst

+   west build -b reel_board zephyr/samples/hello_world -d build-hello_world -- \
+   -DCONFIG_BOOTLOADER_MCUBOOT=y \
+   '-DCONFIG_MCUBOOT_SIGNATURE_KEY_FILE="bootloader/mcuboot/root-rsa-2048.pem"'


just noticed but this should use zephyr-app-commands (unless there is a reason you couldn't but doesn't look like it)
can be addressed as a follow-up

@kartben I copied it from the old sample, I guess it was used because the gen-args line would be to long, but something like that seems to work.

-.. code-block:: console - - west build -b reel_board zephyr/samples/hello_world -d build-hello_world -- \ - -DCONFIG_BOOTLOADER_MCUBOOT=y \ - '-DCONFIG_MCUBOOT_SIGNATURE_KEY_FILE="bootloader/mcuboot/root-rsa-2048.pem"' +.. zephyr-app-commands:: + :app: zephyr/samples/hello_world + :board: reel_board + :gen-args: -DCONFIG_MCUBOOT_SIGNATURE_KEY_FILE=\"bootloader/mcuboot/root-rsa-2048.pem\" + -DCONFIG_BOOTLOADER_MCUBOOT=y + :goals: flash

?

Yeah, this should be the way to have directive options span on multiple lines but it looks like the \n between the two options is being kept in the output so there might be an issue in the zephyr-app-commands code. Like I said, for a follow-up anyway (although tbh I wouldn't worry about going over 100 characters line limit in the case of sphinx "code")

I had to push anyway, so I changed it.

samples/subsys/usb/dfu-next/CMakeLists.txt

henrikbrixandersen · 2025-01-30T12:21:19Z

subsys/usb/device_next/class/usbd_dfu.c

+	.init = dfu_mode_init,
+};
+
+USBD_DEFINE_CLASS(dfu_dfu, &dfu_api, &dfu_data, NULL);


Non-blocking wish:

It would be nice to be able to only enable the run-time part, allowing an application to delegate the actual DFU part to a USB DFU-capable bootloader. This would especially be handy for smaller MCUs with a built-in ROM USB DFU bootloader.

As far as I can tell, this could be done with just a boolean Kconfig for enabling the actual DFU part (default y). What do you think?

I have an intention for a plan to allow USB device configuration to be very minimal, just able to run USB DFU, then add simple bootloader (very unlikely MCUboot) sample to use it the way you described. Not sure how low we can go with flash usage, but something under 32kB should be the goal.

Sounds good. We can follow-up with a PR for allowing just the run-time part at a later point in time.

jfischer-no · 2025-01-30T17:05:08Z

@nordicjm, please reconsider your -1.

kartben · 2025-01-30T17:08:42Z

samples/subsys/usb/dfu-next/README.rst

+
+.. zephyr-app-commands::
+   :zephyr-app: samples/subsys/usb/dfu-next
+   :board: reel_board


BTW I am assuming you have tested this on the reel_board? :) In general I think it would make more sense that the board here (and other places in the README) would be one of the targets you have as integration_platforms
Definitely not worth changing at this point, especially if you're confident this works on reel_board :)

This is my favorite board, IMHO the best board ever.

Given that @jfischer-no was the original designer of the reel_board I expect he has tested it with it :)

Yeah of course, but joke aside I think it's just good practice that we would actually let CI help us keep our docs honest :)

I am dismissing your -1 as sysbuild and sysbuild file is not requried feature. The steps to use this sample with sysbuild are documented.

In the usbd_register_all_classes(), we may need to skip some instances as they may have very specific function like USB DFU "DFU mode" which should not be available by default. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>

This new implementation is written from scratch and is not tied to the image manager and MCUboot. It allows the user to define their own backend and use a simple macro to instantiate an image. On the USB side this is represented by an interface. The number of possible images is configurable using the Kconfig option, and is a fairly inexpensive approach since it only changes the size of the pointer array. The number of images is only limited by the number of possible interfaces in a configuration. The class implementation does not support multiple instances, as there is no real use for it. However, it does provide two class instances, one for runtime mode and one for DFU mode. The switch from runtime to DFU mode can only be performed by the user application, i.e. the application receives a notification when the host wants to switch to DFU mode, and then the application can disable the runtime configuration and enable the DFU configuration. This implementation does not support switching to the DFU mode by bus reset issued by the host. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>

Add a simpler flash backend, similar to what we have in the legacy USB DFU implementation. Support slot-0 and slot-1 flash partitions, but allow them to be enabled/disabled via Kconfig options. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>

The sample defines an image that uses RAMdisk and also provides an option to enable USB DFU flash backend for the "slot-1" flash partition. Assuming the user runs this sample from the slot-0 partition, uploading to flash should work fine for evaluation purposes. If the sample is built with CONFIG_BOOTLOADER_MCUBOOT=y, the sample will mark the image in slot 1 as pending after an image download. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>

jfischer-no · 2025-01-31T21:22:43Z

#81197 has been merged, rebased to resolve conflicts.

jfischer-no added area: USB Universal Serial Bus Experimental Experimental features not enabled by default labels Oct 14, 2024

jfischer-no requested review from carlescufi and tmon-nordic October 14, 2024 10:51

jfischer-no mentioned this pull request Oct 14, 2024

Planned USB device/host support rework/enhancements #42066

Open

57 tasks

zephyrbot added the area: Samples Samples label Oct 14, 2024

zephyrbot requested review from kartben and nashif October 14, 2024 10:51

zephyrbot assigned kartben Oct 14, 2024

carlescufi reviewed Oct 14, 2024

View reviewed changes

subsys/usb/device_next/class/usbd_dfu_flash.c

@@ -0,0 +1,153 @@

/*

Copy link

Member

carlescufi Oct 14, 2024

Choose a reason for hiding this comment

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

@de-nordic @nordicjm could you review this file?

nordicjm requested review from nordicjm and de-nordic October 15, 2024 12:29

nordicjm reviewed Oct 15, 2024

View reviewed changes

tmon-nordic requested changes Oct 18, 2024

View reviewed changes

henrikbrixandersen self-requested a review October 20, 2024 09:55

jfischer-no force-pushed the pr-device_next_dfu branch from 240dfd2 to f2bdaf2 Compare October 22, 2024 16:56

carlescufi requested review from nordicjm and tmon-nordic October 23, 2024 09:56

nordicjm previously requested changes Oct 23, 2024

View reviewed changes

jfischer-no force-pushed the pr-device_next_dfu branch 2 times, most recently from 96febfc to 9c208c7 Compare October 25, 2024 12:43

jfischer-no mentioned this pull request Nov 1, 2024

drivers: udc: kinetis/usbd: fixes for v4.0.0 #80727

Merged

kartben previously approved these changes Jan 30, 2025

View reviewed changes

jfischer-no dismissed kartben’s stale review via 7d98bc4 January 30, 2025 11:00

jfischer-no force-pushed the pr-device_next_dfu branch 2 times, most recently from 7d98bc4 to 234d31b Compare January 30, 2025 11:07

henrikbrixandersen requested changes Jan 30, 2025

View reviewed changes

jfischer-no force-pushed the pr-device_next_dfu branch from 234d31b to 85bf7f6 Compare January 30, 2025 12:47

henrikbrixandersen previously approved these changes Jan 30, 2025

View reviewed changes

tmon-nordic previously approved these changes Jan 30, 2025

View reviewed changes

kartben previously approved these changes Jan 30, 2025

View reviewed changes

jfischer-no requested a review from nordicjm January 30, 2025 17:06

kartben reviewed Jan 30, 2025

View reviewed changes

jfischer-no added 4 commits January 31, 2025 22:14

jfischer-no dismissed stale reviews from kartben, tmon-nordic, and henrikbrixandersen via 67bec6b January 31, 2025 21:17

jfischer-no force-pushed the pr-device_next_dfu branch from 85bf7f6 to 67bec6b Compare January 31, 2025 21:17

kartben added the DNM This PR should not be merged (Do Not Merge) label Jan 31, 2025

kartben approved these changes Jan 31, 2025

View reviewed changes

henrikbrixandersen approved these changes Jan 31, 2025

View reviewed changes

carlescufi added DNM This PR should not be merged (Do Not Merge) and removed DNM This PR should not be merged (Do Not Merge) labels Feb 1, 2025

kartben removed the DNM This PR should not be merged (Do Not Merge) label Feb 4, 2025

kartben merged commit 694c342 into zephyrproject-rtos:main Feb 4, 2025
29 of 31 checks passed

jfischer-no deleted the pr-device_next_dfu branch February 4, 2025 11:17

usb: device_next: implement USB DFU class for the new device support #79794

usb: device_next: implement USB DFU class for the new device support #79794

Uh oh!

Conversation

jfischer-no commented Oct 14, 2024 • edited by kartben Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nordicjm left a comment

Choose a reason for hiding this comment

Uh oh!

nordicjm commented Oct 15, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jfischer-no commented Oct 22, 2024

Uh oh!

nordicjm commented Oct 22, 2024

Uh oh!

nordicjm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfischer-no commented Oct 23, 2024

Uh oh!

nordicjm commented Oct 23, 2024

Uh oh!

jfischer-no commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlescufi commented Nov 6, 2024

Uh oh!

tejlmand commented Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfischer-no Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfischer-no commented Jan 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfischer-no commented Jan 31, 2025

Uh oh!

Uh oh!

Uh oh!

jfischer-no commented Oct 14, 2024 •

edited by kartben

Loading

nordicjm left a comment •

edited

Loading

jfischer-no commented Oct 23, 2024 •

edited

Loading

tejlmand commented Nov 6, 2024 •

edited

Loading

jfischer-no Jan 30, 2025 •

edited

Loading