[Python] Fix threading model #20234
Merged
[Python] Fix threading model #20234
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This fixes a number of issues with the Python REPL's threading model, namely: - Multiple, duplicated methods for stack initialization. - Stack initialization split across really odd places - Incorrect thread initialization - Incorrect order of initialization
pullapprove
bot
requested review from
andy31415,
anush-apple,
arkq,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
erjiaqing,
franck-apple,
gjc13,
harsha-rajendran,
hawk248,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jtung-apple,
kghost,
kpschoedel,
lazarkov,
LuDuda and
mlepage-google
andy31415
reviewed
Jul 1, 2022
bzbarsky-apple
approved these changes
Jul 1, 2022
andy31415
reviewed
Jul 2, 2022
agners
reviewed
Jul 4, 2022
|
Tagging SVE, given there are PRs that are dependent on this. |
andy31415
requested changes
Jul 7, 2022
There was a problem hiding this comment.
Please add back StackInit.cpp to contain at least Platform::MemoryInit.
We should not funnel everything through 'controller' (i.e. it should not go to controller/python) since for a full python package it makes sense to isolate things. There are things python could do without needing a controller. Things like 'parse qr codes' or (arguably hard due to very strong coupling today) dnssd operations.
Damian-Nordic
approved these changes
Jul 7, 2022
only. Updated ChipStack to now use native to implicitly achieve this auto init (which sets up MemoryInit) before continuining onwards with controller-style initialization.
mrjerryjohns
force-pushed
the
python/fix-threading-model-final
branch
from
63798be to
fa7ac2f
Compare
andy31415
approved these changes
Jul 7, 2022
|
PR #20234: Size comparison from 1807fb1 to fa7ac2f Increases (2 builds for cyw30739, telink)
Decreases (1 build for telink)
Full report (16 builds for cyw30739, k32w, linux, mbed, nrfconnect, p6, telink)
|
|
PR #20234: Size comparison from 7514699 to ab27923 Increases (4 builds for cc13x2_26x2, telink)
Decreases (3 builds for cc13x2_26x2, esp32)
Full report (30 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
mrjerryjohns
force-pushed
the
python/fix-threading-model-final
branch
from
9619bf2 to
e6a7b2b
Compare
|
PR #20234: Size comparison from 4f7d5cf to e6a7b2b Increases (6 builds for cc13x2_26x2, cyw30739, efr32, esp32, mbed, telink)
Decreases (2 builds for cc13x2_26x2, telink)
Full report (41 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
github-actions bot
pushed a commit
that referenced
this pull request
Jul 7, 2022
* Fix Python Threading Model This fixes a number of issues with the Python REPL's threading model, namely: - Multiple, duplicated methods for stack initialization. - Stack initialization split across really odd places - Incorrect thread initialization - Incorrect order of initialization * Review feedback * Brought back the auto-stack init in native, albeit, a minimal version only. Updated ChipStack to now use native to implicitly achieve this auto init (which sets up MemoryInit) before continuining onwards with controller-style initialization. * Fix build
woody-apple
added a commit
that referenced
this pull request
Jul 7, 2022
* Fix Python Threading Model This fixes a number of issues with the Python REPL's threading model, namely: - Multiple, duplicated methods for stack initialization. - Stack initialization split across really odd places - Incorrect thread initialization - Incorrect order of initialization * Review feedback * Brought back the auto-stack init in native, albeit, a minimal version only. Updated ChipStack to now use native to implicitly achieve this auto init (which sets up MemoryInit) before continuining onwards with controller-style initialization. * Fix build Co-authored-by: Jerry Johns <[email protected]>
agners
added a commit
to home-assistant-libs/python-matter-server
that referenced
this pull request
Mar 5, 2024
Make sure to initialize logging after the native code init. This makes sure that the C++ pychip_native_init is called after pychip_CommonStackInit. This is required to make sure that the platform specific initialization in src/platform/Linux/PlatformManagerImpl.cpp is being called before using the SDK. See also: project-chip/connectedhomeip#20234
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Fixes the threading issues identified in #20233.
Solution
GetLibraryHandle.StackInit.cpp, and consolidated all stack initialization logic in a single place:pychip_DeviceController_StackInit. This now correctly orchestrates memory initialization, platform init, controller factory initialization and finally, starting up the Matter thread.PlatformMgr().StartEventLoopTask()to start up the Matter thread.pychip_DeviceController_StackShutdownto now actually be the symmetric mirror of init, AND, co-locate that next to Init to make it easy to read.Tests
Ran the patch in #20178 and ensured it passed.