Fix the commissiong mode of "commissioning for on-network" usecases #8763

PSONALl · 2021-08-03T13:52:31Z

Enabled Commissionable Mode of the device for On-Network Commissioning

Change overview

Advertise Commissionable Node after connecting to AP
Check kIsServiceProvisioned, kOperationalDeviceCredentialsProvisioned flags, to check if device is fully provisioned

Testing

How was this tested? (at least one bullet point required)

Tested output of chip-device-ctrl with all-clusters-app example

dhrishi · 2021-08-03T14:18:12Z

examples/all-clusters-app/esp32/main/DeviceCallbacks.cpp

@@ -69,6 +69,9 @@ void DeviceCallbacks::DeviceEventCallback(const ChipDeviceEvent * event, intptr_
            // connectivity. MDNS still wants to refresh its listening interfaces to include the
            // newly selected address.
            chip::app::Mdns::StartServer();
+#ifdef RENDEZVOUS_WAIT_FOR_COMMISSIONING_COMPLETE
+            chip::app::Mdns::AdvertiseCommissionableNode();
+#endif


Better to do this though a CLI or shell command (similar to option present on M5stack through a button) instead of adding it here? Something like:
$ force-wifi-commissioning

dhrishi · 2021-08-03T14:18:51Z

src/include/platform/internal/GenericConfigurationManagerImpl.cpp

@@ -920,6 +920,7 @@ bool GenericConfigurationManagerImpl<ImplClass>::_IsFullyProvisioned()
 #if CHIP_DEVICE_CONFIG_ENABLE_JUST_IN_TIME_PROVISIONING
        (!UseManufacturerCredentialsAsOperational() && _OperationalDeviceCredentialsProvisioned()) &&
 #endif
+        (mFlags.Has(Flags::kIsServiceProvisioned) && mFlags.Has(Flags::kOperationalDeviceCredentialsProvisioned)) &&


Can someone check if this is the correct thing to do and won't break any other platform? cc @cecille @bzbarsky-apple

Is this change still needed now that #8454 has merged?

In any case, this does not look right because outside of TestConfigurationMgr it looks to me like kIsServiceProvisioned is never set. And kOperationalDeviceCredentialsProvisioned looks like it's never set at all.

@bzbarsky-apple , actually this change was added because in the case of On-Network commissioning _IsFullyProvisioned is returning true after getting credentials of AP, which causes code to break and Node doesn't advertise Commissionable Mode = 1 after it gets connected to AP. _IsFullyProvisioned should return false as the node is not fully provisioned yet, so to determine if the node is fully provisioned or not, It should check here if operational device credentials are provisioned, is this conclusion right?

I would pretty much assume that any use of IsFullyProvisioned is broken.... Whatever code is meant to advertise Commissionable Mode = 1 should presumably just check whether we have operational credentials directly. That's what Mdns::StartServer ends up doing (by calling the kinda-broken GetCurrentNodeId; we should have a way to actually ask the boolean "do we have an opcred?" question we really want to ask.

Does kOperationalDeviceCredentialsProvisioned ever get set?

Yes, You are right.. kOperationalDeviceCredentialsProvisioned is never set

Right. So fundamentally:

IsFullyProvisioned in its current form can't really be relied on to ever become true; see Operational advertisement on startup is broken on Linux for on-network pairing (and various other cases) #7911 for another example, where any device with thread or wifi support that is actually connecting via ethernet will always have IsFullyProvisioned false.

We should probably not add new conditions to IsFullyProvisioned

Whatever code is misbehaving because it uses IsFullyProvisioned should probably be changed to test the things it actually cares about.

github-actions · 2021-08-03T15:27:00Z

Size increase report for "esp32-example-build" from 9ca9d47

File	Section	File	VM
chip-lock-app.elf	.flash.text	20	20
chip-all-clusters-app.elf	.flash.text	32	32
chip-temperature-measurement-app.elf	.flash.text	20	20

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
.debug_line,0,24
.flash.text,20,20
.xt.prop._ZN4chip11DeviceLayer8Internal31GenericConfigurationManagerImplINS0_24ConfigurationManagerImplEE19_IsFullyProvisionedEv,0,12
.debug_loc,0,4
[Unmapped],0,-20

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

sections,vmsize,filesize

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

sections,vmsize,filesize
.debug_line,0,67
.debug_ranges,0,32
.flash.text,32,32
.debug_frame,0,16
.debug_loc,0,9
.riscv.attributes,0,1
.debug_info,0,-1
[Unmapped],0,-32

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

sections,vmsize,filesize
.debug_line,0,24
.flash.text,20,20
.xt.prop._ZN4chip11DeviceLayer8Internal31GenericConfigurationManagerImplINS0_24ConfigurationManagerImplEE19_IsFullyProvisionedEv,0,12
.debug_loc,0,-4
[Unmapped],0,-20

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

sections,vmsize,filesize
.debug_line,0,24

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

sections,vmsize,filesize

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

sections,vmsize,filesize

github-actions · 2021-08-03T15:27:29Z

Size increase report for "nrfconnect-example-build" from 9ca9d47

File	Section	File	VM
chip-lock.elf	text	20	20
chip-lock.elf	device_handles	-4	-4
chip-shell.elf	text	20	20
chip-shell.elf	device_handles	12	12

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
.debug_loc,0,72
.debug_line,0,56
.debug_info,0,48
text,20,20
.debug_frame,0,8
.debug_ranges,0,8
device_handles,-4,-4

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

sections,vmsize,filesize
.debug_line,0,60
.debug_loc,0,52
.debug_info,0,48
text,20,20
device_handles,12,12
.debug_frame,0,8
.debug_ranges,0,8

github-actions · 2021-08-03T15:28:28Z

Size increase report for "gn_qpg-example-build" from 9ca9d47

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
.debug_loc,0,71
.debug_line,0,58
.debug_info,0,48
.text,16,16
.debug_frame,0,8
.debug_ranges,0,8
.debug_str,0,3
[Unmapped],0,-16

bzbarsky-apple · 2021-08-03T16:46:03Z

src/include/platform/internal/GenericConfigurationManagerImpl.cpp

@@ -920,6 +920,7 @@ bool GenericConfigurationManagerImpl<ImplClass>::_IsFullyProvisioned()
 #if CHIP_DEVICE_CONFIG_ENABLE_JUST_IN_TIME_PROVISIONING
        (!UseManufacturerCredentialsAsOperational() && _OperationalDeviceCredentialsProvisioned()) &&
 #endif
+        (mFlags.Has(Flags::kIsServiceProvisioned) && mFlags.Has(Flags::kOperationalDeviceCredentialsProvisioned)) &&


Is this change still needed now that #8454 has merged?

In any case, this does not look right because outside of TestConfigurationMgr it looks to me like kIsServiceProvisioned is never set. And kOperationalDeviceCredentialsProvisioned looks like it's never set at all.

bzbarsky-apple · 2021-08-03T16:53:29Z

examples/all-clusters-app/esp32/main/DeviceCallbacks.cpp

@@ -69,6 +69,9 @@ void DeviceCallbacks::DeviceEventCallback(const ChipDeviceEvent * event, intptr_
            // connectivity. MDNS still wants to refresh its listening interfaces to include the
            // newly selected address.
            chip::app::Mdns::StartServer();
+#ifdef RENDEZVOUS_WAIT_FOR_COMMISSIONING_COMPLETE
+            chip::app::Mdns::AdvertiseCommissionableNode();


What is the actual intent of this change? Why do we want to:

Advertise being a commissionable node "unconditionally" (i.e. based on compile-time information only)

But only when RENDEZVOUS_WAIT_FOR_COMMISSIONING_COMPLETE, which is generally false

?

@bzbarsky-apple Intent of this change was to advertise the device as a commissionable node for On-Network Commissioning where RENDEZVOUS_WAIT_FOR_COMMISSIONING_COMPLETE is set.

I know what the code is doing. The question is why we want to advertise the device in that way exactly when that compile-time constant is set.

IsServiceProvisioned is legacy from OpenWeave data and the "service" provisioning data should not be touched/used.

RENDEZVOUS_WAIT_FOR_COMMISSIONING_COMPLETE should not be the gate. It's very clear what should cause commissionable for "already on network": an uncommissioned device but already connected to an IP network (e.g. Thread, Wifi, Ethernet) through non-Matter means, or a device already commissioned after receiving a request to open a commissioning window.

An commissioned device should not be advertising commissionable unless it can be be put into commissioning mode (e.g. some TV usecases).

stale · 2021-08-12T18:06:10Z

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 · 2021-08-19T21:35:53Z

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

* Committing the necessary changes before regen_all.py * Updated mode base to include cluster references so that zap_regen_all now works. * Added autogen code * Added new files created as part autogen. Also added missing data_model/clusters/Mode_DeviceEnergyManagement.xml * Fixed misspell * Added to python __init__.py * Restyled by isort * Updates to map into spec PR #8763 * Updated autogen for DEM Modes to include no optimization etc. * Updated data model for EEVSEM * Disabled ember for generating command hooks * Finally working * Restyled by gn * Updated zap files after merge to master. * Added support into Example EnergyManagementApp * Restyled by gn * Resolving merge conflict * Updated copyright to 2024 * Converted to using std::unique_ptr instead of new/delete * Made modes constexpr per review comment * Added Shutdown() to free Mode clusters and avoid VerifyOrDie failure at src/lib/support/IntrusiveList.h:290: Empty() * Copyright update * Copyright update Co-authored-by: Nivi Sarkar <[email protected]> --------- Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Nivi Sarkar <[email protected]>

boring-cyborg bot added the examples label Aug 3, 2021

restyled-io bot mentioned this pull request Aug 3, 2021

Restyle IP Commissioning Mode Support #8764

Closed

IP Commissioning Mode Support

231f4ab

PSONALl force-pushed the IP_Commissioning_Mode branch from 161bb6b to 231f4ab Compare August 3, 2021 14:13

pullapprove bot requested review from chrisdecenzo, jelderton, jmartinez-silabs, LuDuda, mspang, saurabhst and woody-apple August 3, 2021 14:14

pullapprove bot added the review - pending label Aug 3, 2021

dhrishi reviewed Aug 3, 2021

View reviewed changes

dhrishi changed the title ~~IP Commissioning Mode Support~~ Fix the commissiong mode of "commissioning for on-network" usecases Aug 3, 2021

bzbarsky-apple requested changes Aug 3, 2021

View reviewed changes

stale bot added the stale Stale issue or PR label Aug 12, 2021

stale bot closed this Aug 19, 2021

jamesharrow added a commit to jamesharrow/connectedhomeip that referenced this pull request Jan 18, 2024

Updates to map into spec PR project-chip#8763

5472c77

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the commissiong mode of "commissioning for on-network" usecases #8763

Fix the commissiong mode of "commissioning for on-network" usecases #8763

PSONALl commented Aug 3, 2021

dhrishi Aug 3, 2021

dhrishi Aug 3, 2021

bzbarsky-apple Aug 3, 2021

PSONALl Aug 4, 2021 •

edited

Loading

bzbarsky-apple Aug 5, 2021

PSONALl Aug 5, 2021

bzbarsky-apple Aug 5, 2021

github-actions bot commented Aug 3, 2021

github-actions bot commented Aug 3, 2021

github-actions bot commented Aug 3, 2021

bzbarsky-apple Aug 3, 2021

bzbarsky-apple Aug 3, 2021

PSONALl Aug 4, 2021

bzbarsky-apple Aug 5, 2021

tcarmelveilleux Aug 5, 2021

stale bot commented Aug 12, 2021

stale bot commented Aug 19, 2021

Fix the commissiong mode of "commissioning for on-network" usecases #8763

Fix the commissiong mode of "commissioning for on-network" usecases #8763

Conversation

PSONALl commented Aug 3, 2021

Change overview

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PSONALl Aug 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 3, 2021

github-actions bot commented Aug 3, 2021

github-actions bot commented Aug 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Aug 12, 2021

stale bot commented Aug 19, 2021

PSONALl Aug 4, 2021 •

edited

Loading