Move Generic thread driver Instance from the Platform code To avoid #39461
+288
−150
Conversation
This file contains hidden or 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
…ver from the platform GenericOpenThreadStackManager implementation itself. The driver must be instanciated in the app layer and is injected in the GenericThread Manager at init. This fix circular dependencies issues.
…m still leveraging the GenericThreadDriver implementation. Adapt Silabs NetWorCommissioningWifiDriver to leverage NetworkCommissioningDriver delegate in the same way
github-actions
bot
added
examples
platform
esp32
app
test driver
silabs
nxp
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the circular dependency issue by refactoring the instantiation of the network commissioning drivers. The introduction of NetworkDriverObj and the injection mechanism for the GenericThreadDriver are good steps towards better modularity.
The changes are quite extensive, touching platform-specific configurations and core OpenThread logic. While the overall direction is positive, there are a few critical and medium severity concerns, primarily related to the Silabs platform configuration and the new singleton access pattern for SlWiFiDriver.
Summary of Findings
- Critical: Potential variable redefinition and missing initialization in Silabs MatterConfig: In
examples/platform/silabs/MatterConfig.cpp,networkDriverobjects for WiFi and Thread might be redefined if bothSL_WIFIandCHIP_ENABLE_OPENTHREADare active. Furthermore, the singlenetworkDriver.Init()call is insufficient and ambiguous, likely leading to the WiFi driver not being initialized, which would cause runtime crashes due toSlWiFiDriver::GetInstance()returning a null pointer. - Medium: SlWiFiDriver GetInstance() pattern change: The
SlWiFiDriver::GetInstance()method has been changed to return a raw pointer (SlWiFiDriver*) which could benullptrif called before initialization. Call sites currently dereference this pointer without checking. While likely safe with correct initialization, this is less robust than the previous reference-returning version or a singleton that manages its own instance. - Low: Copyright Year (No comment added due to review settings): The new file
src/app/clusters/network-commissioning/NetworkCommissioningDriverDelegate.hhas a copyright year of 2025. This should typically be the current year or the year of creation.
Merge Readiness
This pull request makes significant strides in resolving a circular dependency, which is a valuable improvement. However, there are critical issues in examples/platform/silabs/MatterConfig.cpp concerning variable redefinition and improper initialization of network drivers that will likely lead to compilation errors or runtime crashes on affected Silabs platforms (especially those with both WiFi and Thread). Additionally, the change in the SlWiFiDriver::GetInstance() pattern introduces a potential for null pointer dereferences that should be carefully considered.
I recommend that these critical and medium severity issues be addressed before this PR is merged. As an AI reviewer, I am not authorized to approve pull requests; please ensure further review and approval from other maintainers after the suggested changes are made.
|
PR #39461: Size comparison from d24f34e to 5fe3e28 Full report (84 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
jmartinez-silabs
force-pushed
the
move_GenericThreadDriverInstance
branch
from
a3069c5 to
9fd6dc2
Compare
|
PR #39461: Size comparison from 89b92ca to 9fd6dc2 Increases above 0.2%:
Full report (46 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, psoc6, qpg, stm32, telink, tizen)
|
…ricThreadDriver in ESP32AppServer thread init
|
PR #39461: Size comparison from 89b92ca to f62f2a2 Full report (84 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
… for the GenericThreadDriver
|
PR #39461: Size comparison from 89b92ca to 573692b Full report (84 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
…n for the GenericThreadDriver
… GenericThreadDriver
… GenericThreadDriver
… the GenericThreadDriver
…he GenericThreadDriver
…the GenericThreadDriver
|
PR #39461: Size comparison from 89b92ca to c05840f Increases above 0.2%:
Full report (81 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, psoc6, qpg, telink, tizen)
|
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.
Summary
The instantiation of the GenericNetworkCommissioningThreadDriver, in the
GenericThreadStackManagerImpl_OpenThreadplatform code, causes a circular dependency between app code and platform code.This was initially done to reduce the duplication of the initiation across multiple platforms using this Generic implementation. However, splitting platform code and application layer brings more advantages and decoupling it has become preferable.
This PR brings the following:
NetworkCommissioningDriver Delegate: Introduces a delegate to manage the instantiation and initialization of both the NetworkCommissioning cluster and its corresponding driver. This structure supports Thread, Wi-Fi, and Ethernet drivers. While no changes are required for the GenericThread driver, minor adjustments are needed for some platform-specific drivers that use singletons—such as the Silabs Wi-Fi NetworkCommissioning driver.
Removal of NO_GENERIC_THREAD_NETWORK_COMMISSIONING_DRIVER: This macro is no longer necessary and has been removed.
Driver Instantiation Moved to App Layer: The GenericThreadNetworkCommissioningDriver is no longer instantiated within the GenericOpenThreadStackManager platform code. Instead, it is now created in the application layer and injected into the manager during initialization. This resolves the circular dependency issue.
Silabs Platform Update: The Silabs platform code has been updated to use the new NetworkCommissioningDriver delegate.
ESP32 Workaround Removed: The workaround for issue [openthread] Dependency inversion in platform & clusters #39441 has been removed from the ESP32 codebase.
TODO: Add initialization logic for NetworkCommissioningDriver on all platforms that previously relied on GenericThreadDriver within GenericThreadStackManagerImpl_OpenThread.
Related issues
fixes #39441
Testing
Build and flash a Silabs EFR32 thread board:
./scripts/build/build_examples.py --enable-flashbundle --target efr32-brd4187c-light buildCommission it :
./chip-tool pairing ble-thread <NODE_ID> hex:<THREAD_DATA_SET> <PINCODE> <DISCRIMINATOR>Test Scan network :
./chip-tool networkcommissioning scan-networks <NODE_ID> 0No change in behaviour
Existing Unit test added in #39289 completes without the
do_not_call_workaround_onlyReadability checklist
This PR will impact multiple Thread platforms that use both GenericThreadStackManagerImpl_OpenThread and GenericNetworkCommissioningThreadDriver. However, each commit is either a small, isolated change or specific to a single platform to simplify the review process.