[E2E][JF] Added GetJointFabricMode() to DeviceInstanceInfoProvider class #39668
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
There was a problem hiding this comment.
Code Review
This PR introduces an API to check if Joint Fabric is enabled and uses it to advertise the corresponding DNS-SD service parameters. The changes look good overall, but I've found a potential issue related to an uninitialized variable, along with some suggestions to improve code clarity and maintainability.
|
PR #39668: Size comparison from 9fbf826 to 735a021 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
bzbarsky-apple
requested changes
Jun 24, 2025
|
PR #39668: Size comparison from 8311948 to 2e77a20 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #39668: Size comparison from 230883d to e673524 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #39668: Size comparison from 3e49728 to ec59978 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
/gemini review |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new API to retrieve the Joint Fabric mode, updating the provider interface and modifying DNSSD advertisement logic accordingly.
- Added the virtual method GetJointFabricMode in DeviceInstanceInfoProvider.h.
- Updated Dnssd.cpp to retrieve and include joint fabric mode in service advertisement, including logging adjustments.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/include/platform/DeviceInstanceInfoProvider.h | Added new virtual method for retrieving the joint fabric mode. |
| src/app/server/Dnssd.cpp | Conditionally retrieves and advertises the joint fabric mode via DNSSD. |
Comments suppressed due to low confidence (1)
src/include/platform/DeviceInstanceInfoProvider.h:224
- The method name 'GetJointFabricMode' does not match the PR title, which refers to 'IsJointFabricEnabled'. Consider aligning the method name and API documentation with the intended functionality.
virtual CHIP_ERROR GetJointFabricMode(uint8_t & jointFabricMode) { return CHIP_ERROR_NOT_IMPLEMENTED; }
There was a problem hiding this comment.
Code Review
The pull request adds the IsJointFabricEnabled() function to the CommissioningModeProvider API and updates app/server/Dnssd.cpp to include Joint Fabric mode in the advertisement parameters. The changes are behind a conditional compilation flag CHIP_DEVICE_CONFIG_ENABLE_JOINT_FABRIC.
andy31415
reviewed
Jun 26, 2025
andy31415
reviewed
Jun 26, 2025
andy31415
reviewed
Jun 26, 2025
|
PR #39668: Size comparison from 83f21ca to 6365e3b Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
bzbarsky-apple
approved these changes
Jul 1, 2025
|
PR #39668: Size comparison from 2f277fc to 5d3fd6b Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
doru91
approved these changes
Jul 3, 2025
|
PR #39668: Size comparison from b6f0333 to 98ce143 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #39668: Size comparison from c4c6bea to ff366e8 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
/geminireview |
|
Hello @vijs, Thank you for requesting a review. I've analyzed the changes in PR #39668 and here's my feedback: Summary of Changes:This pull request introduces a new virtual method In Review Feedback:
Overall, this is a well-executed and focused pull request. The changes are clear, correctly implemented, and follow established coding practices. Once the CI checks pass, this PR should be ready to merge. Great work! |
robertfarnum
approved these changes
Jul 7, 2025
|
PR #39668: Size comparison from ce90b61 to 608ef36 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
dsavitsky-dsr
pushed a commit
to dsavitsky-dsr/connectedhomeip
that referenced
this pull request
Jul 9, 2025
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
Added GetJointFabricMode() to the DeviceInstanceInfoProvider class (an example implementation will be provided in a separate PR), and updated app/server/Dnssd.cpp to advertise the JF TXT value accordingly.
Related issues
Addresses Issue: #39662.
Testing
Verified using the below instructions. OJCW functionality will be tested in a follow-up PR that implements it.