Skip to content

Commit

Permalink
[controller] merge ReadCommissioningInfo2 into ReadCommissioningInfo (#…
Browse files Browse the repository at this point in the history 
…30978)

* [controller] merge ReadCommissioningInfo2 into ReadCommissioningInfo

* Update src/controller/CHIPDeviceController.cpp

Co-authored-by: Boris Zbarsky <[email protected]>

* Update src/controller/CHIPDeviceController.cpp

Co-authored-by: Boris Zbarsky <[email protected]>

* Update src/controller/CHIPDeviceController.cpp

Co-authored-by: Boris Zbarsky <[email protected]>

* Update src/controller/CHIPDeviceController.cpp

Co-authored-by: Boris Zbarsky <[email protected]>

* address comment

---------

Co-authored-by: yunhanw-google <[email protected]>
Co-authored-by: Boris Zbarsky <[email protected]>
  • Loading branch information
@erjiaqing @yunhanw-google @bzbarsky-apple
3 people authored Dec 14, 2023
1 parent bbc6212 commit 8e3d98c
Show file tree
Hide file tree
Showing 5 changed files with 86 additions and 68 deletions.
23 changes: 11 additions & 12 deletions src/controller/AutoCommissioner.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -725,6 +725,15 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio
switch (report.stageCompleted)
{
case CommissioningStage::kReadCommissioningInfo:
break;
case CommissioningStage::kReadCommissioningInfo2: {
if (!report.Is<ReadCommissioningInfo>())
{
ChipLogError(Controller,
"[BUG] Should read commissioning info, but report is not ReadCommissioningInfo. THIS IS A BUG.");
}
ReadCommissioningInfo commissioningInfo = report.Get<ReadCommissioningInfo>();

mDeviceCommissioningInfo = report.Get<ReadCommissioningInfo>();
if (!mParams.GetFailsafeTimerSeconds().HasValue() && mDeviceCommissioningInfo.general.recommendedFailsafe > 0)
{
Expand All @@ -736,22 +745,12 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio
.SetLocationCapability(mDeviceCommissioningInfo.general.locationCapability);
// Don't send DST unless the device says it needs it
mNeedsDST = false;
break;
case CommissioningStage::kReadCommissioningInfo2: {

if (!report.Is<ReadCommissioningInfo2>())
{
ChipLogError(
Controller,
"[BUG] Should read commissioning info (part 2), but report is not ReadCommissioningInfo2. THIS IS A BUG.");
}

ReadCommissioningInfo2 commissioningInfo = report.Get<ReadCommissioningInfo2>();
mParams.SetSupportsConcurrentConnection(commissioningInfo.supportsConcurrentConnection);

if (mParams.GetCheckForMatchingFabric())
{
chip::NodeId nodeId = commissioningInfo.nodeId;
chip::NodeId nodeId = commissioningInfo.remoteNodeId;
if (nodeId != kUndefinedNodeId)
{
mParams.SetRemoteNodeId(nodeId);
Expand All @@ -760,7 +759,7 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio

if (mParams.GetICDRegistrationStrategy() != ICDRegistrationStrategy::kIgnore)
{
if (commissioningInfo.isIcd && commissioningInfo.checkInProtocolSupport)
if (commissioningInfo.isLIT && commissioningInfo.checkInProtocolSupport)
{
mNeedIcdRegistration = true;
ChipLogDetail(Controller, "AutoCommissioner: ICD supports the check-in protocol.");
Expand Down
89 changes: 55 additions & 34 deletions src/controller/CHIPDeviceController.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1922,25 +1922,50 @@ void DeviceCommissioner::OnDeviceConnectionRetryFn(void * context, const ScopedN
// ClusterStateCache::Callback impl
void DeviceCommissioner::OnDone(app::ReadClient *)
{
mReadClient = nullptr;
switch (mCommissioningStage)
{
case CommissioningStage::kReadCommissioningInfo:
ParseCommissioningInfo();
// Silently complete the stage, data will be saved in attribute cache and
// will be parsed after all ReadCommissioningInfo stages are completed.
CommissioningStageComplete(CHIP_NO_ERROR);
break;
case CommissioningStage::kReadCommissioningInfo2:
ParseCommissioningInfo2();
// Note: Only parse commissioning info in the last ReadCommissioningInfo stage.
ParseCommissioningInfo();
break;
default:
// We're not trying to read anything here, just exit
break;
}
}

void DeviceCommissioner::ParseCommissioningInfo()
{
CHIP_ERROR err = CHIP_NO_ERROR;
ReadCommissioningInfo info;

err = ParseCommissioningInfo1(info);
if (err == CHIP_NO_ERROR)
{
err = ParseCommissioningInfo2(info);
}

mAttributeCache = nullptr;

if (mPairingDelegate != nullptr)
{
mPairingDelegate->OnReadCommissioningInfo(info);
}

CommissioningDelegate::CommissioningReport report;
report.Set<ReadCommissioningInfo>(info);
CommissioningStageComplete(err, report);
}

CHIP_ERROR DeviceCommissioner::ParseCommissioningInfo1(ReadCommissioningInfo & info)
{
CHIP_ERROR err;
CHIP_ERROR return_err = CHIP_NO_ERROR;
ReadCommissioningInfo info;

// Try to parse as much as we can here before returning, even if attributes
// are missing or cannot be decoded.
Expand Down Expand Up @@ -2080,17 +2105,8 @@ void DeviceCommissioner::ParseCommissioningInfo()
{
ChipLogError(Controller, "Error parsing commissioning information");
}
mAttributeCache = nullptr;
mReadClient = nullptr;

if (mPairingDelegate != nullptr)
{
mPairingDelegate->OnReadCommissioningInfo(info);
}

CommissioningDelegate::CommissioningReport report;
report.Set<ReadCommissioningInfo>(info);
CommissioningStageComplete(return_err, report);
return return_err;
}

void DeviceCommissioner::ParseTimeSyncInfo(ReadCommissioningInfo & info)
Expand Down Expand Up @@ -2153,34 +2169,31 @@ void DeviceCommissioner::ParseTimeSyncInfo(ReadCommissioningInfo & info)
}
}

void DeviceCommissioner::ParseCommissioningInfo2()
CHIP_ERROR DeviceCommissioner::ParseCommissioningInfo2(ReadCommissioningInfo & info)
{
ReadCommissioningInfo2 info;
CHIP_ERROR return_err = CHIP_NO_ERROR;
CHIP_ERROR err = CHIP_NO_ERROR;

using namespace chip::app::Clusters::GeneralCommissioning::Attributes;
CHIP_ERROR err =
mAttributeCache->Get<SupportsConcurrentConnection::TypeInfo>(kRootEndpointId, info.supportsConcurrentConnection);
if (err != CHIP_NO_ERROR)

if (mAttributeCache->Get<SupportsConcurrentConnection::TypeInfo>(kRootEndpointId, info.supportsConcurrentConnection) !=
CHIP_NO_ERROR)
{
// May not be present so don't return the error code, non fatal, default concurrent
ChipLogError(Controller, "Failed to read SupportsConcurrentConnection: %" CHIP_ERROR_FORMAT, err.Format());
info.supportsConcurrentConnection = true;
}

return_err = ParseFabrics(info);
err = ParseFabrics(info);

if (return_err == CHIP_NO_ERROR)
if (err == CHIP_NO_ERROR)
{
return_err = ParseICDInfo(info);
err = ParseICDInfo(info);
}

CommissioningDelegate::CommissioningReport report;
report.Set<ReadCommissioningInfo2>(info);
CommissioningStageComplete(return_err, report);
return err;
}

CHIP_ERROR DeviceCommissioner::ParseFabrics(ReadCommissioningInfo2 & info)
CHIP_ERROR DeviceCommissioner::ParseFabrics(ReadCommissioningInfo & info)
{
CHIP_ERROR err;
CHIP_ERROR return_err = CHIP_NO_ERROR;
Expand Down Expand Up @@ -2230,7 +2243,7 @@ CHIP_ERROR DeviceCommissioner::ParseFabrics(ReadCommissioningInfo2 & info)
else if (commissionerRootPublicKey.Matches(deviceRootPublicKey))
{
ChipLogProgress(Controller, "DeviceCommissioner::OnDone - fabric root keys match");
info.nodeId = fabricDescriptor.nodeID;
info.remoteNodeId = fabricDescriptor.nodeID;
}
}
}
Expand All @@ -2244,28 +2257,28 @@ CHIP_ERROR DeviceCommissioner::ParseFabrics(ReadCommissioningInfo2 & info)

if (mPairingDelegate != nullptr)
{
mPairingDelegate->OnFabricCheck(info.nodeId);
mPairingDelegate->OnFabricCheck(info.remoteNodeId);
}

return return_err;
}

CHIP_ERROR DeviceCommissioner::ParseICDInfo(ReadCommissioningInfo2 & info)
CHIP_ERROR DeviceCommissioner::ParseICDInfo(ReadCommissioningInfo & info)
{
CHIP_ERROR err;
IcdManagement::Attributes::FeatureMap::TypeInfo::DecodableType featureMap;

err = mAttributeCache->Get<IcdManagement::Attributes::FeatureMap::TypeInfo>(kRootEndpointId, featureMap);
if (err == CHIP_NO_ERROR)
{
info.isIcd = true;
info.isLIT = true;
info.checkInProtocolSupport = !!(featureMap & to_underlying(IcdManagement::Feature::kCheckInProtocolSupport));
}
else if (err == CHIP_ERROR_KEY_NOT_FOUND)
{
// This key is optional so not an error
err = CHIP_NO_ERROR;
info.isIcd = false;
info.isLIT = false;
err = CHIP_NO_ERROR;
}
else if (err == CHIP_ERROR_IM_STATUS_CODE_RECEIVED)
Expand All @@ -2277,7 +2290,7 @@ CHIP_ERROR DeviceCommissioner::ParseICDInfo(ReadCommissioningInfo2 & info)
{
if (statusIB.mStatus == Protocols::InteractionModel::Status::UnsupportedCluster)
{
info.isIcd = false;
info.isLIT = false;
}
else
{
Expand Down Expand Up @@ -2458,7 +2471,8 @@ void DeviceCommissioner::SendCommissioningReadRequest(DeviceProxy * proxy, Optio
readParams.mpAttributePathParamsList = readPaths;
readParams.mAttributePathParamsListSize = readPathsSize;

auto attributeCache = Platform::MakeUnique<app::ClusterStateCache>(*this);
// Take ownership of the attribute cache, so it can be released when SendRequest fails.
auto attributeCache = std::move(mAttributeCache);
auto readClient = chip::Platform::MakeUnique<app::ReadClient>(
engine, proxy->GetExchangeManager(), attributeCache->GetBufferedCallback(), app::ReadClient::InteractionType::Read);
CHIP_ERROR err = readClient->SendRequest(readParams);
Expand Down Expand Up @@ -2509,6 +2523,13 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
break;
case CommissioningStage::kReadCommissioningInfo: {
ChipLogProgress(Controller, "Sending read request for commissioning information");
// Allocate a new ClusterStateCache when starting reading the first batch of attributes.
// The cache will be released in:
// - SendCommissioningReadRequest when failing to send a read request.
// - ParseCommissioningInfo when the last ReadCommissioningInfo stage is completed.
// Currently, we have two ReadCommissioningInfo* stages.
mAttributeCache = Platform::MakeUnique<app::ClusterStateCache>(*this);

// NOTE: this array cannot have more than 9 entries, since the spec mandates that server only needs to support 9
// See R1.1, 2.11.2 Interaction Model Limits
app::AttributePathParams readPaths[9];
Expand Down
10 changes: 6 additions & 4 deletions src/controller/CHIPDeviceController.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -952,12 +952,14 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
void SendCommissioningReadRequest(DeviceProxy * proxy, Optional<System::Clock::Timeout> timeout,
app::AttributePathParams * readPaths, size_t readPathsSize);
#if CHIP_CONFIG_ENABLE_READ_CLIENT
// Parsers for the two different read clients
void ParseCommissioningInfo();
void ParseCommissioningInfo2();
// Parsing attributes read in kReadCommissioningInfo stage.
CHIP_ERROR ParseCommissioningInfo1(ReadCommissioningInfo & info);
// Parsing attributes read in kReadCommissioningInfo2 stage.
CHIP_ERROR ParseCommissioningInfo2(ReadCommissioningInfo & info);
// Called by ParseCommissioningInfo2
CHIP_ERROR ParseFabrics(ReadCommissioningInfo2 & info);
CHIP_ERROR ParseICDInfo(ReadCommissioningInfo2 & info);
CHIP_ERROR ParseFabrics(ReadCommissioningInfo & info);
CHIP_ERROR ParseICDInfo(ReadCommissioningInfo & info);
// Called by ParseCommissioningInfo
void ParseTimeSyncInfo(ReadCommissioningInfo & info);
#endif // CHIP_CONFIG_ENABLE_READ_CLIENT
Expand Down
30 changes: 13 additions & 17 deletions src/controller/CommissioningDelegate.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -684,18 +684,14 @@ struct ReadCommissioningInfo
NetworkClusters network;
BasicClusterInfo basic;
GeneralCommissioningInfo general;
bool requiresUTC = false;
bool requiresTimeZone = false;
bool requiresDefaultNTP = false;
bool requiresTrustedTimeSource = false;
uint8_t maxTimeZoneSize = 1;
uint8_t maxDSTSize = 1;
};

struct ReadCommissioningInfo2
{
NodeId nodeId = kUndefinedNodeId;
bool isIcd = false;
bool requiresUTC = false;
bool requiresTimeZone = false;
bool requiresDefaultNTP = false;
bool requiresTrustedTimeSource = false;
uint8_t maxTimeZoneSize = 1;
uint8_t maxDSTSize = 1;
NodeId remoteNodeId = kUndefinedNodeId;
bool isLIT = false;
bool checkInProtocolSupport = false;
bool supportsConcurrentConnection = true;
};
Expand Down Expand Up @@ -730,8 +726,8 @@ class CommissioningDelegate
public:
virtual ~CommissioningDelegate(){};
/* CommissioningReport is returned after each commissioning step is completed. The reports for each step are:
* kReadCommissioningInfo: ReadCommissioningInfo
* kReadCommissioningInfo2: ReadCommissioningInfo2
* kReadCommissioningInfo: Reported together with ReadCommissioningInfo2
* kReadCommissioningInfo2: ReadCommissioningInfo
* kArmFailsafe: CommissioningErrorInfo if there is an error
* kConfigRegulatory: CommissioningErrorInfo if there is an error
* kConfigureUTCTime: None
Expand All @@ -755,9 +751,9 @@ class CommissioningDelegate
* kSendComplete: CommissioningErrorInfo if there is an error
* kCleanup: none
*/
struct CommissioningReport : Variant<RequestedCertificate, AttestationResponse, CSRResponse, NocChain, OperationalNodeFoundData,
ReadCommissioningInfo, ReadCommissioningInfo2, AttestationErrorInfo,
CommissioningErrorInfo, NetworkCommissioningStatusInfo, TimeZoneResponseInfo>
struct CommissioningReport
: Variant<RequestedCertificate, AttestationResponse, CSRResponse, NocChain, OperationalNodeFoundData, ReadCommissioningInfo,
AttestationErrorInfo, CommissioningErrorInfo, NetworkCommissioningStatusInfo, TimeZoneResponseInfo>
{
CommissioningReport() : stageCompleted(CommissioningStage::kError) {}
CommissioningStage stageCompleted;
Expand Down
2 changes: 1 addition & 1 deletion src/controller/python/OpCredsBinding.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ class TestCommissioner : public chip::Controller::AutoCommissioner
mCompletionError = err;
}
}
if (report.stageCompleted == chip::Controller::CommissioningStage::kReadCommissioningInfo)
if (report.stageCompleted == chip::Controller::CommissioningStage::kReadCommissioningInfo2)
{
mReadCommissioningInfo = report.Get<chip::Controller::ReadCommissioningInfo>();
}
Expand Down

0 comments on commit 8e3d98c

Please sign in to comment.