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.

tmon-nordic · 2024-10-15T13:17:02Z

subsys/usb/device_next/class/Kconfig.dfu

+
+config USBD_DFU_NUMOF_IMAGES
+	int "Number of possible DFU images"
+	range 1 255


The comment in DFU class instance says the range is 0..255 with maximum being 256 images. Should this range be changed to range 1 256?

@jfischer-no you put a thumbs up but the code is still unchanged. Should it be range 1 256 or range 1 255?

Thanks, should be fixed now.

subsys/usb/device_next/class/usbd_dfu.c

subsys/usb/device_next/class/usbd_dfu_flash.c

tmon-nordic · 2024-10-18T13:16:17Z

subsys/usb/device_next/usbd_class.c

+static bool is_disallowed(const struct usbd_class_node *const c_nd)
+{
+	static const char *const list[] = {
+		"dfu_dfu",
+	};
+
+	for (int i = 0; i < ARRAY_SIZE(list); i++) {
+		if (strcmp(c_nd->c_data->name, list[i]) == 0) {
+			return true;
+		}
+	}
+
+	return false;
+}


This just doesn't seem right. This is essentially a blocklist for usbd_register_all_classes() which is not even documented.

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.

jfischer-no · 2025-01-23T14:03:22Z

You're implementing a device firmware update sample which, you know, someone would want to use to do something like a device firmware update, for which MCUboot is used in zephyr to do that.

No, it is the sample for USB DFU class implementation. This sample also supports flash backend and provides information on how to build and test it with MCUboot. Again, as you continue to make false assumptions here, support for MCUboot is outside the Zephyr project, there is no porting layer in Zephyr for MCUboot. There is no rule to support only MCUboot or to use only MCUboot for firmware updates.

Someone really doesn't care using a device firmware update sample to upload to RAM, what use is that?

The only thing that might be of use for is cmsis dap, and this doesn't even provide an anywhere near working demo for that, so what value is this sample if it doesn't provide a usable sample?

To load firmware into RAM, of course. Like we have MSC sample using RAMdisk per default. There are even people who run code in RAM. Or to provide an example of how to implement your own backend to whatever memory, bootloader, subsystem, as other samples provide an example. Users have asked for this. Finally, this allows users to try out implementation and host tools without using bootloader and flash, avoiding all the nightmares with boards flash configurations and exclusions in the legacy sample.

If you want to make something that has no use, make it a test. What you're saying here is it would be perfectly fine to have a USB HCI sample that completely fakes the hci part of it and if someone wanted to use a real HCI device, they'd need to take a sample that isn't a useful sample then add in all the real HCI parts themselves, and last I checked the USB HCI sample was a fully working sample...

No, I did not say that. That is your assumption. This sample provides a simple example to try the USB DFU implementation and a more advanced configuration to try it with the flash backend. The documentation provides examples for these use cases, even how to build with sysbuild. I have read nothing useful from you here, absolutely nothing, you are circling around with your false assumptions. So please either read the code and documentation and provide some useful feedback or just dismiss your review and let this move forward.

nordicjm · 2025-01-23T14:06:34Z

You're implementing a device firmware update sample which, you know, someone would want to use to do something like a device firmware update, for which MCUboot is used in zephyr to do that.

No, it is the sample for USB DFU class implementation. This sample also supports flash backend and provides information on how to build and test it with MCUboot. Again, as you continue to make false assumptions here, support for MCUboot is outside the Zephyr project, there is no porting layer in Zephyr for MCUboot. There is no rule to support only MCUboot or to use only MCUboot for firmware updates.

Ping @kartben on this, this is now what, the 3rd time you've made comments that are flat out wrong, the first time was in the dev working group where you said similar things and I'm getting tired of hearing these wrong comments. MCUboot is integrated into zephyr, it is the default and only supported cross device bootloader in tree (not to say it's the only bootloader zephyr can use) and therefore is the default

jfischer-no · 2025-01-23T14:19:01Z

You're implementing a device firmware update sample which, you know, someone would want to use to do something like a device firmware update, for which MCUboot is used in zephyr to do that.

No, it is the sample for USB DFU class implementation. This sample also supports flash backend and provides information on how to build and test it with MCUboot. Again, as you continue to make false assumptions here, support for MCUboot is outside the Zephyr project, there is no porting layer in Zephyr for MCUboot. There is no rule to support only MCUboot or to use only MCUboot for firmware updates.

Ping @kartben on this, this is now what, the 3rd time you've made comments that are flat out wrong, the first time was in the dev working group where you said similar things and I'm getting tired of hearing these wrong comments. MCUboot is integrated into zephyr, it is the default and only supported cross device bootloader in tree (not to say it's the only bootloader zephyr can use) and therefore is the default

Porting layer for MCUboot is https://github.com/mcu-tools/mcuboot/tree/main/boot/zephyr, what is not Zephyr project repository, unlike other modules, e.g. LVGL, there is no zephyr/modules/mcuboot, what is against "Zephyr modules should not contain code that is written exclusively for Zephyr. Instead, such code should be contributed to the main zephyr tree.". So it is other way around, Zephyr is integrated in MCUboot. You know that, your are one of the contributor. So, again do you @nordicjm have anything to contribute to USB device support?

nashif · 2025-01-23T16:07:09Z

agree with @jfischer-no, USB-DFU class feature deserves its own sample and is not related with any existing update features available through mcuboot or anything else.

nordicjm · 2025-01-24T07:51:46Z

You're implementing a device firmware update sample which, you know, someone would want to use to do something like a device firmware update, for which MCUboot is used in zephyr to do that.

No, it is the sample for USB DFU class implementation. This sample also supports flash backend and provides information on how to build and test it with MCUboot. Again, as you continue to make false assumptions here, support for MCUboot is outside the Zephyr project, there is no porting layer in Zephyr for MCUboot. There is no rule to support only MCUboot or to use only MCUboot for firmware updates.

Ping @kartben on this, this is now what, the 3rd time you've made comments that are flat out wrong, the first time was in the dev working group where you said similar things and I'm getting tired of hearing these wrong comments. MCUboot is integrated into zephyr, it is the default and only supported cross device bootloader in tree (not to say it's the only bootloader zephyr can use) and therefore is the default

Porting layer for MCUboot is https://github.com/mcu-tools/mcuboot/tree/main/boot/zephyr, what is not Zephyr project repository

This is the equivalent of trying to classify a fish by looking at 1 pixel and nothing more, MCUboot is a zephyr application with it's own infrastructure, development/release cycle and development team/organisation supporting more than just zephyr, there's plenty of "porting layer" files inside of zephyr: https://github.com/zephyrproject-rtos/zephyr/tree/main/subsys/dfu https://github.com/zephyrproject-rtos/zephyr/tree/main/subsys/mgmt/mcumgr/grp/img_mgmt https://github.com/zephyrproject-rtos/zephyr/blob/main/cmake/mcuboot.cmake so no you are wrong

agree with @jfischer-no, USB-DFU class feature deserves its own sample and is not related with any existing update features available through mcuboot or anything else.

This is not a sample really, it's useless. In the default configuration you an upload a new firmware image that you can never use, you can also upload to a ramdisk image which, again, you can never use, so if there is to be such a project it should go in tests. This is on par with having a low power sample where when you open the main.c file it's just a comment "write the code yourself"

jfischer-no · 2025-01-24T10:02:26Z

Porting layer for MCUboot is https://github.com/mcu-tools/mcuboot/tree/main/boot/zephyr, what is not Zephyr project repository

This is the equivalent of trying to classify a fish by looking at 1 pixel and nothing more,

I can do that, I am a fish expert, I have it in my last name, even in the avatar is one.

MCUboot is a zephyr application with it's own infrastructure, development/release cycle and development team/organisation supporting more than just zephyr, there's plenty of "porting layer" files inside of zephyr: https://github.com/zephyrproject-rtos/zephyr/tree/main/subsys/dfu https://github.com/zephyrproject-rtos/zephyr/tree/main/subsys/mgmt/mcumgr/grp/img_mgmt https://github.com/zephyrproject-rtos/zephyr/blob/main/cmake/mcuboot.cmake so no you are wrong

Nope, that are dependencies (although DFU subsystem can be used with different bootloader library). This does not make it better, but it does make it more obvious that the MCUboot module violates Zephyr modules should not contain code that is written exclusively for Zephyr. Instead, such code should be contributed to the main zephyr tree..

agree with @jfischer-no, USB-DFU class feature deserves its own sample and is not related with any existing update features available through mcuboot or anything else.

This is not a sample really, it's useless. In the default configuration you an upload a new firmware image that you can never use, you can also upload to a ramdisk image which, again, you can never use, so if there is to be such a project it should go in tests. This is on par with having a low power sample where when you open the main.c file it's just a comment "write the code yourself"

The sample allows easy access for the user, it provides easy configuration to build and flash sample quickly without any dependencies but only small portion or RAM and get them familiar with USB DFU implementation and host tools. This is not what tests should do. There is also another option to build the sample with the flash backend enabled (flash backend uses the DFU subsystem which does not really have MCUboot dependencies) and if the user wants they can use it with MCUboot.
To summarize, the sample provides an overview of how to use the USB DFU implementation and it provides an example of how to use the flash backend provided by the USB DFU implementation (reproducing the same behavior as the legacy sample), thus fulfilling the sample criteria https://docs.zephyrproject.org/latest/samples/sample_definition_and_criteria.html#definition-and-criteria.

I still do not see any valuable contribution from you in this review. Do you @nordicjm have anything to contribute to USB device support?

carlescufi · 2025-01-24T11:23:32Z

Nope, that are dependencies (although DFU subsystem can be used with different bootloader library). This does not make it better, but it does make it more obvious that the MCUboot module violates Zephyr modules should not contain code that is written exclusively for Zephyr. Instead, such code should be contributed to the main zephyr tree..

Obviously that only applies to libraries, not applications. MCUboot is a special case because it is a Zephyr application, not a 3rd-party library, and it is quite obvious that you cannot write a Zephyr applications without using Zephyr code and APIs. I will send a doc patch to fix this.

jfischer-no · 2025-01-24T11:46:07Z

Nope, that are dependencies (although DFU subsystem can be used with different bootloader library). This does not make it better, but it does make it more obvious that the MCUboot module violates Zephyr modules should not contain code that is written exclusively for Zephyr. Instead, such code should be contributed to the main zephyr tree..

Obviously that only applies to libraries, not applications. MCUboot is a special case because it is a Zephyr application, not a 3rd-party library, and it is quite obvious that you cannot write a Zephyr applications without using Zephyr code and APIs. I will send a doc patch to fix this.

You mean MCUboot is an application?
https://docs.mcuboot.com/ clearly describes that is a library. But no matter how, modules should not be an application either. An there should not be any third party aplicaton dependencies in Zephyr tree. For sure a sample should not depend on third party application. And calling it application does not change that the porting layer is not in the Zephyr tree.

nordicjm · 2025-01-24T11:49:14Z

Nope, that are dependencies (although DFU subsystem can be used with different bootloader library). This does not make it better, but it does make it more obvious that the MCUboot module violates Zephyr modules should not contain code that is written exclusively for Zephyr. Instead, such code should be contributed to the main zephyr tree..

Obviously that only applies to libraries, not applications. MCUboot is a special case because it is a Zephyr application, not a 3rd-party library, and it is quite obvious that you cannot write a Zephyr applications without using Zephyr code and APIs. I will send a doc patch to fix this.

You mean MCUboot is an application? https://docs.mcuboot.com/ clearly describes that is a library. But no matter how, modules should not be an application either. An there should not be any third party aplicaton dependencies in Zephyr tree. For sure a sample should not depend on third party application. And calling it application does not change that the porting layer is not in the Zephyr tree.

You must have glossed over first half of this post: #79794 (comment)

carlescufi · 2025-01-24T12:03:55Z

You mean MCUboot is an application?
https://docs.mcuboot.com/ clearly describes that is a library.

It is both, of course.

But no matter how, modules should not be an application either. An there should not be any third party aplicaton dependencies in Zephyr tree. For sure a sample should not depend on third party application. And calling it application does not change that the porting layer is not in the Zephyr tree.

That is your opinion, I disagree with that. In the particular case of a bootloader, and MCUboot in particular, it makes all the sense in the world for Zephyr to pull it as a module, but for the app to be maintained in the MCUboot project. We could of course ask them to move the app to the main Zephyr tree, but then they would lose control over it, which may not be what they (or us) want.

nashif · 2025-01-24T12:26:52Z

This is not a sample really, it's useless. In the default configuration you an upload a new firmware image that you can never use, you can also upload to a ramdisk image which, again, you can never use, so if there is to be such a project it should go in tests. This is on par with having a low power sample where when you open the main.c file it's just a comment "write the code yourself"

whether it is a sample, a test or something else is beside the point, we have this issue in majority of what we call samples. USB DFU class deserves its own sample without pulling the mcuboot into the discussion, how useful it is and if it needs to be changed to become useful is up for discussion.
One can take Zephyr, write some basic code, add usb-dfu support and use that to load applications for example (using llext). This by the way was one of the earilest things in Zephyr and AFAIK arduino works this way and that was before mcuboot was a thing.

jfischer-no · 2025-01-24T12:29:25Z

You mean MCUboot is an application?
https://docs.mcuboot.com/ clearly describes that is a library.

It is both, of course.

But no matter how, modules should not be an application either. An there should not be any third party aplicaton dependencies in Zephyr tree. For sure a sample should not depend on third party application. And calling it application does not change that the porting layer is not in the Zephyr tree.

That is your opinion, I disagree with that. In the particular case of a bootloader, and MCUboot in particular, it makes all the sense in the world for Zephyr to pull it as a module, but for the app to be maintained in the MCUboot project. We could of course ask them to move the app to the main Zephyr tree, but then they would lose control over it, which may not be what they (or us) want.

@carlescufi How is Zephyr modules should not contain code that is written exclusively for Zephyr. Instead, such code should be contributed to the main zephyr tree. my opinion? The rule was also made to avoid broken circular dependencies, which is the problem with MCUboot's "application" library, where we do not know what happens in the porting layer, because the integration is not in the zephyr tree, but someone claims "this is the default bootloader".

But that is tangential, there is clearly no rule to use only the MCUboot "application" library, or to provide firmware update examples using the MCUboot "application" library. You can just ignore what I write about how badly MCUboot is coupled with the Zephyr project. I made my point as maintainer for this PR here, there was literally nothing contributed to the review from @nordicjm.

carlescufi · 2025-01-24T12:32:12Z

@carlescufi How is Zephyr modules should not contain code that is written exclusively for Zephyr. Instead, such code should be contributed to the main zephyr tree. my opinion? The rule was also made to avoid broken circular dependencies, which is the problem with MCUboot's "application" library, where we do not know what happens in the porting layer, because the integration is not in the zephyr tree, but someone claims "this is the default bootloader".

I did not say that this particular item was your opinion. I said that there should be an exception in that text for bootloaders. Then I said that it was your opinion that modules should not be an application. Those are two different things.

jfischer-no · 2025-01-24T12:38:54Z

@carlescufi How is Zephyr modules should not contain code that is written exclusively for Zephyr. Instead, such code should be contributed to the main zephyr tree. my opinion? The rule was also made to avoid broken circular dependencies, which is the problem with MCUboot's "application" library, where we do not know what happens in the porting layer, because the integration is not in the zephyr tree, but someone claims "this is the default bootloader".

I did not say that this particular item was your opinion. I said that there should be an exception in that text for bootloaders. Then I said that it was your opinion that modules should not be an application. Those are two different things.

"Zephyr relies on the source code of several externally maintained projects in order to avoid reinventing the wheel and to reuse as much well-established, mature code as possible when it makes sense."

That sound to me like a library description, but yeah, maybe that is just my interpretation, I would never think that including an application in the default manifest of the Zephyr project would be a good idea.

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

carlescufi · 2025-01-24T14:16:27Z

"Zephyr relies on the source code of several externally maintained projects in order to avoid reinventing the wheel and to reuse as much well-established, mature code as possible when it makes sense."

That sound to me like a library description, but yeah, maybe that is just my interpretation, I would never think that including an application in the default manifest of the Zephyr project would be a good idea.

Not really? MCUboot is:

An externally managed project
Established and mature
Implements a very common use case (bootloader)
Zephyr would have to write an application similar to MCUboot's if it were to have its own bootloader app, which would be "reinventing the wheel".

nordicjm · 2025-01-24T14:48:11Z

This is not a sample really, it's useless. In the default configuration you an upload a new firmware image that you can never use, you can also upload to a ramdisk image which, again, you can never use, so if there is to be such a project it should go in tests. This is on par with having a low power sample where when you open the main.c file it's just a comment "write the code yourself"

whether it is a sample, a test or something else is beside the point, we have this issue in majority of what we call samples. USB DFU class deserves its own sample without pulling the mcuboot into the discussion, how useful it is and if it needs to be changed to become useful is up for discussion. One can take Zephyr, write some basic code, add usb-dfu support and use that to load applications for example (using llext). This by the way was one of the earilest things in Zephyr and AFAIK arduino works this way and that was before mcuboot was a thing.

I don't really follow this, MCUboot has been in zephyr longer than I've used it, and I started using it with 1.13? in 2017-ish, llext wasn't added until zephyr... 3.5, and I can't see how this example is used with llext. Zephyr moves on, back in 1.x there was no sysbuild, if you wanted MCUboot in a project, you needed to go and configure and build MCUboot, then configure and build your application, flash them both and hope the configuration was correct. Now you just build an application with sysbuild and the Kconfig for it and you automatically get your application and MCUboot built, and it's a fully compatible configuration.

jfischer-no · 2025-01-24T15:28:07Z

"Zephyr relies on the source code of several externally maintained projects in order to avoid reinventing the wheel and to reuse as much well-established, mature code as possible when it makes sense."
That sound to me like a library description, but yeah, maybe that is just my interpretation, I would never think that including an application in the default manifest of the Zephyr project would be a good idea.

Not really? MCUboot is:
* An externally managed project

* Established and mature

* Implements a very common use case (bootloader)

* Zephyr would have to write an application similar to [MCUboot's](https://github.com/mcu-tools/mcuboot/tree/main/boot/zephyr) if it were to have its own bootloader app, which would be "reinventing the wheel".

@carlescufi I have trouble processing some statements. I need some help to clarify that.

Does the following text describe a library or not?
"Zephyr relies on the source code of several externally maintained projects in order to avoid reinventing the wheel and to reuse as much well-established, mature code as possible when it makes sense."

If Zephyr would have to write an bootloader code similar to MCUboot's it would be an library or would we call it application?

Is there a rule, that also applies to external "applications" if they are modules, that states that to be classified as a candidate for being included in the default list of modules "Zephyr modules should not contain code that is written exclusively for Zephyr. Instead, such code should be contributed to the main zephyr tree.", or not?

Two questions to Arch WG:

Does the Zephyr project code only have to use the MCUboot application and not provide any other interfaces to implement firmware updates?

Does "firmware update" only mean an update mechanism using Flash APIs, and does the firmware always have to reside in Flash?

And:

"Samples should be limited in scope and should focus only on demonstrating non-trivial or essential aspects of the subsystem(s) or module(s) being highlighted."

This sample is for USB device support, the scope is to show how to use USB DFU implementation. USB DFU device configuration is non-trivial. Essential aspect is the implementation of a backend or the use of flash backend.

Does this sample meet the sample criteria documented here? If not, why not?

jfischer-no · 2025-01-24T15:47:20Z

This is not a sample really, it's useless. In the default configuration you an upload a new firmware image that you can never use, you can also upload to a ramdisk image which, again, you can never use, so if there is to be such a project it should go in tests. This is on par with having a low power sample where when you open the main.c file it's just a comment "write the code yourself"

whether it is a sample, a test or something else is beside the point, we have this issue in majority of what we call samples. USB DFU class deserves its own sample without pulling the mcuboot into the discussion, how useful it is and if it needs to be changed to become useful is up for discussion.

Unfortunately, this sample also needs to cover the upload/download to flash use case (using DFU subsys) to provide similar functionality as the legacy sample. What it does and provide documentation on how to do it, and even how to use it with sysbuild and MCUboot. The user can even see how to use the built-in flash backend and how to implement a custom one. So am actually very confused as to how this can be useless. Finally, this does not force us to keep the long platform exclude list, because we cannot really filter platforms with flash problems or without partitions, because the flash maintainer does not care to provide testing features that would allow the twister to better filter.

One can take Zephyr, write some basic code, add usb-dfu support and use that to load applications for example (using llext). This by the way was one of the earilest things in Zephyr and AFAIK arduino works this way and that was before mcuboot was a thing.

I looked at LLEXT, but it would make it more complicated, especially for the new users. I may be wrong, but not all architectures work well with LLEXT. But this is a nice use case (and also applies to #62286) and something for the example applications.

nashif · 2025-01-24T19:08:57Z

I don't really follow this, MCUboot has been in zephyr longer than I've used it, and I started using it with 1.13? in 2017-ish, llext wasn't added until zephyr... 3.5, and I can't see how this example is used with llext. Zephyr moves on, back in 1.x there was no sysbuild, if you wanted MCUboot in a project, you needed to go and configure and build MCUboot, then configure and build your application, flash them both and hope the configuration was correct. Now you just build an application with sysbuild and the Kconfig for it and you automatically get your application and MCUboot built, and it's a fully compatible configuration.

fun. I did not say llext was used at any point, saying that this could be used. Zephyr was not launched with mcuboot support, mcuboot came at some point later. Zephyr was lauched primarily to support an arduino product among other things (arduino 101 based on intel curie) and the mode of updating the sketch and updates in general was usb dfu. Just saying, USB DFU is a thing, it can exist on its own and without sysbuild/mcuboot and all of that.

nashif · 2025-01-24T19:15:02Z

So am actually very confused as to how this can be useless.

hi, i was not saying it was useless, I honestly just skimmed through the code very fast, my point is, that is a different discussion.

nashif · 2025-01-24T19:27:34Z

I looked at LLEXT, but it would make it more complicated, especially for the new users. I may be wrong, but not all architectures work well with LLEXT. But this is a nice use case (and also applies to #62286) and something for the example applications.

yeah, it is evolving for sure (FYI @teburd @pillo79 )

teburd · 2025-01-25T17:21:18Z

@jfischer-no is right here, not sure what usb dfu has to do with mcuboot... Zephyr in the past had samples showing how to use usb dfu without any mcuboot involvement. DFU is a protocol, sample should show how to implement some hooks the protocol sets up I'd think? Loading an image directly to ram and jumping to it is actually pretty nice, sometimes boot rom's support this sort of boot from usb/serial type functionality without any bootloader to speak of. LLEXT would basically let you load an image into memory without explicitly linking it to specific memory locations which could be interesting but its not required to say... have a boot from usb type functionality here with dfu.

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

56 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 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

jfischer-no force-pushed the pr-device_next_dfu branch from 8d32019 to 0adc27b Compare January 23, 2025 15:35

kartben assigned jfischer-no and unassigned kartben Jan 23, 2025

jfischer-no added 4 commits January 24, 2025 14:50

usb: device_next: implement blocklist for classes

432e4de

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 <[email protected]>

jfischer-no force-pushed the pr-device_next_dfu branch from 0adc27b to c68a721 Compare January 24, 2025 14:05

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

Are you sure you want to change the base?

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

Conversation

jfischer-no commented Oct 14, 2024

carlescufi Oct 14, 2024

Choose a reason for hiding this comment

nordicjm left a comment

Choose a reason for hiding this comment

nordicjm commented Oct 15, 2024

tmon-nordic Oct 15, 2024

Choose a reason for hiding this comment

jfischer-no Oct 22, 2024

Choose a reason for hiding this comment

tmon-nordic Oct 15, 2024

Choose a reason for hiding this comment

tmon-nordic Nov 13, 2024

Choose a reason for hiding this comment

jfischer-no Jan 23, 2025

Choose a reason for hiding this comment

tmon-nordic Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

jfischer-no commented Oct 22, 2024

nordicjm commented Oct 22, 2024

nordicjm left a comment

Choose a reason for hiding this comment

jfischer-no commented Oct 23, 2024

nordicjm commented Oct 23, 2024

jfischer-no commented Oct 23, 2024 • edited Loading

carlescufi commented Nov 6, 2024

tejlmand commented Nov 6, 2024 • edited Loading

jfischer-no commented Jan 23, 2025

nordicjm commented Jan 23, 2025

jfischer-no commented Jan 23, 2025 • edited Loading

nashif commented Jan 23, 2025 • edited Loading

nordicjm commented Jan 24, 2025

jfischer-no commented Jan 24, 2025

carlescufi commented Jan 24, 2025

jfischer-no commented Jan 24, 2025

nordicjm commented Jan 24, 2025

carlescufi commented Jan 24, 2025

nashif commented Jan 24, 2025

jfischer-no commented Jan 24, 2025

carlescufi commented Jan 24, 2025

jfischer-no commented Jan 24, 2025

carlescufi commented Jan 24, 2025 • edited Loading

nordicjm commented Jan 24, 2025 • edited Loading

jfischer-no commented Jan 24, 2025

jfischer-no commented Jan 24, 2025

nashif commented Jan 24, 2025 • edited Loading

nashif commented Jan 24, 2025

nashif commented Jan 24, 2025

teburd commented Jan 25, 2025 • edited Loading

tmon-nordic Oct 18, 2024 •

edited

Loading

jfischer-no commented Oct 23, 2024 •

edited

Loading

tejlmand commented Nov 6, 2024 •

edited

Loading

jfischer-no commented Jan 23, 2025 •

edited

Loading

nashif commented Jan 23, 2025 •

edited

Loading

carlescufi commented Jan 24, 2025 •

edited

Loading

nordicjm commented Jan 24, 2025 •

edited

Loading

nashif commented Jan 24, 2025 •

edited

Loading

teburd commented Jan 25, 2025 •

edited

Loading