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.

Sign up for GitHub

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

Jump to bottom

Re-factor of how threading and event dispatch is handled in the SDK #7725

Closed
mrjerryjohns opened this issue Jun 17, 2021 · 6 comments
Closed

Re-factor of how threading and event dispatch is handled in the SDK #7725

mrjerryjohns opened this issue Jun 17, 2021 · 6 comments
Assignees
@kpschoedel
Labels
stale Stale issue or PR V1.X

Comments

@mrjerryjohns
Copy link
Contributor

mrjerryjohns commented Jun 17, 2021 •
edited
Loading

Problem

  • Approaches to threading and event dispatch vary quite widely between different platforms and approaches (POSIX vs GCD on Darwin vs FreeRTOS)
  • PlatformManager attempted to provide a singular API to unify these differences, but it ended up being too opinionated about policy, and makes it restrictive on certain platforms.
    • E.g. DeviceController::Init calls StartEventLoopTask, which always spins up a thread on POSIX platforms.
  • Results in an illusion of API conformity, when in reality, different platforms are implementing the different PlatformManager APIs quite differently, resulting in race conditions.

Solution

  • Make threading and event dispatch 'external' to the SDK core, with that being consciously decided by the application that is embedding the SDK within a larger whole.
  • SDK itself should rely on a fairly narrow 'RuntimeDispatch' interface that is responsible for managing posting and addition of event handlers (this still needs analysis), as well as for scheduling timers.
  • Still have the various PlatformManager implementations as 'cookie cutter' solutions that provide implementations of the above 'RuntimeDispatch' interface (or its equivalent).
  • Applications can choose to instantiate one of these impls if they choose to, or bring their own.
  • Use chip-tool as a way to showcase the different possible threading strategies an application can use, serving as a reference of sorts for developers to see how they can construct their own. The strategies include (but are not limited to):
    • Separable threads of execution for the application and the chip stack, with locks as the basis for achieving serialization
    • Singular thread of execution for both the application and the chip stack, all running in the same dispatch loop
    • Separable threads of execution for the application and the chip stack, with the application always making calls into the stack through the threading context of the stack, either asynchronously or synchronously.
  • For other example apps and tools maintained in the repository, a POSIX-based solution might achieve the maximum portability across platforms.

Tactical

  1. Design out the 'RuntimeDispatch' interface and put it up for review.
  2. Explore pivoting the existing PlatformManager and association implementations away from CRTP
  3. Figure out how to make it possible for the application to select a particular 'stock' PlatformManager impl, or bring their own.
  4. Modify chip-tool to showcase the above three strategies, perhaps with POSIX and separably, with Darwin GCD.
@gerickson
Copy link
Contributor

Haven't gotten far enough into it; however, as noted in the #6561 review, I am working towards porting these CoreFoundation / CFNetwork libraries and iOS apps to CHIP:

In theory, I should need only the singular main CFRunLoop with no further threading to make these/this work.

As noted in #6561, I should be able to import the InetLayer sockets into CFSocketStreams, schedule them onto the run loop, and go.

@mrjerryjohns
Copy link
Contributor Author

That should definitely be possible with the model being suggested here.

kpschoedel added a commit to kpschoedel/connectedhomeip that referenced this issue Jun 21, 2021
@kpschoedel
#### Problem
71beccb 
Followups from project-chip#6561 Generalize socket-based event loop

#### Change overview

* mDNS: Convert `platform/Linux/MdnsImpl`; remove Darwin stubs.
* Change `mCallbackData` type to `intptr_t` (review feedback).
* Comment for `System::WakeEvent` (this is minimal since it's
  likely to change soon for issue project-chip#7725).

Fixes project-chip#7758 _Convert MDNS to WatchableSocket._

#### Testing

Manual sanity check of mDNS using chip-device-ctrl on Linux.
Otherwise, no change to functionality is intended.
@kpschoedel kpschoedel mentioned this issue Jun 21, 2021
Merged
@kpschoedel
Copy link
Contributor

Tagging #7818 for inspection

@kpschoedel
Copy link
Contributor

Tagging #7803

kpschoedel added a commit to kpschoedel/connectedhomeip that referenced this issue Jun 22, 2021
@kpschoedel
#### Problem
bc0f8d2 
Followups from project-chip#6561 Generalize socket-based event loop

#### Change overview

* mDNS: Convert `platform/Linux/MdnsImpl`; remove Darwin stubs.
* Change `mCallbackData` type to `intptr_t` (review feedback).
* Comment for `System::WakeEvent` (this is minimal since it's
  likely to change soon for issue project-chip#7725).

Fixes project-chip#7758 _Convert MDNS to WatchableSocket._

#### Testing

Manual sanity check of mDNS using chip-device-ctrl on Linux.
Otherwise, no change to functionality is intended.
kpschoedel added a commit to kpschoedel/connectedhomeip that referenced this issue Jun 22, 2021
@kpschoedel
#### Problem
9d1fc1e 
Followups from project-chip#6561 Generalize socket-based event loop

#### Change overview

* mDNS: Convert `platform/Linux/MdnsImpl`; remove Darwin stubs.
* Change `mCallbackData` type to `intptr_t` (review feedback).
* Comment for `System::WakeEvent` (this is minimal since it's
  likely to change soon for issue project-chip#7725).

Fixes project-chip#7758 _Convert MDNS to WatchableSocket._

#### Testing

Manual sanity check of mDNS using chip-device-ctrl on Linux.
Otherwise, no change to functionality is intended.
andy31415 pushed a commit that referenced this issue Jun 23, 2021
@kpschoedel
Followups from #6561 Generalize socket-based event loop (#7799)
9692e9a 
* #### Problem

Followups from #6561 Generalize socket-based event loop

#### Change overview

* mDNS: Convert `platform/Linux/MdnsImpl`; remove Darwin stubs.
* Change `mCallbackData` type to `intptr_t` (review feedback).
* Comment for `System::WakeEvent` (this is minimal since it's
  likely to change soon for issue #7725).

Fixes #7758 _Convert MDNS to WatchableSocket._

#### Testing

Manual sanity check of mDNS using chip-device-ctrl on Linux.
Otherwise, no change to functionality is intended.

* review

* review
kpschoedel added a commit to kpschoedel/connectedhomeip that referenced this issue Jul 9, 2021
@kpschoedel
Revise System Layer clock code
5a458ad 
#### Problem

System Layer contains some unnecessary coupling, and a number of
ambiguous names, involving clock and time.

(This is a standalone fragment of work toward event/threading
refactoring, issue project-chip#7725)

#### Change overview

SystemLayer / SystemClock

- Add distinct type names MonotonicMilliseconds,
  MonotonicMicroseconds, UnixTimeMicroseconds.
- Move System::Layer clock functions to their own class.
- Rename them to make the time units explicit.
- Use them in preference to the System::Platform::Layer versions.
- Remove the HiRes version, which has no separate implementation.
- Move the platform implementations from System::Platform::Layer
  (not to be confused with System::Layer) to System::Platform::Clock.
- Removed the statement that SetUnixTimeMicroseconds() must be
  thread-safe. Since this function is not currently used, this
  imposes no burden on existing code.

TimeUtils

- Rename functions to make clean when ‘Epoch’ means the UNIX epoch.
- Use the scale conversion constants more often.

SystemTimer

- Remove the Timer::Epoch typedef, which wasn't an epoch, and
  rename SetTimerEpoch() to SetTimestamp().
- Remove the deprecated System::Timer::GetCurrentEpoch().

#### Testing

This change largely just renames existing code, so no functional changes
are expected. Existing cases in TestPlatformTime.cpp should catch any
mistakes in unit substititutions or conversions contstants.
@kpschoedel kpschoedel mentioned this issue Jul 9, 2021
Merged
kpschoedel added a commit to kpschoedel/connectedhomeip that referenced this issue Jul 9, 2021
@kpschoedel
Revisions to System::Layer
83548c4 
#### Problem

`System::Layer` has redundancy in event configuration,
and a confusingly-named grab-bag namespace `System::Platform::Layer`

This is a standalone fragment of work toward event/threading
refactoring, issue project-chip#7725.

#### Change overview

- Merge `CHIP_SYSTEM_CONFIG_LWIP_EVENT_OBJECT_TYPE` with
  `CHIP_SYSTEM_CONFIG_EVENT_OBJECT_TYPE`, and
  `CHIP_SYSTEM_CONFIG_LWIP_EVENT_TYPE` with
  `CHIP_SYSTEM_CONFIG_EVENT_TYPE`, since they serve the same purpose
  in LwIP and non-LwIP implementations respectively.
- Remove some dead configuration code.
- Move event functions from the `System::Platform::Layer` namespace
  (not the same kind of ‘layer’ as `System::Layer`) to a specific
  `System::Platform::EventSupport` namespace.
- Move (Will|Did)(Init|Shutdown) functions from the
  `System::Platform::Layer` namespace up to `System::Platform`.

#### Testing

No functional changes, only renaming and pruning.
@kpschoedel kpschoedel mentioned this issue Jul 9, 2021
Merged
kpschoedel added a commit to kpschoedel/connectedhomeip that referenced this issue Jul 10, 2021
@kpschoedel
Revise System Layer clock code
bb25f4b 
#### Problem

System Layer contains some unnecessary coupling, and a number of
ambiguous names, involving clock and time.

(This is a standalone fragment of work toward event/threading
refactoring, issue project-chip#7725)

#### Change overview

SystemLayer / SystemClock

- Add distinct type names MonotonicMilliseconds,
  MonotonicMicroseconds, UnixTimeMicroseconds.
- Move System::Layer clock functions to their own class.
- Rename them to make the time units explicit.
- Use them in preference to the System::Platform::Layer versions.
- Remove the HiRes version, which has no separate implementation.
- Move the platform implementations from System::Platform::Layer
  (not to be confused with System::Layer) to System::Platform::Clock.
- Removed the statement that SetUnixTimeMicroseconds() must be
  thread-safe. Since this function is not currently used, this
  imposes no burden on existing code.

TimeUtils

- Rename functions to make clean when ‘Epoch’ means the UNIX epoch.
- Use the scale conversion constants more often.

SystemTimer

- Remove the Timer::Epoch typedef, which wasn't an epoch, and
  rename SetTimerEpoch() to SetTimestamp().
- Remove the deprecated System::Timer::GetCurrentEpoch().

#### Testing

This change largely just renames existing code, so no functional changes
are expected. Existing cases in TestPlatformTime.cpp should catch any
mistakes in unit substititutions or conversions contstants.
andy31415 pushed a commit that referenced this issue Jul 12, 2021
@kpschoedel
Revise System Layer clock code (#8266)
a70c659 
* Revise System Layer clock code

#### Problem

System Layer contains some unnecessary coupling, and a number of
ambiguous names, involving clock and time.

(This is a standalone fragment of work toward event/threading
refactoring, issue #7725)

#### Change overview

SystemLayer / SystemClock

- Add distinct type names MonotonicMilliseconds,
  MonotonicMicroseconds, UnixTimeMicroseconds.
- Move System::Layer clock functions to their own class.
- Rename them to make the time units explicit.
- Use them in preference to the System::Platform::Layer versions.
- Remove the HiRes version, which has no separate implementation.
- Move the platform implementations from System::Platform::Layer
  (not to be confused with System::Layer) to System::Platform::Clock.
- Removed the statement that SetUnixTimeMicroseconds() must be
  thread-safe. Since this function is not currently used, this
  imposes no burden on existing code.

TimeUtils

- Rename functions to make clean when ‘Epoch’ means the UNIX epoch.
- Use the scale conversion constants more often.

SystemTimer

- Remove the Timer::Epoch typedef, which wasn't an epoch, and
  rename SetTimerEpoch() to SetTimestamp().
- Remove the deprecated System::Timer::GetCurrentEpoch().

#### Testing

This change largely just renames existing code, so no functional changes
are expected. Existing cases in TestPlatformTime.cpp should catch any
mistakes in unit substititutions or conversions contstants.

* fix esp32 lock-app build, and name kMillisecondsPerSecond
bzbarsky-apple pushed a commit that referenced this issue Jul 15, 2021
@kpschoedel
Revisions to System::Layer (#8267)
fd6e541 
* Revisions to System::Layer

#### Problem

`System::Layer` has redundancy in event configuration,
and a confusingly-named grab-bag namespace `System::Platform::Layer`

This is a standalone fragment of work toward event/threading
refactoring, issue #7725.

#### Change overview

- Merge `CHIP_SYSTEM_CONFIG_LWIP_EVENT_OBJECT_TYPE` with
  `CHIP_SYSTEM_CONFIG_EVENT_OBJECT_TYPE`, and
  `CHIP_SYSTEM_CONFIG_LWIP_EVENT_TYPE` with
  `CHIP_SYSTEM_CONFIG_EVENT_TYPE`, since they serve the same purpose
  in LwIP and non-LwIP implementations respectively.
- Remove some dead configuration code.
- Move event functions from the `System::Platform::Layer` namespace
  (not the same kind of ‘layer’ as `System::Layer`) to a specific
  `System::Platform::EventSupport` namespace.
- Move (Will|Did)(Init|Shutdown) functions from the
  `System::Platform::Layer` namespace up to `System::Platform`.

#### Testing

No functional changes, only renaming and pruning.

* restyle

* zap regen
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this issue Sep 23, 2021
@kpschoedel
Followups from project-chip#6561 Generalize socket-based event loop (p…
f96b622 
…roject-chip#7799)

* #### Problem

Followups from project-chip#6561 Generalize socket-based event loop

#### Change overview

* mDNS: Convert `platform/Linux/MdnsImpl`; remove Darwin stubs.
* Change `mCallbackData` type to `intptr_t` (review feedback).
* Comment for `System::WakeEvent` (this is minimal since it's
  likely to change soon for issue project-chip#7725).

Fixes project-chip#7758 _Convert MDNS to WatchableSocket._

#### Testing

Manual sanity check of mDNS using chip-device-ctrl on Linux.
Otherwise, no change to functionality is intended.

* review

* review
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this issue Sep 23, 2021
@kpschoedel
Revise System Layer clock code (project-chip#8266)
2dfba04 
* Revise System Layer clock code

#### Problem

System Layer contains some unnecessary coupling, and a number of
ambiguous names, involving clock and time.

(This is a standalone fragment of work toward event/threading
refactoring, issue project-chip#7725)

#### Change overview

SystemLayer / SystemClock

- Add distinct type names MonotonicMilliseconds,
  MonotonicMicroseconds, UnixTimeMicroseconds.
- Move System::Layer clock functions to their own class.
- Rename them to make the time units explicit.
- Use them in preference to the System::Platform::Layer versions.
- Remove the HiRes version, which has no separate implementation.
- Move the platform implementations from System::Platform::Layer
  (not to be confused with System::Layer) to System::Platform::Clock.
- Removed the statement that SetUnixTimeMicroseconds() must be
  thread-safe. Since this function is not currently used, this
  imposes no burden on existing code.

TimeUtils

- Rename functions to make clean when ‘Epoch’ means the UNIX epoch.
- Use the scale conversion constants more often.

SystemTimer

- Remove the Timer::Epoch typedef, which wasn't an epoch, and
  rename SetTimerEpoch() to SetTimestamp().
- Remove the deprecated System::Timer::GetCurrentEpoch().

#### Testing

This change largely just renames existing code, so no functional changes
are expected. Existing cases in TestPlatformTime.cpp should catch any
mistakes in unit substititutions or conversions contstants.

* fix esp32 lock-app build, and name kMillisecondsPerSecond
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this issue Sep 23, 2021
@kpschoedel
Revisions to System::Layer (project-chip#8267)
0fec8e4 
* Revisions to System::Layer

#### Problem

`System::Layer` has redundancy in event configuration,
and a confusingly-named grab-bag namespace `System::Platform::Layer`

This is a standalone fragment of work toward event/threading
refactoring, issue project-chip#7725.

#### Change overview

- Merge `CHIP_SYSTEM_CONFIG_LWIP_EVENT_OBJECT_TYPE` with
  `CHIP_SYSTEM_CONFIG_EVENT_OBJECT_TYPE`, and
  `CHIP_SYSTEM_CONFIG_LWIP_EVENT_TYPE` with
  `CHIP_SYSTEM_CONFIG_EVENT_TYPE`, since they serve the same purpose
  in LwIP and non-LwIP implementations respectively.
- Remove some dead configuration code.
- Move event functions from the `System::Platform::Layer` namespace
  (not the same kind of ‘layer’ as `System::Layer`) to a specific
  `System::Platform::EventSupport` namespace.
- Move (Will|Did)(Init|Shutdown) functions from the
  `System::Platform::Layer` namespace up to `System::Platform`.

#### Testing

No functional changes, only renaming and pruning.

* restyle

* zap regen
@kpschoedel kpschoedel mentioned this issue Oct 13, 2021
Closed
kpschoedel added a commit to kpschoedel/connectedhomeip that referenced this issue Oct 19, 2021
@kpschoedel
Remove AsyncDNSResolver
d1dad71 
#### Problem

`AsyncDNSResolver` is unused, and also uses threading internally,
which interferes with threading cleanup (issue project-chip#7725).

#### Change overview

Remove it.

#### Testing

CI. (This code had no unit tests.)
@kpschoedel kpschoedel mentioned this issue Oct 19, 2021
Merged
Damian-Nordic pushed a commit that referenced this issue Oct 20, 2021
@kpschoedel
Remove AsyncDNSResolver (#10660)
1c0ded5 
#### Problem

`AsyncDNSResolver` is unused, and also uses threading internally,
which interferes with threading cleanup (issue #7725).

#### Change overview

Remove it.

#### Testing

CI. (This code had no unit tests.)
@stale
Copy link

stale bot commented Sep 5, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale Stale issue or PR label Sep 5, 2022
@stale
Copy link

stale bot commented Sep 18, 2022

This stale issue has been automatically closed. Thank you for your contributions.

@stale stale bot closed this as completed Sep 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kpschoedel kpschoedel

Labels
stale Stale issue or PR V1.X
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@gerickson @mrjerryjohns @woody-apple @kpschoedel