Address testing feedback from opcert callbacks, and re-commissioning #22369

chrisdecenzo · 2022-09-02T16:00:52Z

Problem

Bugs found during testing of new external opcert signing - [1.0] attestation elements and signature are not available during opcert signing #22419
Bug when attempting to commission an already commissioned device - [1.0] Commissioning an already-on-fabric device fails without useful information for commissioner #22421

Change overview

Fix typo on JNI callback lookup
Persist attestation elements and signature so they are available during opcert signing
Read fabric list during attribute read step of commissioning and persist the nodeId if the device is already on the commissioners fabric.
Add code in tv-app and content-app platform to handle the already-on-fabric case
Fixes [1.0] attestation elements and signature are not available during opcert signing #22419
Fixes [1.0] Commissioning an already-on-fabric device fails without useful information for commissioner #22421

Testing

Tested on Android
Tested on linux tv-casting-app and tv-app

src/controller/AutoCommissioner.cpp

src/controller/AutoCommissioner.h

src/controller/CHIPDeviceController.cpp

bzbarsky-apple · 2022-09-07T20:45:20Z

@chrisdecenzo For what it's worth, I think the right approach here at this stage to reduce risk in terms of the DeviceCommissioner changes might be for whatever consumers want the Fabrics attribute info to:

Use the EstablishPASEConnection API on ChipDeviceController to establish a PASE session, passing in a device id that will identify the commissioning process and the pairing code involved.
In the OnPairingComplete callback, if it succeeds, FindCommissioneeDevice with the same device id, and use that to send a read for Fabrics.
Do the sanity check for the fabric already existing. If it does, and the PASE session should just be closed, call StopPairing with the relevant device id.
If the fabric does not already exist, call Commission on the DeviceCommissioner to commission the device.

While it might be good if the SDK provided more built-in support for doing this check, I think it's worth thinking a bit about how exactly that should work in a way that does not hurt the common commissioning case and does not change the default commissioning flow from the one that has been widely tested. (For example, maybe the Fabrics read should only happen in the actual error case when we get the FabricConflict status?)

github-actions · 2022-09-09T20:06:47Z

All PRs require an issue to be accepted, please link an issue or mention it in the body using #<issue_id>

chrisdecenzo · 2022-09-09T20:18:50Z

I created #22525 to track work for making the already-on-fabric check happen by default (with a few suggestions for how best to do it).

Co-authored-by: Restyled.io <[email protected]>

github-actions · 2022-09-10T22:20:11Z

All PRs require an issue to be accepted, please link an issue or mention it in the body using #<issue_id>

chrisdecenzo · 2022-09-13T17:09:22Z

Fixes #22419

github-actions · 2022-09-13T17:10:08Z

All PRs require an issue to be accepted, please link an issue or mention it in the body using #<issue_id>

chrisdecenzo · 2022-09-13T17:11:57Z

Fixes #22419

github-actions · 2022-09-13T17:12:48Z

All PRs require an issue to be accepted, please link an issue or mention it in the body using #<issue_id>

src/controller/CHIPDeviceController.cpp

src/controller/CommissioningDelegate.h

andy31415 · 2022-09-14T18:26:52Z

src/controller/CHIPDeviceController.cpp

@@ -1997,7 +2055,8 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
        app::InteractionModelEngine * engine = app::InteractionModelEngine::GetInstance();
        app::ReadPrepareParams readParams(proxy->GetSecureSession().Value());

-        app::AttributePathParams readPaths[8];
+        // NOTE: this array cannot have more than 9 entries, since 9 is what the spec requires as a minimum on servers


This seems off: this says the spec requires a minimum but we use 9 as the maximum.

This needs revision.

"client cannot send more than 9 since spec mandates 9 as minimum on the server side"

…roject-chip#22369) * Draft: address testing feedback from new opcert callbacks * temporary workaround to isolate a memory corruption * add check for existing fabric during commissioning, handle correctly on content app platform * address comments * address comments * address comments * Restyled by clang-format (project-chip#22524) Co-authored-by: Restyled.io <[email protected]> * address comments Co-authored-by: restyled-io[bot] <32688539+restyled-io[bot]@users.noreply.github.com> Co-authored-by: Restyled.io <[email protected]>

Draft: address testing feedback from new opcert callbacks

0247d32

pullapprove bot requested review from anush-apple, arkq, Byungjoo-Lee, bzbarsky-apple, carol-apple, chshu, chulspro, Damian-Nordic, dhrishi, electrocucaracha, emargolis, erjiaqing, franck-apple, gjc13, harimau-qirex, harsha-rajendran, hawk248 and isiu-apple September 2, 2022 16:01

github-actions bot added the controller label Sep 2, 2022

pullapprove bot requested review from arkq, jelderton, Byungjoo-Lee and jepenven-silabs September 2, 2022 16:01

address comments

abe2d30

bzbarsky-apple requested changes Sep 7, 2022

View reviewed changes

address comments

31f2dc8

restyled-io bot mentioned this pull request Sep 9, 2022

Restyle Address testing feedback from opcert callbacks, and re-commissioning #22524

Merged

Restyled by clang-format (#22524)

6a625ad

Co-authored-by: Restyled.io <[email protected]>

chrisdecenzo requested a review from tcarmelveilleux September 13, 2022 17:55

tcarmelveilleux reviewed Sep 13, 2022

View reviewed changes

src/controller/CHIPDeviceController.cpp Outdated Show resolved Hide resolved

tcarmelveilleux approved these changes Sep 13, 2022

View reviewed changes

hawk248 approved these changes Sep 13, 2022

View reviewed changes

pullapprove bot added review - changed requested and removed review - pending labels Sep 13, 2022

bzbarsky-apple reviewed Sep 14, 2022

View reviewed changes

chrisdecenzo added 2 commits September 14, 2022 08:33

address comments

d55de3e

address comments

bc7c142

bzbarsky-apple approved these changes Sep 14, 2022

View reviewed changes

pullapprove bot added review - approved and removed review - changed requested labels Sep 14, 2022

chrisdecenzo merged commit 88fa9ab into master Sep 14, 2022

chrisdecenzo deleted the tvapps-android25 branch September 14, 2022 18:05

andy31415 reviewed Sep 14, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address testing feedback from opcert callbacks, and re-commissioning #22369

Address testing feedback from opcert callbacks, and re-commissioning #22369

chrisdecenzo commented Sep 2, 2022 •

edited

Loading

bzbarsky-apple commented Sep 7, 2022

github-actions bot commented Sep 9, 2022

chrisdecenzo commented Sep 9, 2022

github-actions bot commented Sep 10, 2022

chrisdecenzo commented Sep 13, 2022

github-actions bot commented Sep 13, 2022

chrisdecenzo commented Sep 13, 2022

github-actions bot commented Sep 13, 2022

andy31415 Sep 14, 2022

chrisdecenzo Sep 14, 2022

Address testing feedback from opcert callbacks, and re-commissioning #22369

Address testing feedback from opcert callbacks, and re-commissioning #22369

Conversation

chrisdecenzo commented Sep 2, 2022 • edited Loading

Problem

Change overview

Testing

bzbarsky-apple commented Sep 7, 2022

github-actions bot commented Sep 9, 2022

chrisdecenzo commented Sep 9, 2022

github-actions bot commented Sep 10, 2022

chrisdecenzo commented Sep 13, 2022

github-actions bot commented Sep 13, 2022

chrisdecenzo commented Sep 13, 2022

github-actions bot commented Sep 13, 2022

andy31415 Sep 14, 2022

Choose a reason for hiding this comment

chrisdecenzo Sep 14, 2022

Choose a reason for hiding this comment

chrisdecenzo commented Sep 2, 2022 •

edited

Loading