Make PairingDelegate callbacks clearer

[bzbarsky-apple]

Problem

PairingDelegate is a mishmash of "notify about PASE established", "notify about CASE established" (using the same callback!), etc.

When you get OnPairingComplete you have no idea what it actually means, especially if you are trying to commission one device while establishing CASE to another unrelated device.

Proposed Solution

Come up with a clear definition of what the callbacks on this interface mean (changing the interface as needed in the process), implement them.

[cecille]

related - #15409. I think that issue is s result of different assumptions about what the pairing callback means.

[caipiblack]

Currently If I do the commissioning of a thread device from our application, I have to check the Matter logs to know what happens.

I think it should be good to implement more states in the OnStatusUpdate() callback so we can use it to display "commissioning status" on the final application.

For example:

• Searching the device (BLE, SoftAP, OnNetwork)
• Device discovered (BLE, SoftAP, OnNetwork)
• Waiting for the device in the end network (should be called when we know the device is going to connect on final network)
• Device discovered in the end network

Or better: add a status for each commissioning steps in chip::Controller::DevicePairingDelegate::Status !

    enum Status : uint8_t
    {
        SecurePairingSuccess = 0,
        SecurePairingFailed,
    };

    /////////// callbacks from DevicePairingDelegate /////////
    void OnStatusUpdate(chip::Controller::DevicePairingDelegate::Status status) override;
    void OnPairingComplete(CHIP_ERROR error) override;
    void OnCommissioningComplete(NodeId deviceId, CHIP_ERROR error) override;

Edit 10/06/2022:

My previous remarks are not up-to-date now as some changes has been made. Now we have a callback to be notified about commissioning steps changes (OnCommissioningStatusUpdate())

But I have new remarks:

• Currently kSecurePairing is not reported in OnCommissioningStatusUpdate() (but it is present in chip::Controller::CommissioningStage).
• We have to use OnStatusUpdate() to be notified about kSecurePairing failure.
• We have to use OnPairingComplete() to be notified about kSecurePairing success.

As discussed here this is probably due to the fact that kSecurePairing is related to "pairing" (establishment of PASE session) and everything else is "commissioning".

Propositions:

• Regroup the "step change notifications" in the same callback for kSecurePairing and everything else
• Or if we want to keep it in different callbacks, refactor/regroup OnStatusUpdate() and OnPairingComplete()

[stale]

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

[tehampson]

Just to add more context to this issue so that when someone is working on a solution they get a slightly bigger picture of that was spotted while working on #24919.

Currently when SetUpCodePairer::OnDeviceDiscoveredTimeoutCallback it will call OnSessionEstablishmentError which ends up calling DeviceCommissioner::RendezvousCleanup. In the case where discovery times out, because we have no PASE connection being established this particular case is not calling OnPairingComplete. Because they are not getting OnPairingComplete callback they have hooked into OnStatusUpdate where status == Status::SecurePairingFailed where they are not getting the proper error code. When considering a solution to make PairingDelegate callbacks clearer, consider the case where discovery fails and what kind of call back would be more clear

[stale]

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