Thread: commissioning over Thread MeshCoP#42992
Merged
Merged
Conversation
github-actions
Bot
added
tools
examples
scripts
setup payload
platform
linux
lib
inet
controller
app
github
workflows
support
labels
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces support for Matter commissioning over Thread MeshCoP, a significant feature enhancement. The changes span from the chip-tool commissioner logic to the device-side stack, including new testing infrastructure. The implementation of the CommissionProxy on the commissioner side is central to this feature, alongside new rendezvous logic in the Thread stack on the device side. My review has identified a critical issue related to thread lifetime management, along with a few medium-severity issues concerning dead code and incorrect log messages. Addressing these points will improve the robustness and maintainability of this new functionality.
bukepo
force-pushed
the
mcot-initial
branch
3 times, most recently
from
71dcc4f to
8b8971d
Compare
bukepo
added
the
changing-git-submodules-on-purpose
bukepo
force-pushed
the
mcot-initial
branch
3 times, most recently
from
0c2cd89 to
af1d5e1
Compare
|
PR #42992: Size comparison from 6d8d7f7 to af1d5e1 Full report (6 builds for cc32xx, psoc6)
|
bukepo
force-pushed
the
mcot-initial
branch
6 times, most recently
from
bb8bf65 to
c1ba80b
Compare
andy31415
reviewed
Feb 9, 2026
andy31415
reviewed
Feb 9, 2026
andy31415
reviewed
Feb 9, 2026
andy31415
reviewed
Feb 9, 2026
andy31415
reviewed
Feb 9, 2026
andy31415
reviewed
Feb 9, 2026
andy31415
reviewed
Feb 9, 2026
andy31415
reviewed
Feb 9, 2026
andy31415
reviewed
Feb 9, 2026
andy31415
reviewed
Feb 9, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 48 changed files in this pull request and generated 10 comments.
Comments suppressed due to low confidence (2)
src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.hpp:1
_RendezvousStart()takes the Thread stack lock, but_RendezvousStop()calls into OpenThread (otSeekerStop) without locking. This asymmetry can race with other Thread stack operations and violates the pattern used elsewhere in this class. Wrap the OpenThread call(s) in the sameLockThreadStack()/UnlockThreadStack()discipline as_RendezvousStart()(and ensure timer cancellation/state resets are safe relative to outstanding callbacks).
src/lib/support/ThreadDiscoveryCode.cpp:1- The constructor accepts a
uint16_tbut packs only 12 bits. Values > 0x0FFF will be truncated bydiscHigh4masking, which can create collisions and make debugging very hard. Either validate the input (return/ASSERT on invalid discriminator), or explicitly maskdiscriminator &= 0x0FFFand document that only 12-bit values are supported.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 48 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.hpp:1
- The “short discriminator” branch uses
AsUInt64Short()but still setsdiscerner.mLength = 64, which makes the match behave like a full 64-bit exact match (including the bytes you intentionally zeroed). This can incorrectly reject valid networks that match only the short discriminator. SetmLengthto the intended short-discriminator bit-length (per Thread MeshCoP joiner discerner semantics) and ensuremValuecorresponds to what OT expects for that length.
src/setup_payload/SetupPayload.h:1 kThreadis ambiguous: many devices “support Thread” without necessarily supporting “commissioning over Thread MeshCoP rendezvous.” Since this flag is being used specifically to indicate MeshCoP commissioning support, consider renaming it to something likekThreadMeshCoP(or at least update the comment to explicitly say “supports commissioning over Thread MeshCoP”) to avoid confusing QR payload semantics.
|
PR #42992: Size comparison from e675366 to 9e60e98 Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #42992: Size comparison from 2f0f1f5 to 7eb3451 Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 48 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (3)
src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.hpp:1
_RendezvousStart()explicitly locks/unlocks the Thread stack around OpenThread calls, but_RendezvousStop()callsotSeekerStop()without taking the same lock. This can race with other OpenThread operations (including the seeker callback path). Wrap OpenThread interactions here withImpl()->LockThreadStack()/Impl()->UnlockThreadStack()for consistency and thread safety.
src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.hpp:1- This function performs multiple OpenThread
otSeeker*operations without locking the Thread stack (unlike_RendezvousStart). Since it's invoked from both the seeker evaluator and scheduled SystemLayer work, it risks concurrent OpenThread access. Consider ensuring allotSeeker*calls here execute under the sameImpl()->LockThreadStack()/UnlockThreadStack()protection, or constrain execution to the OpenThread thread context.
src/setup_payload/SetupPayload.h:1 kThreadis ambiguous in the context of rendezvous flags (it could mean 'device is a Thread device' vs 'supports commissioning over Thread MeshCoP'). Consider renaming to something more specific (e.g.kThreadMeshCoP/kThreadRendezvous) to clarify semantics for QR code readers and commissioner logic.
|
PR #42992: Size comparison from 4eb9bda to f87c7ea Full report (12 builds for cc13x4_26x4, cc32xx, nrfconnect, qpg, realtek, stm32)
|
This commit adds support of Matter commissioning over Thread.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 48 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (4)
src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.hpp:1
_RendezvousStart()explicitly locks the OpenThread stack, but_RendezvousStop()callsotSeekerStop()withoutImpl()->LockThreadStack()/UnlockThreadStack(). This introduces a thread-safety mismatch and risks races with other OpenThread API usage. Wrap the OpenThread calls in_RendezvousStop()(and any other rendezvous helpers that call OpenThread APIs) with the same locking strategy used elsewhere in the class.
src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.hpp:1TryNextNetwork()calls multiple OpenThread Seeker APIs (otSeekerSetUpNextConnection,otSeekerIsRunning,otSeekerStop,otSeekerStart) without taking the Thread stack lock. Since this method is invoked from_HandleSeekerScanEvaluator()and also via timer/lambda paths, it should follow the same locking discipline as other OpenThread interactions to avoid races (especially if invoked from different execution contexts). Consider addingImpl()->LockThreadStack()/UnlockThreadStack()around the OpenThread calls in this method.
src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.h:1- These rendezvous-related members are not guarded by
#if CHIP_DEVICE_CONFIG_ENABLE_THREAD_MESHCOP, even though the feature is compiled conditionally elsewhere. This increases object size and forces extra type dependencies (e.g.PeerAddressand the callback type) even when MeshCoP is disabled. Consider wrapping the rendezvous-only state (andTryNextNetwork()declaration) inside the same#if CHIP_DEVICE_CONFIG_ENABLE_THREAD_MESHCOPblock as the rendezvous APIs.
src/lib/support/ThreadDiscoveryCode.cpp:1 - The constructor is documented as taking a 12-bit discriminator, but it silently accepts any
uint16_tand discards bits 12–15. This can cause unintended collisions (different inputs mapping to the same discovery code) and makes misuse hard to detect. Consider enforcingdiscriminator <= 0x0FFF(e.g., via an assertion,VerifyOrDie, or explicit masking plus a comment explaining the behavior).
|
PR #42992: Size comparison from 4eb9bda to ead53c8 Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
andy31415
approved these changes
Feb 10, 2026
andy31415
left a comment
andy31415
left a comment
Contributor
There was a problem hiding this comment.
Approving for now as this went some review passes.
Provided some feedback for future improvement separately.
Contributor
|
How does this relate/compare to #42841? It seems to be at-odds |
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
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
This commit adds Matter commissioning over Thread support.
On the device side, it performs Thread discovery to collect candidate networks and then try them one by one by sending unsolicited DNS response to notify the commissioner about its commissionable service. This message is composed based on the minimal MDNS library.
On the commissioner side, it creates CommissionProxy to configure the steering data and accept Joiner messages. Once a joiner comes in and the discriminator matches, it starts the commissioning just like the on-network commissioning.
A rendezvous flag is added for the commissioner to recognize that the device supports commissioning over Thread MeshCoP. However, the chip-tool will continue commissioning even if this flag is not present considering in-market devices may get the feature via OTA but without having the opportunity to update the QR Code.
Related issues
This is the initial PR for the Matter commissioning over Thread feature.
Testing
A new test "Run Thread-MeshCoP commissioning test" is added to verify this feature end-to-end.