-
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
System timer API does not document what thread the callback is called on #7497
Comments
My assumption was that this was how it would work, simply because that's the most straight forward implementation. And in the absence of a truly platform-agnostic mechanism to dispatch work onto other threads (e.g. something that does what delegates do in .NET, what signals and slots do in Qt, etc., but with a generic and consistent interface for all of our platforms), that seems pretty tough to do. I certainly don't know how to do it in a platform-agnostic way. I think that's OK though. It just means you need to understand this and dispatch to your particular thread context from the callback. So two steps to get code executing in your desired thread context instead of one. One observation. This interface:
I have personally found it more natural to have a delegate class type defined and pass this instead of function and data pointers. It doesn't really matter. You can accomplish the same with both. But that's become a pattern elsewhere in the stack. It just feels easier (to me) to be able to reference |
Right, to be clear I am not asking to change what the API does, just to have clear documentation about the contract. |
From my coarse analysis, it looks like it runs on the CHIP thread. Tracing through Timer::Start in SystemTimer.cpp, I found: On POSIX:
On FreeRTOS:
On Zephr:
|
#### Problem Fixes project-chip#7497 System timer API does not document what thread the callback is called on #### Change overview Added comment: > The SDK is not generally thread safe. System::Layer methods should > only be called from a single context, or otherwise externally > synchronized. For platforms that use a CHIP event loop thread, timer > callbacks are invoked on that thread; for platforms that use a CHIP > lock, the lock is held. #### Testing Changes comment only.
#### Problem Fixes project-chip#7497 System timer API does not document what thread the callback is called on #### Change overview Added comment: > The SDK is not generally thread safe. System::Layer methods should > only be called from a single context, or otherwise externally > synchronized. For platforms that use a CHIP event loop thread, timer > callbacks are invoked on that thread; for platforms that use a CHIP > lock, the lock is held. #### Testing Changes comment only.
#### Problem Fixes #7497 System timer API does not document what thread the callback is called on #### Change overview Added comment: > The SDK is not generally thread safe. System::Layer methods should > only be called from a single context, or otherwise externally > synchronized. For platforms that use a CHIP event loop thread, timer > callbacks are invoked on that thread; for platforms that use a CHIP > lock, the lock is held. #### Testing Changes comment only.
#### Problem Fixes project-chip#7497 System timer API does not document what thread the callback is called on #### Change overview Added comment: > The SDK is not generally thread safe. System::Layer methods should > only be called from a single context, or otherwise externally > synchronized. For platforms that use a CHIP event loop thread, timer > callbacks are invoked on that thread; for platforms that use a CHIP > lock, the lock is held. #### Testing Changes comment only.
Problem
The system timer API does not define what thread the timer callback will happen on. I think it's always the "message thread", though I don't know whether we have that clearly defined as a concept. But, importantly, the callback is NOT necessarily going to happen on the thread that started the timer, as far as I can tell. This is pretty non-obvious.
Proposed Solution
Document exactly how this API works, so consumers don't get surprised.
@tcarmelveilleux @andy31415 @msandstedt
The text was updated successfully, but these errors were encountered: