Fully asynchrounous host code (v2)#3490
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| #if OSAL_MUTEX_REQUIRED | ||
| #if CFG_TUH_HUB | ||
| osal_queue_delete(_usbh_daq); |
Check warning
Code scanning / CodeQL
Expression has no effect Warning
There was a problem hiding this comment.
Hey Bot, this function will have side effects if CFG_TUSB_OS is different from OPT_OS_NONE
|
Hmm, any idea why hil-tinyusb would fail with this PR, but passed with #5484? |
Size Difference ReportBecause TinyUSB code size varies by port and configuration, the metrics below represent the averaged totals across all example builds. Note: If there is no change, only one value is shown. Changes >1% in size
Changes <1% in sizeNo entries. No changes
|
usb subsystem on my VM sometime got "stuck" and/or timeout due to lots of usb attach/activity. May need to re-run or reset the whole system. |
OK. Anyway, after I did the [Merge branch 'hathach:master' into non-blocking-host-v2] two days ago, all tests passed. |
…e with enumeration delay
| if (is_empty) { | ||
| return; // Exit if this is the only event in the queue, otherwise we loop forever | ||
| } | ||
| TU_ASSERT(osal_queue_send(_usbh_daq, &event, in_isr),); |
There was a problem hiding this comment.
multiple queue is not currently supported. E.g freeRTOS will put running task() to suspended mode and switch to other task when trying to get from the queue. I am making some refactor, and also rever this for now.
There was a problem hiding this comment.
You can just ignore the third patch for now then...
There was a problem hiding this comment.
(All three patches can be applied independently)
There was a problem hiding this comment.
Also: the second queue is only used with timeout=0, and only from the task() thread, so maybe the OS_NONE implementation could be used for that second queue?
There was a problem hiding this comment.
oh right, I overlook it, we can use queue receive with timeout=0, then. I did reverted, but will re-add that in the next push.
| #define CFG_TUH_TASK_USE_TIME_MILLIS_API 1 | ||
| #endif | ||
| #endif | ||
|
|
There was a problem hiding this comment.
My goal with that check was to enable the new asynchronous code with or without OS (because it is better in BOTH cases).
Unfortunately, somme platforms do not define tusb_time_millis_api() so I had to use this check to support them and get the CI green.
If tusb_time_millis_api() could be defined for all platforms, then I think this conditional code could just be removed. Unfortunately, adding the missing tusb_time_millis_api() is not something I can do.
There was a problem hiding this comment.
most rtos support time api that return time since boot. I will add osal_time_millis() and make it official. It is currently implement in user (bsp in the example).
…t change not set after 20ms
hathach
left a comment
hathach
left a comment
There was a problem hiding this comment.
perfect, thank you @ceedriic for the PR. I make some changes
- replace CFG_TUH_TASK_USE_TIME_MILLIS_API with CFG_TUSB_OS_HAS_SCHEDULER
- rename usbh_wait_delay to usbh_call_after_ms() and also take arg. This is useful helper, maybe used in other places as well in the future
- merge all delay/after callback to make it easier to maintain
I also make some other changes while doing local tests as well. Let me know what you think, or do we need to make any other improvement.
|
|
||
| #if CFG_TUH_HUB | ||
| // Deferred attachment queue, only needed when using hub | ||
| OSAL_QUEUE_DEF(usbh_int_set, _usbh_daqdef, CFG_TUH_HUB, hcd_event_t); |
There was a problem hiding this comment.
these event is only generated one per hub, since we process hub port status 1 by 1. The only time it got another attachment is from other hub, or from same hub (with multiple device) when an enumerating device is set_configured(). I short, max size is CFG_TUH_HUB
There was a problem hiding this comment.
Oh, great, this is why I couldn't test this code with a single hub :)
|
I'm reviewing the latest code, give me an hour or so... |
implement tusb_time_millis_api() with osal_time_millis() when OS is not NONE
hathach
force-pushed
the
non-blocking-host-v2
branch
from
0cbdf07 to
e492748
Compare
beware, there is a huge change list that replace board_millis() with tusb_time_millis_api() (which is required when OS=none). You can skip files in examples and hw/bsp |
hathach
force-pushed
the
non-blocking-host-v2
branch
from
e492748 to
8444c25
Compare
Thank you for reviewing/improving!
This is the part I do not understand: I agree my option name (CFG_TUH_TASK_USE_TIME_MILLIS_API) was bad,
Yes, both these changes make the code better.
I've basically two small things:
Into a simplified version: But I think it is wrong for two reasons:
Other than that, at least for a quick look, I like your changes. |
|
@ceedriic you are right, we shouldn't rely on rtos delay since it blocks main event queue. I apparently does not have enough sleep, let me try to revert those change. |
- call after (enum non-blocking delay) also support rtos now
hathach
left a comment
hathach
left a comment
There was a problem hiding this comment.
@ceedriic I revert the changes to also support non-blocking on RTOS, also rename to usbh_defer_func_ms_async() and make it a helper, though usage outside of enumeration need some kind of mutex. We will address that later if needed. Let me know if you think it looks good now ?
| }; | ||
|
|
||
| // process async delay in enumeration | ||
| static void enum_delay_async(uintptr_t state) { |
Check notice
Code scanning / CodeQL
Unused static function Note
I took a quick look at the usbh.c change, and I think is is OK, I've just 3 comments (the only important is the first one)
Thinking about it, I think the whole chunk here could also be simply replaced by a single line, without conditionals, to prevent sleeping a second time with an RTOS:
|
|
|
||
| // above after_cb() can re-schedule another function, we need to re-check and reduce timeout of | ||
| // the main event timeout to make sure we aren't blocking more than call_after timeout. | ||
| if (_usbh_data.call_after.func != NULL && |
There was a problem hiding this comment.
@ceedriic there is a gotcha, the callback can re-schedule another function. If we miss that main queue can sleep forever, and then there is no event (since we need that callback to submit new transfer for new event).
There was a problem hiding this comment.
Oops, Yep, good catch!
There was a problem hiding this comment.
I'm just worried about the unsigned comparaisons with your latest patch.
There was a problem hiding this comment.
I need to go for lunch but I will get back to you after
There was a problem hiding this comment.
@hathach
What do you think about the following alternative patch?
I think it's simpler and follow the rule to exit tuh_task() after we've processed some events.
diff --git a/src/host/usbh.c b/src/host/usbh.c
index a674b040e..80f8e5672 100644
--- a/src/host/usbh.c
+++ b/src/host/usbh.c
@@ -666,6 +666,7 @@ void tuh_task_ext(uint32_t timeout_ms, bool in_isr) {
TU_LOG_USBH("USBH invoke scheduled function\r\n");
_usbh_data.call_after.func = NULL;
after_cb(_usbh_data.call_after.arg);
+ timeout_ms = 0; // no sleeping after processing en event
} else if (timeout_ms > (uint32_t)ms) {
// reduce main event timeout to make sure we don't blocking more than call_after timeout
timeout_ms = (uint32_t)ms;
@@ -786,16 +787,7 @@ void tuh_task_ext(uint32_t timeout_ms, bool in_isr) {
break;
}
- #if CFG_TUSB_OS_HAS_SCHEDULER
- // return if there are no more events, to allow application to run other backgrounds
- if (osal_queue_empty(_usbh_q)
- #if CFG_TUH_HUB
- && osal_queue_empty(_usbh_daq)
- #endif
- ) {
- return;
- }
- #endif
+ timeout_ms = 0; // no sleeping after processing en event
}
}
There was a problem hiding this comment.
@ceedriic I have push a new update, that better address the signe comparison. Aso the timeout_ms = 0 is good one, just applied that.
There was a problem hiding this comment.
@hathach:
in your latest patch if remain_ms <= 0, you also need to set timeout_ms = 0
There was a problem hiding this comment.
yeah, that is kind of unexpected for a new calback expired right after scheduled but it is possible
|
Perfect!
…On 03.03.2026 05:01, Ha Thach wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In src/host/usbh.c
<#3490 (comment)>:
> } else {
// currently enumerating another device
TU_LOG_USBH("[%u:] USBH Defer Attach until current enumeration complete\r\n", event.rhport);
- const bool is_empty = osal_queue_empty(_usbh_q);
- queue_event(&event, in_isr);
- if (is_empty) {
- return; // Exit if this is the only event in the queue, otherwise we loop forever
- }
+ TU_ASSERT(osal_queue_send(_usbh_daq, &event, in_isr),);
fixed now with latest push.
—
Reply to this email directly, view it on GitHub
<#3490 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABXD4VUT6ASAZQIUTGIIBUD4OZKIPAVCNFSM6AAAAACUYFKWBOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTQOBQGA4DMNRXGA>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
3 participants
Describe the PR
This remove all calls to tusb_time_delay_ms_api() from inside tuh_task_ext().
Additional context
This is a cleaned-up version of #3482
Use of the host API was not really possible with OPT_OS_NONE due to blocking calls in tuh_task_ext().
With an OS, it will also improve the situation because:
I've split this in 3 main patches