Skip to content

Commit

Permalink
Stop storing breadcrumb in persistent storage.
Browse files Browse the repository at this point in the history
Breadcrumb is not supposed to be stored persistently.

Also adds a bunch of tests and fixes various bugs where we were
setting breadcrumbs when we should not (e.g. in error conditions) and
not setting them when we should (e.g. when the fail-safe expires).

Fixes project-chip#17515
  • Loading branch information
bzbarsky-apple committed Apr 26, 2022
1 parent 59e6a7a commit 27e8a8c
Show file tree
Hide file tree
Showing 38 changed files with 567 additions and 115 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,6 @@ class GeneralCommissioningAttrAccess : public AttributeAccessInterface
GeneralCommissioningAttrAccess() : AttributeAccessInterface(Optional<EndpointId>::Missing(), GeneralCommissioning::Id) {}

CHIP_ERROR Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder) override;
CHIP_ERROR Write(const ConcreteDataAttributePath & aPath, AttributeValueDecoder & aDecoder) override;

private:
CHIP_ERROR ReadIfSupported(CHIP_ERROR (ConfigurationManager::*getter)(uint8_t &), AttributeValueEncoder & aEncoder);
Expand Down Expand Up @@ -100,37 +99,13 @@ CHIP_ERROR GeneralCommissioningAttrAccess::Read(const ConcreteReadAttributePath
case SupportsConcurrentConnection::Id: {
return ReadSupportsConcurrentConnection(aEncoder);
}
case Breadcrumb::Id: {
return aEncoder.Encode(DeviceLayer::DeviceControlServer::DeviceControlSvr().GetBreadcrumb());
}
default: {
break;
}
}
return CHIP_NO_ERROR;
}

CHIP_ERROR GeneralCommissioningAttrAccess::Write(const ConcreteDataAttributePath & aPath, AttributeValueDecoder & aDecoder)
{
// TODO: There was discussion about moving the breadcrumb to the attribute store, which would make this function obsolete

if (aPath.mClusterId != GeneralCommissioning::Id)
{
// We shouldn't have been called at all.
return CHIP_ERROR_INVALID_ARGUMENT;
}

switch (aPath.mAttributeId)
{
case Attributes::Breadcrumb::Id:
Attributes::Breadcrumb::TypeInfo::DecodableType value;
ReturnErrorOnFailure(aDecoder.Decode(value));
return DeviceLayer::DeviceControlServer::DeviceControlSvr().SetBreadcrumb(value);
default:
return CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE;
}
}

CHIP_ERROR GeneralCommissioningAttrAccess::ReadIfSupported(CHIP_ERROR (ConfigurationManager::*getter)(uint8_t &),
AttributeValueEncoder & aEncoder)
{
Expand Down Expand Up @@ -208,6 +183,8 @@ bool emberAfGeneralCommissioningClusterArmFailSafeCallback(app::CommandHandler *
{
// Force the timer to expire immediately.
failSafeContext.ForceFailSafeTimerExpiry();
// Don't set the breadcrumb, since expiring the failsafe should
// reset it anyway.
response.errorCode = CommissioningError::kOk;
commandObj->AddResponse(commandPath, response);
}
Expand All @@ -216,10 +193,10 @@ bool emberAfGeneralCommissioningClusterArmFailSafeCallback(app::CommandHandler *
CheckSuccess(
failSafeContext.ArmFailSafe(accessingFabricIndex, System::Clock::Seconds16(commandData.expiryLengthSeconds)),
Failure);
Breadcrumb::Set(commandPath.mEndpointId, commandData.breadcrumb);
response.errorCode = CommissioningError::kOk;
commandObj->AddResponse(commandPath, response);
}
DeviceLayer::DeviceControlServer::DeviceControlSvr().SetBreadcrumb(commandData.breadcrumb);
}
else
{
Expand Down Expand Up @@ -265,11 +242,11 @@ bool emberAfGeneralCommissioningClusterCommissioningCompleteCallback(
CheckSuccess(server->CommissioningComplete(handle->AsSecureSession()->GetPeerNodeId(), handle->GetFabricIndex()),
Failure);

Breadcrumb::Set(commandPath.mEndpointId, 0);
response.errorCode = CommissioningError::kOk;
}
}

DeviceLayer::DeviceControlServer::DeviceControlSvr().SetBreadcrumb(0);
commandObj->AddResponse(commandPath, response);

return true;
Expand All @@ -282,9 +259,8 @@ bool emberAfGeneralCommissioningClusterSetRegulatoryConfigCallback(app::CommandH
MATTER_TRACE_EVENT_SCOPE("SetRegulatoryConfig", "GeneralCommissioning");
DeviceControlServer * server = &DeviceLayer::DeviceControlServer::DeviceControlSvr();

CheckSuccess(server->SetRegulatoryConfig(to_underlying(commandData.newRegulatoryConfig), commandData.countryCode,
commandData.breadcrumb),
Failure);
CheckSuccess(server->SetRegulatoryConfig(to_underlying(commandData.newRegulatoryConfig), commandData.countryCode), Failure);
Breadcrumb::Set(commandPath.mEndpointId, commandData.breadcrumb);

Commands::SetRegulatoryConfigResponse::Type response;
response.errorCode = CommissioningError::kOk;
Expand All @@ -293,8 +269,21 @@ bool emberAfGeneralCommissioningClusterSetRegulatoryConfigCallback(app::CommandH
return true;
}

namespace {
void OnPlatformEventHandler(const DeviceLayer::ChipDeviceEvent * event, intptr_t arg)
{
if (event->Type == DeviceLayer::DeviceEventType::kFailSafeTimerExpired)
{
// Spec says to reset Breadcrumb attribute to 0.
Breadcrumb::Set(0, 0);
}
}

} // anonymous namespace

void MatterGeneralCommissioningPluginServerInitCallback()
{
DeviceLayer::DeviceControlServer::DeviceControlSvr().SetBreadcrumb(0);
Breadcrumb::Set(0, 0);
registerAttributeAccessOverride(&gAttrAccess);
DeviceLayer::PlatformMgrImpl().AddEventHandler(OnPlatformEventHandler);
}
214 changes: 214 additions & 0 deletions src/app/tests/suites/TestGeneralCommissioning.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@ config:
nodeId: 0x12344321
cluster: "General Commissioning"
endpoint: 0
discriminator:
type: INT16U
defaultValue: 3840
payload:
type: CHAR_STRING
defaultValue: "MT:-24J0AFN00KA0648G00" # This value needs to be generated automatically

tests:
- label: "Wait for the commissioned device to be retrieved"
Expand Down Expand Up @@ -52,6 +58,214 @@ tests:
response:
value: 81

- label: "Reboot to reset Breadcrumb"
cluster: "SystemCommands"
command: "Reboot"

- label: "Connect to the device again"
cluster: "DelayCommands"
command: "WaitForCommissionee"
arguments:
values:
- name: "nodeId"
value: nodeId

- label: "Read back Breadcrumb after reboot and ensure it was not persisted"
command: "readAttribute"
attribute: "Breadcrumb"
response:
value: 0

- label: "Set Breadcrumb to nonzero value"
command: "writeAttribute"
attribute: "Breadcrumb"
arguments:
value: 1

- label: "Check Breadcrumb set worked"
command: "readAttribute"
attribute: "Breadcrumb"
response:
value: 1

- label: "Send CommissioningComplete without armed fail-safe"
command: "CommissioningComplete"
response:
values:
- name: errorCode
value: 3 # NoFailSafe

- label: "Check Breadcrumb was not touched by invalid CommissioningComplete"
command: "readAttribute"
attribute: "Breadcrumb"
response:
value: 1

- label: "Open Commissioning Window from alpha"
cluster: "AdministratorCommissioning"
command: "OpenBasicCommissioningWindow"
timedInteractionTimeoutMs: 10000
arguments:
values:
- name: "CommissioningTimeout"
value: 180

# Arming a fail-safe over CASE while a commissioning window is open should not work.
- label: "Try to arm fail-safe"
command: "ArmFailSafe"
arguments:
values:
- name: expiryLengthSeconds
value: 10
- name: breadcrumb
value: 5000
response:
values:
- name: errorCode
value: 4 # BusyWithOtherAdmin

- label:
"Check Breadcrumb was not touched by ArmFailSafe with commissioning
window open"
command: "readAttribute"
attribute: "Breadcrumb"
response:
value: 1

- label: "Reset Breadcrumb to 0 so we can commission"
command: "writeAttribute"
attribute: "Breadcrumb"
arguments:
value: 0

- label: "Commission from beta"
identity: "beta"
cluster: "CommissionerCommands"
command: "PairWithQRCode"
arguments:
values:
- name: "nodeId"
value: 0x12345
- name: "payload"
value: payload

- label: "Wait for the commissioned device to be retrieved for beta"
identity: "beta"
cluster: "DelayCommands"
command: "WaitForCommissionee"
arguments:
values:
- name: "nodeId"
value: 0x12345

- label: "Arm fail-safe"
command: "ArmFailSafe"
arguments:
values:
- name: expiryLengthSeconds
value: 500
- name: breadcrumb
value: 2
response:
values:
- name: errorCode
value: 0 # OK

- label: "Check Breadcrumb was properly set by ArmFailSafe"
command: "readAttribute"
attribute: "Breadcrumb"
response:
value: 2

- label: "Try to arm fail-safe from wrong fabric"
command: "ArmFailSafe"
identity: "beta"
arguments:
values:
- name: expiryLengthSeconds
value: 10
- name: breadcrumb
value: 5000
response:
values:
- name: errorCode
value: 4 # BusyWithOtherAdmin

- label:
"Check Breadcrumb was not touched by ArmFailSafe with existing
fail-safe armed"
command: "readAttribute"
attribute: "Breadcrumb"
response:
value: 2

- label: "Send CommissioningComplete from wrong fabric"
command: "CommissioningComplete"
identity: "beta"
response:
values:
- name: errorCode
value: 2 # InvalidAuthentication

- label:
"Check Breadcrumb was not touched by CommissioningComplete from wrong
fabric"
command: "readAttribute"
attribute: "Breadcrumb"
response:
value: 2

- label: "Close out the fail-safe gracefully"
command: "CommissioningComplete"
response:
values:
- name: errorCode
value: 0 # Ok

- label: "Check Breadcrumb was reset to 0 by CommissioningComplete"
command: "readAttribute"
attribute: "Breadcrumb"
response:
value: 0

- label: "Arm fail-safe again"
command: "ArmFailSafe"
arguments:
values:
- name: expiryLengthSeconds
value: 500
- name: breadcrumb
value: 3
response:
values:
- name: errorCode
value: 0 # OK

- label: "Check Breadcrumb was set by arming fail-safe again"
command: "readAttribute"
attribute: "Breadcrumb"
response:
value: 3

- label: "Force-expire the fail-safe"
command: "ArmFailSafe"
arguments:
values:
- name: expiryLengthSeconds
value: 0
- name: breadcrumb
value: 4
response:
values:
- name: errorCode
value: 0 # OK

- label: "Check Breadcrumb was reset by expiring the fail-safe"
command: "readAttribute"
attribute: "Breadcrumb"
response:
value: 0

- label: "Validate presence of SupportsConcurrentConnection"
command: "readAttribute"
attribute: "SupportsConcurrentConnection"
Expand Down
4 changes: 3 additions & 1 deletion src/darwin/Framework/CHIP/templates/tests/tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,9 @@ function getTests()
'TestDelayCommands',
'TestDescriptorCluster',
'TestBasicInformation',
'TestGeneralCommissioning',
// TestGeneralCommissioning does reboots that need to have side-effects, so
// can't be tested in this test framework.
//'TestGeneralCommissioning',
'TestGroupsCluster',
'TestGroupKeyManagementCluster',
'TestIdentifyCluster',
Expand Down
2 changes: 0 additions & 2 deletions src/include/platform/ConfigurationManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,6 @@ class ConfigurationManager
#endif
virtual CHIP_ERROR GetRegulatoryLocation(uint8_t & location) = 0;
virtual CHIP_ERROR GetCountryCode(char * buf, size_t bufSize, size_t & codeLen) = 0;
virtual CHIP_ERROR GetBreadcrumb(uint64_t & breadcrumb) = 0;
virtual CHIP_ERROR StoreSerialNumber(const char * serialNum, size_t serialNumLen) = 0;
virtual CHIP_ERROR StorePrimaryWiFiMACAddress(const uint8_t * buf) = 0;
virtual CHIP_ERROR StorePrimary802154MACAddress(const uint8_t * buf) = 0;
Expand All @@ -118,7 +117,6 @@ class ConfigurationManager
virtual CHIP_ERROR StoreHardwareVersion(uint16_t hardwareVer) = 0;
virtual CHIP_ERROR StoreRegulatoryLocation(uint8_t location) = 0;
virtual CHIP_ERROR StoreCountryCode(const char * code, size_t codeLen) = 0;
virtual CHIP_ERROR StoreBreadcrumb(uint64_t breadcrumb) = 0;
virtual CHIP_ERROR GetRebootCount(uint32_t & rebootCount) = 0;
virtual CHIP_ERROR StoreRebootCount(uint32_t rebootCount) = 0;
virtual CHIP_ERROR GetTotalOperationalHours(uint32_t & totalOperationalHours) = 0;
Expand Down
4 changes: 1 addition & 3 deletions src/include/platform/DeviceControlServer.h
Original file line number Diff line number Diff line change
Expand Up @@ -89,14 +89,12 @@ class DeviceControlServer final
// ===== Members for internal use by other Device Layer components.

CHIP_ERROR CommissioningComplete(NodeId peerNodeId, FabricIndex accessingFabricIndex);
CHIP_ERROR SetRegulatoryConfig(uint8_t location, const CharSpan & countryCode, uint64_t breadcrumb);
CHIP_ERROR SetRegulatoryConfig(uint8_t location, const CharSpan & countryCode);
CHIP_ERROR ConnectNetworkForOperational(ByteSpan networkID);

void SetSwitchDelegate(SwitchDeviceControlDelegate * delegate) { mSwitchDelegate = delegate; }
SwitchDeviceControlDelegate * GetSwitchDelegate() const { return mSwitchDelegate; }
FailSafeContext & GetFailSafeContext() { return mFailSafeContext; }
CHIP_ERROR SetBreadcrumb(uint64_t breadcrumb);
uint64_t GetBreadcrumb();

static DeviceControlServer & DeviceControlSvr();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,6 @@ class GenericConfigurationManagerImpl : public ConfigurationManager
CHIP_ERROR StoreRegulatoryLocation(uint8_t location) override;
CHIP_ERROR GetCountryCode(char * buf, size_t bufSize, size_t & codeLen) override;
CHIP_ERROR StoreCountryCode(const char * code, size_t codeLen) override;
CHIP_ERROR GetBreadcrumb(uint64_t & breadcrumb) override;
CHIP_ERROR StoreBreadcrumb(uint64_t breadcrumb) override;
CHIP_ERROR GetRebootCount(uint32_t & rebootCount) override;
CHIP_ERROR StoreRebootCount(uint32_t rebootCount) override;
CHIP_ERROR GetTotalOperationalHours(uint32_t & totalOperationalHours) override;
Expand Down
Loading

0 comments on commit 27e8a8c

Please sign in to comment.