Do not use an undefined node id for mdns advertising

[vivien-apple]

Problem

If an accessory already connected to the network and without any admin pairing is on the network, the mdns advertisement is 0000000000000000-0000000000000000 instead of beeing (for example): 0000000000000000-0000000000BC5C01 if the node id is 12344321.

Change overview

• Do not use an undefined node id from the admin pairing table

Testing

• It was tested on Mac with chip-all-clusters-app, with chip_config_network_layer_ble=false, advertising over mdns on the local network.
• Similarly to Darwin - Pass the port advertised by mdns in network bytes order #7487, I'm trying to get mdns advertisement working in order to have a working test.

[github-actions]

Size increase report for "gn_qpg6100-example-build" from 77e727c

File	Section	File	VM
chip-qpg6100-lighting-example.out	.text	16	16

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
.text,16,16
.debug_info,0,4
.debug_str,0,-1
.debug_line,0,-3
.debug_ranges,0,-16
[Unmapped],0,-16
.debug_loc,0,-32

[msandstedt]

Wrt the TODO above:

// TODO: once operational credentials are implemented, node ID should be read from them

I would think that once we are getting the node ID from operational credentials, we should remove this check.

[github-actions]

Size increase report for "nrfconnect-example-build" from 77e727c

File	Section	File	VM
chip-lock.elf	text	8	8
chip-lock.elf	device_handles	-8	-8

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
text,8,8
.debug_info,0,4
.debug_line,0,-4
device_handles,-8,-8
.debug_ranges,0,-16
.debug_loc,0,-32

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

sections,vmsize,filesize

[andy31415]

Could you explain the situation a bit?

Why do we have an admin pairing info without a well defined node id? Should the admin pairing info be missing in that case?
I would have expected the fix to be "do not have an invalid admin pairing" rather than "if admin pairing is invalid, ignore it".

[bzbarsky-apple]

I think this could definitely use a comment answering @andy31415's question. Is this about the time period between an admin being added and a node id being provisioned? Or nodes that were partially provisioned? Or something else?

[woody-apple]

@saurabhst @jelderton @Damian-Nordic @LuDuda ?

[Damian-Nordic]

Not sure what's @vivien-apple's case, but it once happened to me that the pairing window has expired and when I started BLE advertising again, a new admin has been allocated and after the commissioning the previously allocated admin had the node ID uninitialized. I guess this change will fix such issues although it might be better to remove incomplete admins when commissioning fails.

[andy31415]

Marking as changes requested to not get merged until we understand the case.

not having an invalid state at all seems better than ignoring invalid state when we see it.

[vivien-apple]

Could you explain the situation a bit?

Why do we have an admin pairing info without a well defined node id? Should the admin pairing info be missing in that case?
I would have expected the fix to be "do not have an invalid admin pairing" rather than "if admin pairing is invalid, ignore it".

I'm... not sure. Asking @shana-apple since she may have an answer.

But actually this is interesting. I dig more into the code and I'm curious of what is the right fix here.

The current code flow is, when the pairing window is opened, the next available admin id is populated to the AdminPairingTable (in this case 0) without a node id. The reason for not assigning any node id is because it is currently assigned at

connectedhomeip/src/transport/SecureSessionMgr.cpp

Line 438 in 0bca292

// TODO: Remove temporary code once AddOptCert is implemented

The comment "AddOpCert is implemented" suggests that NodeId should be populated when an operational certificate is received ?

[shana-apple]

Could you explain the situation a bit?
Why do we have an admin pairing info without a well defined node id? Should the admin pairing info be missing in that case?
I would have expected the fix to be "do not have an invalid admin pairing" rather than "if admin pairing is invalid, ignore it".

I'm... not sure. Asking @shana-apple since she may have an answer.

But actually this is interesting. I dig more into the code and I'm curious of what is the right fix here.

The current code flow is, when the pairing window is opened, the next available admin id is populated to the AdminPairingTable (in this case 0) without a node id. The reason for not assigning any node id is because it is currently assigned at

connectedhomeip/src/transport/SecureSessionMgr.cpp

Line 438 in 0bca292

// TODO: Remove temporary code once AddOptCert is implemented

The comment "AddOpCert is implemented" suggests that NodeId should be populated when an operational certificate is received ?

This will be resolved by my next patch. This wasn't just AddOpCert landing but further implementation is needed to hook it all up.

[vivien-apple]

After some investigations, it seems like the issue here is that the accessory is locally broadcasting using AdvertiseOperational instead of AdvertiseCommissionableNode. I will update the patch

[woody-apple]

/rebase

[woody-apple]

/rebase

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale]

This stale pull request has been automatically closed. Thank you for your contributions.