[E2E][JF] Implemented JCM mDNS advertisement and OJCW #38994
Conversation
There was a problem hiding this comment.
Please update the testing summary to contain commands tested and observed results. Verified using docs/guides/joint_fabric_guide.md instructions. seems to try to make it conventient to say manually tested.
However the whole intent of our policy is to make it inconvenient to just say "manually tested" to (strongly!) encourage writing automated tests. So in this case:
- write details on how you tested (generally what app you built, chip-tool or repl commands run and what were the observed results)
- explain why automated testing is impossible or what the schedule for adding automated tests is
src/lib/dnssd/ServiceNaming.cpp
Outdated
| { | ||
| return CHIP_ERROR_INVALID_ARGUMENT; | ||
| } | ||
| requiredSize = snprintf(buffer, bufferLen, "_J%u", static_cast<uint16_t>(subtype.code)); |
There was a problem hiding this comment.
I don't see this _J bit in the spec. Where is it?
There was a problem hiding this comment.
"J" stands for "Joint Fabric" and follows the same convention used in Commissioner Discovery (Section 4.3.3), where the "_" prefix is followed by the first letter of the acronym. Are you okay with this approach, or do you have any suggestions for improvement?
There was a problem hiding this comment.
The spec lists which subtypes are advertised. It does not list this subtype.
If you think this should be advertised, fix the spec to say so. There are no "conventions" here, and please don't make up things that are not in the spec. The subtypes that are defined as being advertised are in '4.3.1.3. Commissioning Subtypes" in the spec. And note that it's not always "first letter" for the subtype.
There was a problem hiding this comment.
Agreed, It is missing from the spec. I will review the need for a _J subtype for the Joint Fabric open commissioning window advertisement. The spec does define the JF key on the other hand.
There was a problem hiding this comment.
Yes, I am not saying the JF TXT bits are not in the spec, just that this _J part isn't.
There was a problem hiding this comment.
@bzbarsky-apple Agreed. @vijs I do not understand why we need _J? I believe the JF key is sufficient to meet the requirements, implementation and test concerns for JF commissioning. If there reason we need the _J subtype please explain.
There was a problem hiding this comment.
Without _J, it's possible that the controller could initiate the Joint Commissioning flow on a device that doesn't support Joint Commissioning, since only service subtypes are filtered according to the current CHIP tool implementation. This can be discussed separately.
To unblock this PR, _J has been removed in the latest commit to align with the latest specification.
There was a problem hiding this comment.
Upon further research by Doru and I, we believe that the following spec text should be amended in section 4.3.1.7 as follows:
5. The key/value pair CM=3 SHALL indicate that the publisher is currently in Commissioning Mode
and requires use of a dynamically generated Passcode for commissioning corresponding to the
verifier that was passed to the device using the OpenJointCommissioningWindow command.
@vijs Please confirm above, I will update the spec text accordingly.
There was a problem hiding this comment.
Moving this conversation to #39062 for further discussion.
src/lib/dnssd/TxtFields.h
Outdated
| @@ -47,6 +47,7 @@ static constexpr size_t kKeyDeviceNameMaxLength = 32; | |||
| static constexpr size_t kKeyRotatingDeviceIdMaxLength = 100; | |||
| static constexpr size_t kKeyPairingInstructionMaxLength = 128; | |||
| static constexpr size_t kKeyPairingHintMaxLength = 10; | |||
| static constexpr size_t kKeyJointFabricModeMaxLength = 10; | |||
There was a problem hiding this comment.
It mirrors Pairing Hint - they have the same types and text lengths.
There was a problem hiding this comment.
No, they do not. Pairing hint has 20 bits defined, which means it can go up to 524288. So technically it should be 6, not 10, but people add new pairing hints every so often...
JF has 4 bits defined and this PR assumes that it will never have more than 8 bits, so the most it can go to in the spec right now is 16, and the most it can go to in this code is 256. There's absolutely no reason for a max length > 3 here.
There was a problem hiding this comment.
Agreed, I changed it to 3 in the latest commit.
vijs
changed the title
|
PR #38994: Size comparison from 386263d to 7d65b8f Increases above 0.2%:
Full report (54 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
General comment: I would say that this PR implements a subset of #38203 - the OJCW command. #38203 text also requires handling of some errors paths, so I would advise creating a separate sub-issue (see the "Create sub-issue" button) if those are scheduled for a follow up PR. What I would like to see in the testing section or added in the docs/guides/joint_fabric.md would be to show that |
examples/chip-tool/commands/pairing/OpenCommissioningWindowCommand.h
Outdated
Show resolved
Hide resolved
examples/chip-tool/commands/pairing/OpenCommissioningWindowCommand.h
Outdated
Show resolved
Hide resolved
|
PR #38994: Size comparison from bb6d92f to 4378967 Increases above 0.2%:
Full report (54 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
vijs
force-pushed
the
feature/38544
branch
2 times, most recently
from
df9e098 to
1e18994
Compare
You asked me to review this PR saying that all the comments have been addressed. However, most of the comments haven't been addressed. |
I've updated the PR description to indicate that it addresses a subset of #38203. A followup PR will address the rest. |
vijs
force-pushed
the
feature/38544
branch
2 times, most recently
from
4b7e2bf to
0dae1fb
Compare
github-actions
bot
added
the
documentation
vijs
force-pushed
the
feature/38544
branch
2 times, most recently
from
e954be2 to
d071c07
Compare
|
PR #38994: Size comparison from 05e766b to d071c07 Full report (4 builds for cc32xx, linux)
|
@vijs I see you updated the testing section with "look at these lines". Please add details in the testing section and do not reference the MD file. Tell me what you ran and what results you have seen. To be clear: we are intentionally forcing you to write out how you test it and it is inconvenient because this is a "manual test" and we want to discourage that. And very important: explain why you are manually testing this. Why could you not automate some/all the tests? MDNS currently has tests ... why were not tests added/changed as part of your changes to dnssd code? where are the unit tests for all the other files? This PR needs unit tests since unit tests are possible at least in a subset of files. |
examples/chip-tool/BUILD.gn
Outdated
| @@ -91,6 +91,8 @@ static_library("chip-tool-utils") { | |||
| "commands/icd/ICDCommand.h", | |||
| "commands/pairing/OpenCommissioningWindowCommand.cpp", | |||
| "commands/pairing/OpenCommissioningWindowCommand.h", | |||
| "commands/pairing/OpenJointCommissioningWindowCommand.cpp", | |||
There was a problem hiding this comment.
Should this be excluded from the build when Joint Fabric is not wanted?
There was a problem hiding this comment.
This should be excluded from the build in all the cases: it's jf-control-app which is being used for JF and this allows us to keep chip-tool files untouched.
There was a problem hiding this comment.
This has been excluded from chip-tool in the latest commit.
There was a problem hiding this comment.
I confirm, @robertfarnum please resolve this conversation.
docs/guides/joint_fabric_guide.md
Outdated
| Check for the following logs on the jf-admin-app side: | ||
|
|
||
| ``` | ||
| >>> [DIS] Advertise commission parameter vendorID=65521 productID=32769 discriminator=1261/04 cm=2 cp=0 jf=14 |
There was a problem hiding this comment.
jf-admin-app from Ecosystem B has 0xFFF2 Vendor ID (see the instructions where it has been commissioned using --commissioner-vendor-id 0xFFF2). Why does it advertise a vendorID of 65521 (0XFFF1)?
There was a problem hiding this comment.
--commissioner-vendor-id only sets the vendor ID of the commissioner, not the device. To set the vendor ID on the device side, you need to add --vendor-id to the command that launches the jfb-admin-app.
There was a problem hiding this comment.
I understand, makes sense. In this case please use --vendor-id 0xFFF1 as parameter for jf-admin-app on Ecosystem 1 and --vendor-id 0xFFF2 as parameter for jf-admin-app on Ecosystem 2 - instructions have to be updated accordingly.
The --commissioner-vendor-id and --vendor-id must be identical for the jf-control-app and jf-admin-app of the same Ecosystem.
Thanks, please create a sub-issue for #38203 with what remains to be implemented (I guess it's the error path handling). |
examples/chip-tool/commands/pairing/OpenJointCommissioningWindowCommand.cpp
Outdated
Show resolved
Hide resolved
examples/chip-tool/commands/pairing/OpenJointCommissioningWindowCommand.h
Outdated
Show resolved
Hide resolved
| @@ -272,6 +273,7 @@ void registerCommandsPairing(Commands & commands, CredentialIssuerCommands * cre | |||
| // make_unique<CommissionedListCommand>(), | |||
| make_unique<StartUdcServerCommand>(credsIssuerConfig), | |||
| make_unique<OpenCommissioningWindowCommand>(credsIssuerConfig), | |||
| make_unique<OpenJointCommissioningWindowCommand>(credsIssuerConfig), | |||
There was a problem hiding this comment.
Main goal of jf-control-app is to avoid changing chip-tool files: what jf-control-app did was to copy inside its own folder the chip-tool files that required specific JF changes. However maybe we don't have to copy the entire Commands.h file and manage somehow to make this specific change directly in the main.cpp of the jf-control-app during command registration.
There was a problem hiding this comment.
That was the original implementation, but your earlier review requested this change. To avoid unnecessary back-and-forth, the original approach has been reinstated and is reflected in the latest commit. Let's avoid revisiting this unless there's a strong reason.
There was a problem hiding this comment.
Note that I am investing long hours in reviewing the work from the PR, so naming this a "back-and-forth" process sounds a little bit harsh:
- First iteration of this PR: jcm open-joint-commissioning-window. My review: it's more natural to have the open-joint-commissioning-window inside the pairing command (e.g.: pairing open-joint-commissioning-window) because the existing open-commissioning-window follows the same pattern (e.g.: pairing open-commissioning-window command)
- Second iteration of this PR: pairing open-joint-commissioning-window. My review: good invocation pattern but this required changes inside a chip-tool file (Commands.h) so I was wondering if there is a way to avoid changing Commands.h while keeping the same invocation pattern. Just to detail my comment above: one option for doing that would be to add an Update function in the Commands.cpp file that can add a new command to an existing cluster command. Then this Update command can be called in main.cpp@jf-control-app around registerCommandsPairing command.
- You reverted to the first iteration of this PR while suggesting that my requested changes are triggering unnecessary back-and-forth.
Added tests as suggested and updated the Testing subsection above with detailed instructions from joint_fabric_guide.md. |
| &mIteration, "Number of PBKDF iterations to use to derive the verifier. Ignored if 'option' is 0."); | ||
| AddArgument("discriminator", 0, 4095, &mDiscriminator, "Discriminator to use for advertising. Ignored if 'option' is 0."); | ||
| AddArgument("timeout", 0, UINT16_MAX, &mTimeout, "Time, in seconds, before this command is considered to have timed out."); | ||
| AddArgument("endpoint-id", 0, UINT16_MAX, &mEndpointId, "Endpoint the command is targeted at."); |
There was a problem hiding this comment.
Can we move the endpoint-id immediately after the node-id? I see this is the convention for other commands (node-id followed by endpoint-id).
docs/guides/joint_fabric_guide.md
Outdated
| - Open Joint Commissioning Window on JF Admin App of Ecosystem B | ||
|
|
||
| ``` | ||
| >>> jcm open-joint-commissioning-window 11 400 1000 1261 1 |
There was a problem hiding this comment.
node-id (11) must be followed by endpoint-id (1)
| @@ -390,6 +391,14 @@ CHIP_ERROR CommissioningWindowManager::OpenEnhancedCommissioningWindow(Seconds32 | |||
| return err; | |||
| } | |||
|
|
|||
| CHIP_ERROR CommissioningWindowManager::OpenJointCommissioningWindow(Seconds32 commissioningTimeout, uint16_t discriminator, | |||
There was a problem hiding this comment.
All the JF introduced code in the existing code-SDK files should be guarded by CHIP_DEVICE_CONFIG_ENABLE_JOINT_FABRIC introduced by https://github.com/project-chip/connectedhomeip/pull/38749/files (merged).
| auto & failSafeContext = Server::GetInstance().GetFailSafeContext(); | ||
| auto & commissionMgr = Server::GetInstance().GetCommissioningWindowManager(); | ||
|
|
||
| VerifyOrExit(fabricInfo != nullptr, status.Emplace(AdministratorCommissioning::StatusCode::kPAKEParameterError)); |
There was a problem hiding this comment.
AdministratorCommissioning::StatusCode::kPAKEParameterError belongs to the AdministratorCommissioning Cluster and it shouldn't be used by Joint Fabric Adminstrator Cluster. Instead, the XML for Joint Fabric Administrator Cluster must be updated to add this cluster-specific status codes. Same comment for the other AdministratorCommissioning Status codes refereced inside this file. Here is the spec-PR: https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/11699
|
PR #38994: Size comparison from 0157dc6 to f6cd980 Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, nrfconnect, psoc6, qpg, stm32)
|
Implemented JCM mDNS advertisement and Open Joint Commissioning Window
Addresses #38544, #39101 & a subset of #38203
Testing
Verified using the below instructions
Check for the following logs on the jf-admin-app side: