-
Notifications
You must be signed in to change notification settings - Fork 2k
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
usbus: allow to forward EP0 transfer events to other handlers #19493
Open
dylad
wants to merge
5
commits into
RIOT-OS:master
Choose a base branch
from
dylad:pr/riotboot_dfu/drop_ztimer
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+24
−33
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Dylan Laduranty <[email protected]>
Signed-off-by: Dylan Laduranty <[email protected]>
Signed-off-by: Dylan Laduranty <[email protected]>
Signed-off-by: Dylan Laduranty <[email protected]>
Signed-off-by: Dylan Laduranty <[email protected]>
benpicco
reviewed
Apr 28, 2023
/* Forward to every handler, EP0 transfer event, except the first | ||
* handler as this is the current handler used for this event */ | ||
for (usbus_handler_t *hdl = handler->next; hdl; hdl = hdl->next) { | ||
if(usbus_handler_isset_flag(hdl, USBUS_HANDLER_FLAG_TR_EP0_FWD)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested change
if(usbus_handler_isset_flag(hdl, USBUS_HANDLER_FLAG_TR_EP0_FWD)) { | |
if (usbus_handler_isset_flag(hdl, USBUS_HANDLER_FLAG_TR_EP0_FWD)) { |
@@ -141,6 +141,7 @@ extern "C" { | |||
*/ | |||
#define USBUS_HANDLER_FLAG_TR_FAIL (0x0010) | |||
#define USBUS_HANDLER_FLAG_TR_STALL (0x0020) /**< Report transfer stall complete */ | |||
#define USBUS_HANDLER_FLAG_TR_EP0_FWD (0x0100) /**< Forward transfer complete */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So EP0 means transfer complete?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Area: sys
Area: System
Area: USB
Area: Universal Serial Bus
CI: ready for build
If set, CI server will compile all applications for all available boards for the labeled PR
Process: API change
Integration Process: PR contains or issue proposes an API change. Should be handled with care.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contribution description
This PR introduces a new
usbus
handler flag to allows to forward transfer complete event from EP0 only (control endpoints) to other handlers.The idea is to let other interfaces know the state of the transfer on these EPs. Each interface
shouldcould set this flag (at any moment) so it will receive these events.For a more real use case on DFU:
Currently, DFU uses
ztimer
for a unique purpose, schedule the reboot. At the end of the firmware transfer,dfu-util
will request the 'DFU state' of the device. Device must answer it, then reboot to start the newly flashed application. Thus, we cannot simply reboot as soon as we fill the buffer and ready the transfer withusbdev_ep_xmit()
. We need to wait the end of the transfer and that the host ACK it. This is whyztimer
was used. To wait a bit, to let the transfer completes properly, then reboot. Rebooting without this small delay, leads to an error indfu-util
message log (but the device is correctly flashed).dfu-util: unable to read DFU status after completion (LIBUSB_ERROR_PIPE)
Nevertheless, using
ztimer
for this purpose is more a workaround than a proper solution. That's why, a new flag is introduced by this PR. Each handler can set this bit at any point in time (we don't necessarily need it all the time). When the EP0 transfer event handling is done byusbus
, it will loop through all available handlers (except the first one which is already being call, to avoid a deathloop) and forward the event to interfaces that need it.ztimer
is now optional forriotboot_dfu
. It will only initztimer
if the dependency is pull by another module (e.gperiph_usbdev
on STM32).If
ztimer
dependency was only pull by DFU, this PR allows to reduce the bootloader size:saml21-xpro w/ PR same toolchain
Testing procedure
Flash
riotboot_dfu
on any board that supportperiph_usbdev
andriotboot
make -j8 BOARD=xxxx -C bootloaders/riotboot_dfu flash
Reboot into bootloader by resetting your board while pushing on BTN0.
Then flash any test app on your board through DFU:
PROGRAMMER=dfu-util USEMODULE=usbus_dfu make -j8 BOARD=xxxx -C tests/shell riotboot/flash-slot1
Board should reboot automatically and start the newly flashed test application.
No error message are reported by
dfu-util
Issues/PRs references
None.