kSecurePairing not reported in OnCommissioningStatusUpdate()

[caipiblack]

Problem

On TE9 commit, the class DevicePairingDelegate define virtual methods that we can implement to notify the application about problems and steps during the commissioning.

The function OnCommissioningStatusUpdate() is called when each commissioning step defined in chip::Controller::CommissioningStage are finished except one: kSecurePairing

The step kSecurePairing have it's own callback OnStatusUpdate()

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

    /**
     * @brief
     *   Called when the pairing reaches a certain stage.
     *
     * @param status Current status of pairing
     */
    virtual void OnStatusUpdate(DevicePairingDelegate::Status status) {}

    virtual void OnCommissioningStatusUpdate(PeerId peerId, CommissioningStage stageCompleted, CHIP_ERROR error) {}

So my issue is just to know:

1. Is it wanted that kSecurePairing is not reported with OnCommissioningStatusUpdate() ?
2. Why not removing OnStatusUpdate() and using OnCommissioningStatusUpdate() for kSecurePairing ?

[caipiblack]

Also at the begining of the commissioning, if the device is not discoverable by BLE/WiFi/IP OnStatusUpdate() is called with DevicePairingDelegate::Status::SecurePairingFailed

But in case of success, it is not called at the same point (before everyhing else) with DevicePairingDelegate::Status::SecurePairingSuccess. Is it wanted ?

Some examples

Example of a success commissioning:

CommandPairing::OnPairingComplete(error=0)
CommandPairing::OnCommissioningStatusUpdate(nodeId=11, stageCompleted=2 , error=0)
CommandPairing::OnCommissioningStatusUpdate(nodeId=11, stageCompleted=3 , error=0)
CommandPairing::OnCommissioningStatusUpdate(nodeId=11, stageCompleted=4 , error=0)
CommandPairing::OnCommissioningStatusUpdate(nodeId=11, stageCompleted=5 , error=0)
CommandPairing::OnCommissioningStatusUpdate(nodeId=11, stageCompleted=6 , error=0)
CommandPairing::OnCommissioningStatusUpdate(nodeId=11, stageCompleted=7 , error=0)
CommandPairing::OnCommissioningStatusUpdate(nodeId=11, stageCompleted=8 , error=0)
CommandPairing::OnCommissioningStatusUpdate(nodeId=11, stageCompleted=9 , error=0)
CommandPairing::OnCommissioningStatusUpdate(nodeId=11, stageCompleted=10 , error=0)
CommandPairing::OnCommissioningStatusUpdate(nodeId=11, stageCompleted=11 , error=0)
CommandPairing::OnCommissioningStatusUpdate(nodeId=11, stageCompleted=12 , error=0)
CommandPairing::OnStatusUpdate(status=0)
CommandPairing::OnCommissioningStatusUpdate(nodeId=11, stageCompleted=13 , error=0)
CommandPairing::OnCommissioningStatusUpdate(nodeId=11, stageCompleted=14 , error=0)
CommandPairing::OnCommissioningStatusUpdate(nodeId=11, stageCompleted=16 , error=0)
Receive mDNS record: nodeId: 11
Receive mDNS record: nodeId: 11
CommandPairing::OnCommissioningStatusUpdate(nodeId=11, stageCompleted=18 , error=0)
CommandPairing::OnCommissioningStatusUpdate(nodeId=11, stageCompleted=19 , error=0)
CommandPairing::OnCommissioningStatusUpdate(nodeId=11, stageCompleted=20 , error=0)
CommandPairing::OnCommissioningComplete(deviceId=11, error=0)
CommandPairing::OnCommissioningSuccess(deviceId=11)

Example when the device is turned off during the commissioning:

CommandPairing::OnPairingComplete(error=0)
CommandPairing::OnCommissioningStatusUpdate(nodeId=13, stageCompleted=2 , error=0)
CommandPairing::OnCommissioningStatusUpdate(nodeId=13, stageCompleted=3 , error=0)
CommandPairing::OnCommissioningStatusUpdate(nodeId=13, stageCompleted=4 , error=0)
CommandPairing::OnCommissioningStatusUpdate(nodeId=13, stageCompleted=5 , error=0)
CommandPairing::OnCommissioningStatusUpdate(nodeId=13, stageCompleted=6 , error=0)
CommandPairing::OnCommissioningStatusUpdate(nodeId=13, stageCompleted=7 , error=0)
CommandPairing::OnCommissioningStatusUpdate(nodeId=13, stageCompleted=8 , error=0)
CommandPairing::OnCommissioningStatusUpdate(nodeId=13, stageCompleted=9 , error=0)
CommandPairing::OnCommissioningStatusUpdate(nodeId=13, stageCompleted=10 , error=0)
CommandPairing::OnCommissioningStatusUpdate(nodeId=13, stageCompleted=11 , error=0)
CommandPairing::OnCommissioningStatusUpdate(nodeId=13, stageCompleted=12 , error=0)
CommandPairing::OnCommissioningStatusUpdate(nodeId=13, stageCompleted=13 , error=50)
CommandPairing::OnCommissioningStatusUpdate(nodeId=13, stageCompleted=20 , error=0)
CommandPairing::OnCommissioningComplete(deviceId=13, error=50)
CommandPairing::OnCommissioningFailure(deviceId=13)

Example when the device is turned off before the commissioning

CommandPairing::OnStatusUpdate(status=1)

So what we can see:

# Called when we find the device over BLE/WiFi/IP at the begin of commissioning:
CommandPairing::OnPairingComplete(error=0)

# Called when we don't find the device over BLE/WiFi/IP at the begin of commissioning:
CommandPairing::OnStatusUpdate(status=1)

# Never called:
CommandPairing::OnCommissioningStatusUpdate(nodeId=13, stageCompleted=1 , error=0)

[bzbarsky-apple]

Is it wanted that kSecurePairing is not reported with OnCommissioningStatusUpdate() ?

There are two steps here: "pairing" and "commissioning"... The former means "establish a PASE session" and the latter is "everything after that".

But more fundamentally, there are APIs, people were trying to not break them, we added more notifications, and the result is a mess that is known to not be very good. See existing issues on this.

Why not removing OnStatusUpdate() and using OnCommissioningStatusUpdate() for kSecurePairing ?

See above.

Is it wanted ?

Probably not but see above about people adding things here without it really making sense necessarily...

[caipiblack]

I think, having some mechanisms to be notified about commissioning steps is a good idea for those (like us) building controller apps. It can be used to display the commissioning progress to the user (with progress circle, checks ..)

But currently we can't only use OnCommissioningStatusUpdate() because the step kSecurePairing is not reported. So we have to use OnStatusUpdate() to be notified about a failure and OnPairingComplete() to be notified about success.

I'm going to close this issue as there are issues talking about it

#15612