-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DeviceLayer] Add SetupPinCode and SetupDiscriminator to configuration manager #1655
Conversation
@gjc13 PTAL |
63c343b
to
4924181
Compare
@erjiaqing FYI, builds fail |
default "CHIPUS" | ||
config USE_TEST_SETUP_PIN_CODE | ||
string "Use Test Setup Pin Code" | ||
range 0 99999999 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry If I'm confused here. By setup pin code you mean the manual pairing code ? If that's correct the renaming is a bit confusing since the manual pairing code contains a field named.. pin code, that is called in the codebase setUpPinCode (https://github.com/project-chip/connectedhomeip/blob/master/src/setup_payload/ManualSetupPayloadGenerator.cpp#L37)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setup Pin Code is for setUpPinCode
in codebase, the 27bit integer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good. I'm less confused then !
In this case, the valid range is between 0 and 134217727 (2^27-1) and 0x0 is normally used to specify that the device has already been paired (for device that supports dynamic generation of QRCode).
And the comments above says that this string is expected to end with a valid Verhoeff check character. But this part of the comment refers to the whole Pairing Code, not the setup pin code afaict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I forget to remove that, thanks.
I remembered that the spec said the pin code printed on the device is a 8 digit number, thus 99999999 requires 27 bits, although 27 bits can represent 1.3e9 in fact. I guess device with screen which can display a QR code should also follow this limit.
As for the 0x0, since 0x0 will never be a valid pin code (since it indicate the device is already paired in some case), I think it is also ok if 0x0 means not to use the default value.
However, I wonder if using 0x0 to disable default discriminator is a good choice since discriminator is just a 12bit random number.
4924181
to
afac815
Compare
f228707
to
9ff2de8
Compare
9ff2de8
to
39945b6
Compare
39945b6
to
46a0f10
Compare
Size increase report for "gn_nrf-example-build"
Full report output
|
Size increase report for "gn_linux-example-build"
Full report output
|
Size increase report for "nrf-example-build"
Full report output
|
Size increase report for "linux-example-build"
Full report output
|
Size increase report for "esp32-example-build"
Full report output
|
Merge in WMN_TOOLS/matter from cleanup/wifi_move to develop/operation_WALL-E Squashed commit of the following: commit 628cd88 Author: Ricardo Casallas <[email protected]> Date: Tue Mar 26 07:36:33 2024 -0400 Wi-fi cleanup: Code review. commit dc140d6 Author: Ricardo Casallas <[email protected]> Date: Thu Mar 21 13:42:59 2024 -0400 [Silabs] Wi-fi folders moved. commit 6f4d473 Author: Jean-Francois Penven <[email protected]> Date: Mon Mar 25 10:22:15 2024 -0400 Fix 917 lock app (project-chip#32671)
Problem
Current, configuration manager still uses PairingCode and missing SetupDiscriminator, which is required to generate QR code and BLE advertisement.
Solution
Modify configutation manager to include them.
fixes #1654