Skip to content
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

Add optional hooks for DCD and HCD events #2303

Merged
merged 3 commits into from
Nov 24, 2023
Merged

Add optional hooks for DCD and HCD events #2303

merged 3 commits into from
Nov 24, 2023

Conversation

projectgus
Copy link
Contributor

@projectgus projectgus commented Nov 1, 2023

These are intended to allow bare metal platforms with one-shot scheduling capabilities to schedule the TinyUSB task handlers whenever they know there is work for them to do.

I've been experimenting with this approach for MicroPython, where we don't usually have an RTOS but we do have scheduling capability. Selectively scheduling tud_task_ext() to run when we know there are events in the queue reduces latency for processing, while also avoiding the overhead of polling tud_task_ext() when there may not have been any USB activity.

(For context, see micropython/micropython#12846 - that PR uses a linker wrap around dcd_event_handler to hook it. If this PR is accepted then we can change to using the new macro for the hooks.)

Thanks for all your work on TinyUSB, it's a fantastic library!

These are intended to allow bare metal platforms with one-shot scheduling
capabilities to schedule the TinyUSB task handlers whenever they know there is
work for them to do.

Signed-off-by: Angus Gratton <angus@redyak.com.au>
Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

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

thank you very much, this is a great idea and I really like it. Though I would like to tweak it to use weak callback function to be consistent with the rest of API. e.g

void tud_event_hook_cb(const void* event, bool in_isr) _weak
void tuh_event_hook_cb(const void* event, bool in_isr) _weak

The reasson to change to void* is dcd_event_t/hcd_event_t type are not public (not included with tusb.h).

Let me know if this suggestion make sense to you.

@projectgus
Copy link
Contributor Author

projectgus commented Nov 24, 2023

That's great, thanks! Would you like me to update the PR to change the hooks?

Probably the "event" parameter can be removed from the hook. I added it on a whim as they're "free" with a macro, but I can't think of a use case so it can probably be removed.

I guess passing through "in_isr" is still potentially useful, though.

@hathach
Copy link
Owner

hathach commented Nov 24, 2023

That's great, thanks! Would you like me to update the PR to change the hooks?

Yeah, please do it when you have the time.

Probably the "event" parameter can be removed from the hook. I added it on a whim as they're "free" with a macro, but I can't think of a use case so it can probably be removed.

I guess passing through "in_isr" is still potentially useful, though.

Yeah, you are right, application does not have any usage with internal data. Maybe we can change it to have rhport, eventid (header of the event) is better. I don't know if applications ever need that but maybe useful some cases especially rhport when there could be multiple host controllers.

void tud_event_hook_cb(uint8_t rhport, uint32_t eventid, bool in_isr) _weak
void tuh_event_hook_cb(uint8_t rhport, uint32_t eventid, bool in_isr) _weak

@hathach
Copy link
Owner

hathach commented Nov 24, 2023

actually, since I am working on PRs, I could do it just now, if you haven't started thge update yet.

@projectgus
Copy link
Contributor Author

I haven't, so feel free if you have time. Thanks!

Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

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

all is good now, thank you very much for the PR

@hathach hathach merged commit 02017a8 into hathach:master Nov 24, 2023
43 checks passed
@projectgus
Copy link
Contributor Author

@hathach Sorry I didn't reply earlier, that looks great thank you!

@projectgus projectgus deleted the feature/isr_event_hook branch December 1, 2023 04:39
@hathach
Copy link
Owner

hathach commented Dec 1, 2023

All is good.

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