Skip to content

Commit

Permalink
Don't lose track of the "no compatible credentials" error in commissi…
Browse files Browse the repository at this point in the history
…oning. (#17662)

When AutoCommissioner::GetNextCommissioningStageInternal moves out of
the kSendNOC state, it can end up in one of three states:
kWiFiNetworkSetup, kThreadNetworkSetup, and kCleanup.  This third case
happens if we don't have credentials for the network technology the
commissionee supports.  In that case, we set the error outparam and
proceed to cleanup.

Unfortunately, the caller completely ignores the outparam, except for
logging.  So we get situations like
#17244 where the
log looks like this:

    CHIP:CTL: Finished commissioning step 'SendNOC' with error '../../../examples/chip-tool/third_party/connectedhomeip/src/controller/CHIPDeviceController.cpp:1390: Success'
    CHIP:CTL: Required network information not provided in commissioning parameters
    CHIP:CTL: Parameters supplied: wifi (yes) thread (no)
    CHIP:CTL: Device supports: wifi (no) thread(yes)
    CHIP:CTL: Going from commissioning step 'SendNOC' with lastErr = '../../../examples/chip-tool/third_party/connectedhomeip/src/controller/AutoCommissioner.cpp:180: CHIP Error 0x0000002F: Invalid argument' --> 'Cleanup'
    CHIP:CTL: Performing next commissioning step 'Cleanup' with completion status = '../../../examples/chip-tool/third_party/connectedhomeip/src/controller/CHIPDeviceController.cpp:1390: Success'
    CHIP:CTL: Finished commissioning step 'Cleanup' with error '../../../examples/chip-tool/third_party/connectedhomeip/src/controller/CHIPDeviceController.cpp:1497: Success'
    CHIP:TOO: Device commissioning completed with success

and that last line shows the pairing delegate was notified that
commissioning completed successfully, even though we bailed out
partway through.

The fix is to store the error in our completionStatus.  With this
change, the log ends up looking like this:

    CHIP: [CTL] Successfully finished commissioning step 'SendNOC'
    CHIP: [CTL] Required network information not provided in commissioning parameters
    CHIP: [CTL] Parameters supplied: wifi (no) thread (yes)
    CHIP: [CTL] Device supports: wifi (yes) thread(no)
    CHIP: [CTL] Going from commissioning step 'SendNOC' with lastErr = '../../../examples/chip-tool/third_party/connectedhomeip/src/controller/AutoCommissioner.cpp:203: CHIP Error 0x0000002F: Invalid argument' -> 'Cleanup'
    CHIP: [CTL] Performing next commissioning step 'Cleanup' with completion status = '../../../examples/chip-tool/third_party/connectedhomeip/src/controller/AutoCommissioner.cpp:203: CHIP Error 0x0000002F: Invalid argument'
    CHIP: [TOO] Device commissioning Failure: ../../../examples/chip-tool/third_party/connectedhomeip/src/controller/AutoCommissioner.cpp:203: CHIP Error 0x0000002F: Invalid argument

and we see the PairingDelegate is notified with an error status, as expected.

Another option would have been to add a new commissioning step for "no
network credentials" and set the completion status to error from that.
That would have the benefit of also setting the failedStage in the
completion status, which we can't easily set right now because part of
the problem is that we don't actually know what stage to proceed to
and hence which stage to deem as having failed.
  • Loading branch information
bzbarsky-apple authored and pull[bot] committed Nov 28, 2023
1 parent df440bb commit f265e3a
Showing 1 changed file with 8 additions and 0 deletions.
8 changes: 8 additions & 0 deletions src/controller/AutoCommissioner.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,14 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio
return CHIP_ERROR_INCORRECT_STATE;
}

// If GetNextCommissioningStage indicated a failure, don't lose track of
// that. But don't overwrite any existing failures we had hanging
// around.
if (completionStatus.err == CHIP_NO_ERROR)
{
completionStatus.err = err;
}

DeviceProxy * proxy = mCommissioneeDeviceProxy;
if (nextStage == CommissioningStage::kSendComplete ||
(nextStage == CommissioningStage::kCleanup && mOperationalDeviceProxy != nullptr))
Expand Down

0 comments on commit f265e3a

Please sign in to comment.