Skip to content

Enable specifying the alternate setting for the DFU interface per-target#43

Merged
devanlai merged 2 commits intodevanlai:masterfrom
twelho:dfu-custom-alt-setting
Sep 11, 2021
Merged

Enable specifying the alternate setting for the DFU interface per-target#43
devanlai merged 2 commits intodevanlai:masterfrom
twelho:dfu-custom-alt-setting

Conversation

@twelho
Copy link
Copy Markdown
Contributor

@twelho twelho commented Sep 6, 2021

Continuation of my bug hunting and feature implementation round started in #41.

This works by "padding" the DFU alternate setting with dummy no-op descriptors that occupy all previous alternate setting values. AFAICT while these no-op interfaces are not documented in the USB defined class codes, they don't identify as anything in Linux and cause no driver to be loaded nor any warnings in the system logs, and they should be fine for other operating systems as well. When USB_ALT > 0, the DFU interface appears in the specific alternate setting, which dfu-util does automatically identify and use, alternatively it can be manually targeted with the -a flag. To have USB_ALT be a per-target (optional) property the target.h implementor must load it (since it has the macro value to keep everything compile-time), which is why I've had to split out the descriptor structs into their own file to be preprocessed as part of target_stm32f103.c or target_stm32l1.c. The USB initialization code can then call target_usb_descriptor() to get the pointer to the configuration descriptor defined by the target.

This is the smallest change I could come up with, and I'm keen to hear if this compile-time approach is acceptable. I'm aware that the macros used to repeat structs when generating the dummy descriptors aren't very aesthetically pleasing, but I'm not aware of any better way to this without causing runtime overhead and too much complexity.

This works by "padding" the DFU alternate setting with dummy no-op descriptors
that occupy all previous alternate setting values. AFAICT while these no-op
interfaces are not documented in the USB defined class codes, they don't
identify as anything in Linux and cause no driver to be loaded nor any warnings
in the system logs, and they should be fine for other operating systems as
well. When USB_ALT > 0, the DFU interface appears in the specific alternate
setting, which dfu-util does automatically identify and use, alternatively it
can be manually targeted with the -a flag. To have USB_ALT be a per-target
(optional) property the target.h implementor must load it (since it has the
macro value to keep everything compile-time), which is why I've had to split
out the descriptor structs into their own file to be preprocessed as part of
target_stm32f103.c or target_stm32l1.c. The USB initialization code can then
call target_usb_descriptor() to get the pointer to the configuration
descriptor defined by the target.
Copy link
Copy Markdown
Owner

@devanlai devanlai left a comment

Choose a reason for hiding this comment

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

This is an interesting use-case that I hadn't thought of. This implementation looks like it works fine on Windows and Linux.

I think that as of #38, it's no longer necessary to move the USB descriptors into the target implementations. Instead, usb_conf.c alone can handle inserting the dummy alternate settings based on the value of USB_ALT from config.h.

Some other minor stuff:

  1. I would prefer a slightly different name for the alternate setting macro, maybe something like USB_DFU_ALTN.
  2. The non-dummy DFU alternate setting should have iInterface set to 4, not 0, so that it has the interface string associated with it.

…o to USB_DFU_ALTN

Assesses review feedback. I could initially not get the macros to work in
usb_conf.c, but they seem to work now even though the changes from pull devanlai#38
have been present in the development tree from the start.
@twelho
Copy link
Copy Markdown
Contributor Author

twelho commented Sep 10, 2021

You're right, usb_conf.c can read USB_ALT (now called USB_DFU_ALTN as per your suggestion). For some reason I could not get this to work when doing the initial implementation even though #38 has been in my development tree from the start. Oh well, it works now and is much cleaner like this. Good catch wrt. iInterface, that was left over from development back when I tried to isolate some warnings Linux gave about the alternate setting configuration. The warnings eventually turned out to be caused by cur_altsetting being a null pointer, and since there's no example on how to do multiple alternate settings with libopencm3 it took me a while to figure that out.

@twelho twelho requested a review from devanlai September 10, 2021 20:46
Copy link
Copy Markdown
Owner

@devanlai devanlai left a comment

Choose a reason for hiding this comment

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

This looks good to me now. I'll need to do some sanity testing tomorrow, but assuming it works, I expect to merge this as-is.

@devanlai devanlai merged commit 29fdafe into devanlai:master Sep 11, 2021
@twelho twelho deleted the dfu-custom-alt-setting branch September 11, 2021 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants