Skip to content

Commit

Permalink
Fix reporting for Administrator Commissioning attributes. (#23589)
Browse files Browse the repository at this point in the history
* Fix reporting for Administrator Commissioning attributes.

Nothing was marking the attributes dirty when the data backing them changed.

* Remove the dependency of af-types.h from reporting.h, so reporting.h can be
  part of the SDK library, not the app.
* Move reporting.h to the same library as the other reporting files.
* Fix the one example app that was using the EmberAfAttributeType bits in
  reporting.h.
* Fix color control server depending on reporting.h to get af-types.h.
* Add convenience MakeNullable methods, like we have for Optional.
* Make sure we call MatterReportingAttributeChangeCallback as needed in
  CommissioningWindowManager.
* Add a unit test that fails without these changes.
* Fix the existing TestCommissioningWindow unit test to not hardcode fabric
  indices, since that's not needed anymore.

* Address review comment.
  • Loading branch information
bzbarsky-apple authored Nov 18, 2022
1 parent 3198f01 commit 95d337c
Show file tree
Hide file tree
Showing 17 changed files with 1,362 additions and 79 deletions.
4 changes: 1 addition & 3 deletions examples/bridge-app/linux/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -420,9 +420,7 @@ void HandleDevicePowerSourceStatusChanged(DevicePowerSource * dev, DevicePowerSo

if (itemChangedMask & DevicePowerSource::kChanged_BatLevel)
{
uint8_t batChargeLevel = dev->GetBatChargeLevel();
MatterReportingAttributeChangeCallback(dev->GetEndpointId(), PowerSource::Id, PowerSource::Attributes::BatChargeLevel::Id,
ZCL_INT8U_ATTRIBUTE_TYPE, &batChargeLevel);
MatterReportingAttributeChangeCallback(dev->GetEndpointId(), PowerSource::Id, PowerSource::Attributes::BatChargeLevel::Id);
}

if (itemChangedMask & DevicePowerSource::kChanged_Description)
Expand Down
1 change: 1 addition & 0 deletions src/app/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ static_library("app") {
"WriteHandler.cpp",
"reporting/Engine.cpp",
"reporting/Engine.h",
"reporting/reporting.h",
]

public_deps = [
Expand Down
1 change: 0 additions & 1 deletion src/app/chip_data_model.gni
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,6 @@ template("chip_data_model") {
"${_app_root}/clusters/on-off-server/on-off-server.h",
"${_app_root}/clusters/scenes/scenes-tokens.h",
"${_app_root}/clusters/scenes/scenes.h",
"${_app_root}/reporting/reporting.h",
"${_app_root}/util/DataModelHandler.cpp",
"${_app_root}/util/af-event.cpp",
"${_app_root}/util/attribute-size-util.cpp",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

#include <app-common/zap-generated/cluster-objects.h>
#include <app/ConcreteCommandPath.h>
#include <app/reporting/reporting.h>
#include <app/util/af-types.h>
#include <app/util/basic-types.h>

/**********************************************************
Expand Down
12 changes: 12 additions & 0 deletions src/app/data-model/Nullable.h
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,18 @@ struct Nullable : protected Optional<T>
bool operator!=(const Nullable & other) const { return !(*this == other); }
};

template <class T>
constexpr Nullable<std::decay_t<T>> MakeNullable(T && value)
{
return Nullable<std::decay_t<T>>(InPlace, std::forward<T>(value));
}

template <class T, class... Args>
constexpr Nullable<T> MakeNullable(Args &&... args)
{
return Nullable<T>(InPlace, std::forward<Args>(args)...);
}

} // namespace DataModel
} // namespace app
} // namespace chip
7 changes: 7 additions & 0 deletions src/app/reporting/Engine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -964,3 +964,10 @@ void Engine::ScheduleUrgentEventDeliverySync(Optional<FabricIndex> fabricIndex)

void __attribute__((weak)) MatterPreAttributeReadCallback(const chip::app::ConcreteAttributePath & attributePath) {}
void __attribute__((weak)) MatterPostAttributeReadCallback(const chip::app::ConcreteAttributePath & attributePath) {}

// TODO: MatterReportingAttributeChangeCallback should just live in libCHIP,
// instead of being in ember-compatibility-functions. It does not depend on any
// app-specific generated bits.
void __attribute__((weak))
MatterReportingAttributeChangeCallback(chip::EndpointId endpoint, chip::ClusterId clusterId, chip::AttributeId attributeId)
{}
11 changes: 2 additions & 9 deletions src/app/reporting/reporting.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,13 @@
#pragma once

#include <app/ConcreteAttributePath.h>
#include <app/util/af-types.h>

/** @brief Reporting Attribute Change
*
* This function is called by the framework when an attribute managed by the
* framework changes. The application should call this function when an
* externally-managed attribute changes. The application should use the change
* notification to inform its reporting decisions.
*/
void MatterReportingAttributeChangeCallback(chip::EndpointId endpoint, chip::ClusterId clusterId, chip::AttributeId attributeId,
EmberAfAttributeType type, uint8_t * data);

/*
* Same but with just an attribute path and no data available.
* externally-managed attribute changes. This function triggers attribute
* reports for subscriptions as needed.
*/
void MatterReportingAttributeChangeCallback(chip::EndpointId endpoint, chip::ClusterId clusterId, chip::AttributeId attributeId);

Expand Down
76 changes: 66 additions & 10 deletions src/app/server/CommissioningWindowManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
* limitations under the License.
*/

#include <app/reporting/reporting.h>
#include <app/server/CommissioningWindowManager.h>
#include <app/server/Dnssd.h>
#include <app/server/Server.h>
Expand All @@ -28,6 +29,9 @@ using namespace chip::app::Clusters;
using namespace chip::System::Clock;

using AdministratorCommissioning::CommissioningWindowStatus;
using chip::app::DataModel::MakeNullable;
using chip::app::DataModel::Nullable;
using chip::app::DataModel::NullNullable;

namespace {

Expand Down Expand Up @@ -93,10 +97,11 @@ void CommissioningWindowManager::ResetState()
mECMDiscriminator = 0;
mECMIterations = 0;
mECMSaltLength = 0;
mWindowStatus = CommissioningWindowStatus::kWindowNotOpen;

mOpenerFabricIndex.SetNull();
mOpenerVendorId.SetNull();
UpdateWindowStatus(CommissioningWindowStatus::kWindowNotOpen);

UpdateOpenerFabricIndex(NullNullable);
UpdateOpenerVendorId(NullNullable);

memset(&mECMPASEVerifier, 0, sizeof(mECMPASEVerifier));
memset(mECMSalt, 0, sizeof(mECMSalt));
Expand Down Expand Up @@ -294,8 +299,8 @@ CommissioningWindowManager::OpenBasicCommissioningWindowForAdministratorCommissi
{
ReturnErrorOnFailure(OpenBasicCommissioningWindow(commissioningTimeout, CommissioningWindowAdvertisement::kDnssdOnly));

mOpenerFabricIndex.SetNonNull(fabricIndex);
mOpenerVendorId.SetNonNull(vendorId);
UpdateOpenerFabricIndex(MakeNullable(fabricIndex));
UpdateOpenerVendorId(MakeNullable(vendorId));

return CHIP_NO_ERROR;
}
Expand Down Expand Up @@ -329,8 +334,8 @@ CHIP_ERROR CommissioningWindowManager::OpenEnhancedCommissioningWindow(Seconds16
}
else
{
mOpenerFabricIndex.SetNonNull(fabricIndex);
mOpenerVendorId.SetNonNull(vendorId);
UpdateOpenerFabricIndex(MakeNullable(fabricIndex));
UpdateOpenerVendorId(MakeNullable(vendorId));
}

return err;
Expand All @@ -356,6 +361,10 @@ void CommissioningWindowManager::CloseCommissioningWindow()

CommissioningWindowStatus CommissioningWindowManager::CommissioningWindowStatusForCluster() const
{
// If the condition we use to determine whether we were opened via the
// cluster ever changes, make sure whatever code affects that condition
// marks calls MatterReportingAttributeChangeCallback for WindowStatus as
// needed.
if (mOpenerVendorId.IsNull())
{
// Not opened via the cluster.
Expand All @@ -375,7 +384,7 @@ void CommissioningWindowManager::OnFabricRemoved(FabricIndex removedIndex)
if (!mOpenerFabricIndex.IsNull() && mOpenerFabricIndex.Value() == removedIndex)
{
// Per spec, we should clear out the stale fabric index.
mOpenerFabricIndex.SetNull();
UpdateOpenerFabricIndex(NullNullable);
}
}

Expand Down Expand Up @@ -424,11 +433,11 @@ CHIP_ERROR CommissioningWindowManager::StartAdvertisement()

if (mUseECM)
{
mWindowStatus = CommissioningWindowStatus::kEnhancedWindowOpen;
UpdateWindowStatus(CommissioningWindowStatus::kEnhancedWindowOpen);
}
else
{
mWindowStatus = CommissioningWindowStatus::kBasicWindowOpen;
UpdateWindowStatus(CommissioningWindowStatus::kBasicWindowOpen);
}

if (mAppDelegate != nullptr)
Expand Down Expand Up @@ -526,4 +535,51 @@ void CommissioningWindowManager::ExpireFailSafeIfArmed()
}
}

void CommissioningWindowManager::UpdateWindowStatus(CommissioningWindowStatus aNewStatus)
{
CommissioningWindowStatus oldClusterStatus = CommissioningWindowStatusForCluster();
mWindowStatus = aNewStatus;
if (CommissioningWindowStatusForCluster() != oldClusterStatus)
{
// The Administrator Commissioning cluster is always on the root endpoint.
MatterReportingAttributeChangeCallback(kRootEndpointId, AdministratorCommissioning::Id,
AdministratorCommissioning::Attributes::WindowStatus::Id);
}
}

void CommissioningWindowManager::UpdateOpenerVendorId(Nullable<VendorId> aNewOpenerVendorId)
{
// Changing the opener vendor id affects what
// CommissioningWindowStatusForCluster() returns.
CommissioningWindowStatus oldClusterStatus = CommissioningWindowStatusForCluster();

if (mOpenerVendorId != aNewOpenerVendorId)
{
// The Administrator Commissioning cluster is always on the root endpoint.
MatterReportingAttributeChangeCallback(kRootEndpointId, AdministratorCommissioning::Id,
AdministratorCommissioning::Attributes::AdminVendorId::Id);
}

mOpenerVendorId = aNewOpenerVendorId;

if (CommissioningWindowStatusForCluster() != oldClusterStatus)
{
// The Administrator Commissioning cluster is always on the root endpoint.
MatterReportingAttributeChangeCallback(kRootEndpointId, AdministratorCommissioning::Id,
AdministratorCommissioning::Attributes::WindowStatus::Id);
}
}

void CommissioningWindowManager::UpdateOpenerFabricIndex(Nullable<FabricIndex> aNewOpenerFabricIndex)
{
if (mOpenerFabricIndex != aNewOpenerFabricIndex)
{
// The Administrator Commissioning cluster is always on the root endpoint.
MatterReportingAttributeChangeCallback(kRootEndpointId, AdministratorCommissioning::Id,
AdministratorCommissioning::Attributes::AdminFabricIndex::Id);
}

mOpenerFabricIndex = aNewOpenerFabricIndex;
}

} // namespace chip
8 changes: 8 additions & 0 deletions src/app/server/CommissioningWindowManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,14 @@ class CommissioningWindowManager : public SessionEstablishmentDelegate,
*/
void ExpireFailSafeIfArmed();

/**
* Helpers to ensure the right attribute reporting happens when our state is
* updated.
*/
void UpdateWindowStatus(app::Clusters::AdministratorCommissioning::CommissioningWindowStatus aNewStatus);
void UpdateOpenerVendorId(app::DataModel::Nullable<VendorId> aNewOpenerVendorId);
void UpdateOpenerFabricIndex(app::DataModel::Nullable<FabricIndex> aNewOpenerFabricIndex);

AppDelegate * mAppDelegate = nullptr;
Server * mServer = nullptr;

Expand Down
Loading

0 comments on commit 95d337c

Please sign in to comment.