Skip to content
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

Advertise all of our available operational identities. #8923

Merged

Conversation

bzbarsky-apple
Copy link
Contributor

@bzbarsky-apple bzbarsky-apple commented Aug 12, 2021

Problem

Right now we only advertise one of our operational identities.

Change overview

Advertise all of them.

Testing

In one terminal:

scripts/examples/gn_build_example.sh examples/all-clusters-app/linux out/debug/standalone chip_config_network_layer_ble=false
scripts/examples/gn_build_example.sh examples/chip-tool out/debug/standalone/
./out/debug/standalone/chip-all-clusters-app 

In another terminal:

./out/debug/standalone/chip-tool pairing onnetwork 0 20202021 3840 ::1 5540
./out/debug/standalone/chip-tool administratorcommissioning open-basic-commissioning-window 5 0
[ edit examples/chip-tool/config/PersistentStorage.cpp to make PersistentStorage::GetLocalNodeId return a different default value ]
rm /tmp/chip_tool_config.ini 
./out/debug/standalone/chip-tool pairing onnetwork 17 20202021 3840 ::1 5540

And observe the logs on the chip-all-clusters-app side:

[1628733601714] [83487:34874796] CHIP: [DL] Mdns: OnRegister name: 0000000000000000-7361F916541A0357, type: _matter._tcp., domain: local., flags: 2
[1628733602318] [83487:34874796] CHIP: [DL] Mdns: OnRegister name: 0000000000000000-836809B91CFA5C0B, type: _matter._tcp., domain: local., flags: 2

The fabric ids are still wrong because the pairing commands in chip-tool ignore the passed-in fabric id and just always use 0 when generating all the opcerts.... (because the relevant controller apis don't have a way to pass in a fabric id).

@github-actions
Copy link

Size increase report for "nrfconnect-example-build" from 50d80b9

File Section File VM
chip-lock.elf text 52 52
chip-lock.elf rodata 16 16
chip-lock.elf device_handles -4 -4
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
.debug_info,0,847
.debug_loc,0,593
.debug_ranges,0,512
.debug_line,0,402
.debug_abbrev,0,99
text,52,52
.debug_frame,0,16
rodata,16,16
.shstrtab,0,-2
device_handles,-4,-4
.debug_str,0,-13
.symtab,0,-144
.strtab,0,-278


@bzbarsky-apple bzbarsky-apple force-pushed the advertise-all-operational branch from fc472e3 to de865d6 Compare August 12, 2021 14:54
@msandstedt
Copy link
Contributor

The fabric ids are still wrong because the pairing commands in chip-tool ignore the passed-in fabric id and just always use 0 when generating all the opcerts.... (because the relevant controller apis don't have a way to pass in a fabric id).

Not to re-litigate things, but I think we are saying it is the controller that can only deal with a 0-fabric. There still seems like there is an opportunity for accessories to advertise the fabric ID actually enclosed in their certs (which isn't spec compliant, but seems saner).

The logic that 'there are bugs on both the controller and accessory side so, therefore, this is OK' is a bit hard to accept. On the other hand, it's impossible to move to a fabricReference-nodeId representation until the controller limitations are actually addressed.

@msandstedt
Copy link
Contributor

The fabric ids are still wrong because the pairing commands in chip-tool ignore the passed-in fabric id and just always use 0 when generating all the opcerts.... (because the relevant controller apis don't have a way to pass in a fabric id).

Not to re-litigate things, but I think we are saying it is the controller that can only deal with a 0-fabric. There still seems like there is an opportunity for accessories to advertise the fabric ID actually enclosed in their certs (which isn't spec compliant, but seems saner).

The logic that 'there are bugs on both the controller and accessory side so, therefore, this is OK' is a bit hard to accept. On the other hand, it's impossible to move to a fabricReference-nodeId representation until the controller limitations are actually addressed.

Oh, I apologize! You are doing this. Thank you!

I was confused by your statement that 'fabric IDs are still wrong'. Well yes, they are zero because that's what the controller can currently support. But the accessory is reporting this from the cert, not from some random value in the configuration manager. Correct?

@github-actions
Copy link

Size increase report for "gn_qpg-example-build" from 0b406de

File Section File VM
chip-qpg6100-lighting-example.out .text 4 4
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-qpg6100-lighting-example.out.map and ./pull_artifact/chip-qpg6100-lighting-example.out.map:

BLOAT EXECUTION FAILED WITH CODE 1:
bloaty: unknown file type for file './pull_artifact/chip-qpg6100-lighting-example.out.map'

Comparing ./master_artifact/chip-qpg6100-lighting-example.out and ./pull_artifact/chip-qpg6100-lighting-example.out:

sections,vmsize,filesize
.debug_loc,0,583
.debug_ranges,0,440
.debug_frame,0,16
.text,4,4
.shstrtab,0,-1
[Unmapped],0,-4
.debug_aranges,0,-24
.debug_str,0,-101
.symtab,0,-160
.strtab,0,-243
.debug_abbrev,0,-1822
.debug_line,0,-2890
.debug_info,0,-14934


@github-actions
Copy link

Size increase report for "esp32-example-build" from 0b406de

File Section File VM
chip-all-clusters-app.elf .flash.text 120 120
chip-all-clusters-app.elf .flash.rodata -8 -8
chip-lock-app.elf .flash.text 52 52
chip-lock-app.elf [1 Others] -8 -8
chip-shell.elf .flash.text 8 8
chip-bridge-app.elf .flash.text -52 -52
chip-temperature-measurement-app.elf [1 Others] -8 -8
chip-temperature-measurement-app.elf .flash.rodata -16 -16
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_info,0,505
.debug_loc,0,332
.debug_ranges,0,296
.debug_line,0,278
.flash.text,120,120
.debug_abbrev,0,27
.debug_frame,0,12
.riscv.attributes,0,-1
.strtab,0,-4
.flash.rodata,-8,-8
.debug_str,0,-13
[Unmapped],0,-112

Comparing ./master_artifact/chip-persistent-storage.elf and ./pull_artifact/chip-persistent-storage.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-lock-app.elf and ./pull_artifact/chip-lock-app.elf:

sections,vmsize,filesize
.debug_loc,0,461
.debug_ranges,0,280
.xt.prop._ZN4chip9Transport19ConstFabricIteratoreqERKS1_,0,112
.xt.lit._ZNK4chip8OptionalIPKcE5ValueEv,0,80
.xt.prop._ZNK4chip9Transport11FabricTable4cendEv,0,76
.xt.prop._ZNK4chip9Transport11FabricTable6cbeginEv,0,76
.flash.text,52,52
.debug_frame,0,48
.xt.lit._ZN4chip11DeviceLayer8Internal31GenericConfigurationManagerImplINS0_24ConfigurationManagerImplEE17_GetFailSafeArmedERb,0,40
.symtab,0,16
.shstrtab,0,12
.debug_aranges,0,-8
[1 Others],-8,-8
[Unmapped],0,-44
.xt.lit._ZN4chip11DeviceLayer8Internal31GenericConfigurationManagerImplINS0_24ConfigurationManagerImplEE12_GetFabricIdERy,0,-88
.debug_str,0,-99
.xt.prop._ZN4chip11DeviceLayer8Internal31GenericConfigurationManagerImplINS0_24ConfigurationManagerImplEE12_GetFabricIdERy,0,-112
.strtab,0,-172
.debug_abbrev,0,-3906
.debug_line,0,-4517
.debug_info,0,-68315

Comparing ./master_artifact/chip-ipv6only-app.elf and ./pull_artifact/chip-ipv6only-app.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize
.flash.text,8,8
[Unmapped],0,-8

Comparing ./master_artifact/chip-bridge-app.elf and ./pull_artifact/chip-bridge-app.elf:

sections,vmsize,filesize
.debug_loc,0,466
.debug_ranges,0,280
.xt.prop._ZN4chip9Transport19ConstFabricIteratoreqERKS1_,0,112
.xt.lit._ZNK4chip8OptionalIPKcE5ValueEv,0,80
.xt.prop._ZNK4chip9Transport11FabricTable4cendEv,0,76
.xt.prop._ZNK4chip9Transport11FabricTable6cbeginEv,0,76
[Unmapped],0,52
.debug_frame,0,48
.xt.lit._ZN4chip11DeviceLayer8Internal31GenericConfigurationManagerImplINS0_24ConfigurationManagerImplEE17_GetFailSafeArmedERb,0,40
.symtab,0,16
.shstrtab,0,12
.debug_aranges,0,-8
.flash.text,-52,-52
.xt.lit._ZN4chip11DeviceLayer8Internal31GenericConfigurationManagerImplINS0_24ConfigurationManagerImplEE12_GetFabricIdERy,0,-88
.debug_str,0,-101
.xt.prop._ZN4chip11DeviceLayer8Internal31GenericConfigurationManagerImplINS0_24ConfigurationManagerImplEE12_GetFabricIdERy,0,-112
.strtab,0,-172
.debug_abbrev,0,-3362
.debug_line,0,-4427
.debug_info,0,-38672

Comparing ./master_artifact/chip-pigweed-app.elf and ./pull_artifact/chip-pigweed-app.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-temperature-measurement-app.elf and ./pull_artifact/chip-temperature-measurement-app.elf:

sections,vmsize,filesize
.debug_loc,0,453
.debug_ranges,0,280
.xt.prop._ZN4chip9Transport19ConstFabricIteratoreqERKS1_,0,112
.xt.lit._ZNK4chip8OptionalIPKcE5ValueEv,0,80
.xt.prop._ZNK4chip9Transport11FabricTable4cendEv,0,76
.xt.prop._ZNK4chip9Transport11FabricTable6cbeginEv,0,76
.debug_frame,0,48
.xt.lit._ZN4chip11DeviceLayer8Internal31GenericConfigurationManagerImplINS0_24ConfigurationManagerImplEE17_GetFailSafeArmedERb,0,40
[Unmapped],0,24
.symtab,0,16
.shstrtab,0,12
.debug_aranges,0,-8
[1 Others],-8,-8
.flash.rodata,-16,-16
.xt.lit._ZN4chip11DeviceLayer8Internal31GenericConfigurationManagerImplINS0_24ConfigurationManagerImplEE12_GetFabricIdERy,0,-88
.debug_str,0,-100
.xt.prop._ZN4chip11DeviceLayer8Internal31GenericConfigurationManagerImplINS0_24ConfigurationManagerImplEE12_GetFabricIdERy,0,-112
.strtab,0,-172
.debug_abbrev,0,-3377
.debug_line,0,-4432
.debug_info,0,-38676


@bzbarsky-apple
Copy link
Contributor Author

I was confused by your statement that 'fabric IDs are still wrong'. Well yes, they are zero because that's what the controller can currently support. But the accessory is reporting this from the cert, not from some random value in the configuration manager. Correct?

Right. In my specific test steps the fabric id being advertised for the second "pairing" is 0, not 17, because the controller doesn't actually put 17 in the cert. But the accessory is advertising whatever is in the cert.

@bzbarsky-apple bzbarsky-apple merged commit 1e5fedf into project-chip:master Aug 12, 2021
@bzbarsky-apple bzbarsky-apple deleted the advertise-all-operational branch August 12, 2021 17:20
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Device identities not derived from Certificates Add ability to advertise multiple node IDs on mDNS
4 participants