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

Osal queue timeout #1440

Merged
merged 6 commits into from
Apr 20, 2022
Merged

Osal queue timeout #1440

merged 6 commits into from
Apr 20, 2022

Conversation

hathach
Copy link
Owner

@hathach hathach commented Apr 20, 2022

Describe the PR

  • add mili-second timeout to osal_queue_receive (mynewt does not support this).
  • tud_task() and tuh_task() timeout in 1 ms wait forever on event queue, but will exit once processed all events to allow RTOS thread to be shared with other background work e.g cdc flush().
  • tud_task_ext() and tuh_task_ext() are added to take timeout_ms and in_isr (not yet supported)
  • In case where 1 tick > 1ms (e.g 1 tick = 10ms), at least 1 tick will be used.
  • add TU_ATTR_ALWAYS_INLINE to most of osal function.
  • add tinyusb docs and disable blank issue

@dhalbert

@Nikitarc
Copy link

Please set a configurable timeout for each queue (something like CFG_TUSB_TMO_xxx). Not a hard value. Currently there is no timeout so I managed it in the OSAL layer: sometimes 0 and other times infinite. The value 1 is not practical because it leads to useless context switches when there is nothing to do, and a waste of time if we wanted 0. Moreover, if tud_task() is called by an interrupt, it is forbidden to have a timeout!
You say that a 1KHz SOF interrupt is expensive, but you are going to introduce a 1KHz context switch. It's the same order of magnitude...

@hathach
Copy link
Owner Author

hathach commented Apr 20, 2022

yeah, that makes sense on the context switch overhead, the tud_task() should take an timeout as argument instead. Though regarding ISR, tud_task() must not be called within ISR when using within an RTOS, current mutex, queue API does not take this into account (yet)

…eout and in_isr

also allow exit tud_task,tuh_task after processing all events for
running other background task for user
@hathach
Copy link
Owner Author

hathach commented Apr 20, 2022

@Nikitarc thank you for your feedback, I have updated the pr to add new API tud_task_ext() and tuh_task_ext() that takes timeout_ms and in_isr as parameters. The task will exit once processed all events so that thread can still be shared with other background function if needed.

@hathach hathach merged commit 55a5fd5 into master Apr 20, 2022
@hathach hathach deleted the osal-queue-timeout branch April 20, 2022 16:32
@Nikitarc
Copy link

I see you add timeout_ms, and specify 0xFFFFFFFF = wait forever.
Please add a define for this value, e.g. in osal.h and use it in usbd.h and usbh.h.
If this value changes in the future, it will be easier to track.
Greetings

@hathach
Copy link
Owner Author

hathach commented Apr 21, 2022

there is one OSAL_TIMEOUT_WAIT_FOREVER, you could also use the UINT32_MAX as well.

#define OSAL_TIMEOUT_WAIT_FOREVER (UINT32_MAX)

@Nikitarc
Copy link

Thanks for the info on OSAL_TIMEOUT_WAIT_FOREVER.
Why doesn't TinyUSB's code use it?

@hathach
Copy link
Owner Author

hathach commented Apr 25, 2022

osal.h isn't considered as public header, I don't have time right now to change it.I intend to use -1 with int, but decide 0xFFFFFFFF isn't too bad. This may change in the future.

@Nikitarc
Copy link

_osal.h isn't considered as public header
This is strange.
An application should include tusb.h wich is public.
And tusb.h includes osal.h. So an application can use what is in osal.h.

Anyway, we are not talking about application code, but about USB stack code. In my opinion the USB stack code should use its own definitions.

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