Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use ProviderChangeListener directly to report attrbiute change #36756

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+84 −42
Open

Use ProviderChangeListener directly to report attrbiute change #36756

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion src/app/codegen-data-model-provider/CodegenDataModelProvider.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,17 @@ class EnumeratorCommandFinder
}
};

class DefaultProviderChangeListener : public DataModel::ProviderChangeListener
Copy link
Contributor

@andy31415 andy31415 Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets place this in its own header. I also believe that detail::app::detail::DefaultProviderChangeListener does not make it obvious on what it is. This is not any default but rather maybe EmberDirtyMarker or EmberProviderChangeListener or CodegenProviderChangeListener

Thinking of it maybe eventually as we stabilize the API we can figure out a better name than ProviderChangeListener as that name makes me think of "a listener that gets notified when a provider changes" which is not at all what it is. Do you have any suggestions?

Something like AttributeDirtyMarker or DataChangeNotifier or something like that. Obviously not for this PR, but this PR made it obvious to me that the naming is not great.

{
public:
explicit DefaultProviderChangeListener(DataModel::Provider & provider) : mProvider(provider) {}

void MarkDirty(const AttributePathParams & path) override;

private:
DataModel::Provider & mProvider;
};

} // namespace detail

/// An implementation of `InteractionModel::Model` that relies on code-generation
Expand Down Expand Up @@ -126,6 +137,8 @@ class CodegenDataModelProvider : public DataModel::Provider
};

public:
CodegenDataModelProvider() : mChangeListener(*this) {}

/// clears out internal caching. Especially useful in unit tests,
/// where path caching does not really apply (the same path may result in different outcomes)
void Reset()
Expand Down Expand Up @@ -185,7 +198,7 @@ class CodegenDataModelProvider : public DataModel::Provider
ConcreteCommandPath FirstGeneratedCommand(const ConcreteClusterPath & cluster) override;
ConcreteCommandPath NextGeneratedCommand(const ConcreteCommandPath & before) override;

void Temporary_ReportAttributeChanged(const AttributePathParams & path) override;
DataModel::ProviderChangeListener & GetAttributeChangeReporter() override { return mChangeListener; }

private:
// Iteration is often done in a tight loop going through all values.
Expand Down Expand Up @@ -221,6 +234,8 @@ class CodegenDataModelProvider : public DataModel::Provider
// Ember requires a persistence provider, so we make sure we can always have something
PersistentStorageDelegate * mPersistentStorageDelegate = nullptr;

detail::DefaultProviderChangeListener mChangeListener;

/// Finds the specified ember cluster
///
/// Effectively the same as `emberAfFindServerCluster` except with some caching capabilities
Expand Down
36 changes: 20 additions & 16 deletions src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,26 @@ std::optional<CHIP_ERROR> TryWriteViaAccessInterface(const ConcreteDataAttribute

} // namespace

namespace detail {

void DefaultProviderChangeListener::MarkDirty(const AttributePathParams & path)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add unit tests for this.

{
ContextAttributesChangeListener change_listener(mProvider.CurrentContext());
if (path.mClusterId != kInvalidClusterId)
{
emberAfAttributeChanged(path.mEndpointId, path.mClusterId, path.mAttributeId, &change_listener);
}
else
{
// When the path has wildcard cluster Id, call the emberAfEndpointChanged to mark attributes on the given endpoint
// as having changing, but do NOT increase/alter any cluster data versions, as this happens when a bridged endpoint is
// added or removed from a bridge and the cluster data is not changed during the process.
emberAfEndpointChanged(path.mEndpointId, &change_listener);
}
}

} // namespace detail

DataModel::ActionReturnStatus CodegenDataModelProvider::WriteAttribute(const DataModel::WriteAttributeRequest & request,
AttributeValueDecoder & decoder)
{
Expand Down Expand Up @@ -261,21 +281,5 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::WriteAttribute(const Dat
return CHIP_NO_ERROR;
}

void CodegenDataModelProvider::Temporary_ReportAttributeChanged(const AttributePathParams & path)
{
ContextAttributesChangeListener change_listener(CurrentContext());
if (path.mClusterId != kInvalidClusterId)
{
emberAfAttributeChanged(path.mEndpointId, path.mClusterId, path.mAttributeId, &change_listener);
}
else
{
// When the path has wildcard cluster Id, call the emberAfEndpointChanged to mark attributes on the given endpoint
// as having changing, but do NOT increase/alter any cluster data versions, as this happens when a bridged endpoint is
// added or removed from a bridge and the cluster data is not changed during the process.
emberAfEndpointChanged(path.mEndpointId, &change_listener);
}
}

} // namespace app
} // namespace chip
18 changes: 0 additions & 18 deletions src/app/data-model-provider/MetadataTypes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,24 +221,6 @@ class ProviderMetadataTree
// returned as responses.
virtual ConcreteCommandPath FirstGeneratedCommand(const ConcreteClusterPath & cluster) = 0;
virtual ConcreteCommandPath NextGeneratedCommand(const ConcreteCommandPath & before) = 0;

/// Workaround function to report attribute change.
///
/// When this is invoked, the caller is expected to increment the cluster data version, and the attribute path
/// should be marked as `dirty` by the data model provider listener so that the reporter can notify the subscriber
/// of attribute changes.
/// This function should be invoked when attribute managed by attribute access interface is modified but not
/// through an actual Write interaction.
/// For example, if the LastNetworkingStatus attribute changes because the NetworkCommissioning driver detects a
/// network connection status change and calls SetLastNetworkingStatusValue(). The data model provider can recognize
/// this change by invoking this function at the point of change.
///
/// This is a workaround function as we cannot notify the attribute change to the data model provider. The provider
/// should own its data and versions.
///
/// TODO: We should remove this function when the AttributeAccessInterface/CommandHandlerInterface is able to report
/// the attribute changes.
virtual void Temporary_ReportAttributeChanged(const AttributePathParams & path) = 0;
};

} // namespace DataModel
Expand Down
9 changes: 9 additions & 0 deletions src/app/data-model-provider/Provider.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,15 @@ class Provider : public ProviderMetadataTree
virtual std::optional<ActionReturnStatus> Invoke(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments,
CommandHandler * handler) = 0;

/// Returns a reference to the ProviderChangeListener used for reporting attribute changes.
///
/// This is used to notify listeners about changes in attributes managed by the provider.
/// Listeners can be used to react to updates in the underlying data model.
///
/// It is expected that implementations provide a valid reference, ensuring that
/// attribute change reporting is functional throughout the lifecycle of the Provider.
virtual ProviderChangeListener & GetAttributeChangeReporter() = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we hit naming again. A GetAttributeChangeReporter should return a class AttributeChangeReporter or more closely related name. Maybe we can figure out something better.

I would add to the comments that direct hookups to this should eventually be rare and to prefer to get passed in such change listeners in constructors for easier testing/mocking.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't understand what the actual long-term plan here is. Specifically:

  1. Who owns the data that might actually change? Presumably they are responsible for changing it as needed.
  2. Who owns the DataVersion for a cluster instance? Presumably the entity from item 1 is either this entity, or if not is responsible for notifying it, right?
  3. Who is responsible for notifying the reporting engine that things are dirty?

ProviderChangeListener seems to be designed to be a per-InteractionModelContext singleton right now, but this is not how it's being used here...

Also, how is ProviderChangeListener different from chip::app::AttributesChangedListener and if it's not why do we have both?

It feels like we are adding more and more complexity here without a clear plan for what the final (simple!) goal should be.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can define some design doc ... maybe we should start in releases. In my mind:

  • DataModel::Provider owns everything and it notifies outside about "something dirty". This includes DataModel::Provider owns the clusterVersion

  • Existing AttributeAccessInterfaces own the attributes. So they can change them and they may choose to mark the cluster dirty or not (quiet reporting attributes would not be marked)

I am looking for a binding between AttributeAccessInterface (data owner) and DataModel::Provider clusterVersion (cluster/metadata owner), hence my belief that AAI should get some sort of Listener that it can notify when something changed. I would need:

  • AAI and CommandHandlerInterfaces need abiltiy to say "things changed", which should go to Provider (to update cluster version) and then go to reporting engine (to trigger reports)
  • Interface should be available to the App as well to trigger things on external changes (e.g. I manually pres a button, or sensor detects temperature change or something). However I would prefer if application interacts with clusters for this (so app tells cluster "i changed data" or "change this value for me" and cluster in turns notifies the provider who notifes the InteractionModelEngine.

Thoughts? Should we create some dock/hackmd or chat separaely to actually define what is sane?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't tell whether the above bullets are mutually exclusive or "we should do both these things" (in the first set of two bullets). Doing a sync discussion about this sounds like it might be good, just to make sure we are not talking past each other....

But my first question is this: why should the DataVersion be owned by the provider, not the cluster implementation? Is that so we can use the same cluster implementation with different providers?

And if the idea is that the provider owns the DataVersion, why do we have a separate interface defined for telling it "data version should update" instead of just having that be a method on the provider?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably warrants some design doc/discussion. Will see about creating one and discussing separately. In my mind:

  • Cluster owning the version makes the most sense, however I did not see an obvious path given the current ember coupling. There is no "ask cluster implementation about data version" but rather a global chip::DataVersion *emberAfDataVersionStorage() and a global emberAfAttributeChanged callback that increments that storage. Because it was global, the closest I found was the provider (which is often global, at least to start) would own this (i.e. codegen provider owns all emberAf* calls)

  • Separate interface seems to make sense to be able to inject to some clusters implementation the ability to notify the world their internal state changes. This can be DataModel::Provider however that seems to be a much larger interface. Thinking of mocking or generally dependency management, I prefer to pass in smaller dependencies as most clusters only need version change support.


private:
InteractionModelContext mContext = { nullptr };
};
Expand Down
14 changes: 9 additions & 5 deletions src/app/reporting/reporting.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,9 @@ void MatterReportingAttributeChangeCallback(EndpointId endpoint, ClusterId clust
// applications notifying about changes from their end.
assertChipStackLockedByCurrentThread();

InteractionModelEngine::GetInstance()->GetDataModelProvider()->Temporary_ReportAttributeChanged(
AttributePathParams(endpoint, clusterId, attributeId));
DataModel::ProviderChangeListener & changeListener =
InteractionModelEngine::GetInstance()->GetDataModelProvider()->GetAttributeChangeReporter();
changeListener.MarkDirty(AttributePathParams(endpoint, clusterId, attributeId));
}

void MatterReportingAttributeChangeCallback(const ConcreteAttributePath & aPath)
Expand All @@ -40,8 +41,9 @@ void MatterReportingAttributeChangeCallback(const ConcreteAttributePath & aPath)
// applications notifying about changes from their end.
assertChipStackLockedByCurrentThread();

InteractionModelEngine::GetInstance()->GetDataModelProvider()->Temporary_ReportAttributeChanged(
AttributePathParams(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId));
DataModel::ProviderChangeListener & changeListener =
InteractionModelEngine::GetInstance()->GetDataModelProvider()->GetAttributeChangeReporter();
changeListener.MarkDirty(AttributePathParams(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId));
}

void MatterReportingAttributeChangeCallback(EndpointId endpoint)
Expand All @@ -50,5 +52,7 @@ void MatterReportingAttributeChangeCallback(EndpointId endpoint)
// applications notifying about changes from their end.
assertChipStackLockedByCurrentThread();

InteractionModelEngine::GetInstance()->GetDataModelProvider()->Temporary_ReportAttributeChanged(AttributePathParams(endpoint));
DataModel::ProviderChangeListener & changeListener =
InteractionModelEngine::GetInstance()->GetDataModelProvider()->GetAttributeChangeReporter();
changeListener.MarkDirty(AttributePathParams(endpoint));
}
14 changes: 14 additions & 0 deletions src/app/tests/test-interaction-model-api.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,14 @@ class TestOnlyAttributeValueDecoderAccessor
AttributeValueDecoder & mDecoder;
};

class TestProviderChangeListener : public DataModel::ProviderChangeListener
{
public:
explicit TestProviderChangeListener() {}

void MarkDirty(const AttributePathParams & path) override {}
};

// strong defintion in TestCommandInteraction.cpp
__attribute__((weak)) void DispatchSingleClusterCommand(const ConcreteCommandPath & aRequestCommandPath,
chip::TLV::TLVReader & aReader, CommandHandler * apCommandObj)
Expand Down Expand Up @@ -149,6 +157,12 @@ std::optional<ActionReturnStatus> TestImCustomDataModel::Invoke(const InvokeRequ
return std::make_optional<ActionReturnStatus>(CHIP_ERROR_NOT_IMPLEMENTED);
}

ProviderChangeListener & TestImCustomDataModel::GetAttributeChangeReporter()
{
static TestProviderChangeListener changeListener;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why static? Since this is a test, could this thing be a property of the object itself? We could make it accumulate a std::vector of attributepathparams passed in, so we can validate their values.

return changeListener;
}

DataModel::EndpointEntry TestImCustomDataModel::FirstEndpoint()
{
return CodegenDataModelProviderInstance(nullptr /* delegate */)->FirstEndpoint();
Expand Down
2 changes: 1 addition & 1 deletion src/app/tests/test-interaction-model-api.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ class TestImCustomDataModel : public DataModel::Provider
AttributeValueDecoder & decoder) override;
std::optional<DataModel::ActionReturnStatus> Invoke(const DataModel::InvokeRequest & request,
chip::TLV::TLVReader & input_arguments, CommandHandler * handler) override;
DataModel::ProviderChangeListener & GetAttributeChangeReporter() override;

DataModel::EndpointEntry FirstEndpoint() override;
DataModel::EndpointEntry NextEndpoint(EndpointId before) override;
Expand All @@ -131,7 +132,6 @@ class TestImCustomDataModel : public DataModel::Provider
std::optional<DataModel::CommandInfo> GetAcceptedCommandInfo(const ConcreteCommandPath & path) override;
ConcreteCommandPath FirstGeneratedCommand(const ConcreteClusterPath & cluster) override;
ConcreteCommandPath NextGeneratedCommand(const ConcreteCommandPath & before) override;
void Temporary_ReportAttributeChanged(const AttributePathParams & path) override {}
};

} // namespace app
Expand Down
14 changes: 14 additions & 0 deletions src/controller/tests/data_model/DataModelFixtures.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,14 @@ class TestOnlyAttributeValueDecoderAccessor
AttributeValueDecoder & mDecoder;
};

class TestProviderChangeListener : public DataModel::ProviderChangeListener
{
public:
explicit TestProviderChangeListener() {}

void MarkDirty(const AttributePathParams & path) override {}
};

namespace DataModelTests {

ScopedChangeOnly<ReadResponseDirective> gReadResponseDirective(ReadResponseDirective::kSendDataResponse);
Expand Down Expand Up @@ -474,6 +482,12 @@ std::optional<ActionReturnStatus> CustomDataModel::Invoke(const InvokeRequest &
return std::nullopt; // handler status is set by the dispatch
}

ProviderChangeListener & CustomDataModel::GetAttributeChangeReporter()
{
static TestProviderChangeListener changeListener;
return changeListener;
}

DataModel::EndpointEntry CustomDataModel::FirstEndpoint()
{
return CodegenDataModelProviderInstance(nullptr /* delegate */)->FirstEndpoint();
Expand Down
2 changes: 1 addition & 1 deletion src/controller/tests/data_model/DataModelFixtures.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ class CustomDataModel : public DataModel::Provider
AttributeValueDecoder & decoder) override;
std::optional<DataModel::ActionReturnStatus> Invoke(const DataModel::InvokeRequest & request,
chip::TLV::TLVReader & input_arguments, CommandHandler * handler) override;
DataModel::ProviderChangeListener & GetAttributeChangeReporter() override;

DataModel::EndpointEntry FirstEndpoint() override;
DataModel::EndpointEntry NextEndpoint(EndpointId before) override;
Expand All @@ -143,7 +144,6 @@ class CustomDataModel : public DataModel::Provider
std::optional<DataModel::CommandInfo> GetAcceptedCommandInfo(const ConcreteCommandPath & path) override;
ConcreteCommandPath FirstGeneratedCommand(const ConcreteClusterPath & cluster) override;
ConcreteCommandPath NextGeneratedCommand(const ConcreteCommandPath & before) override;
void Temporary_ReportAttributeChanged(const AttributePathParams & path) override {}
};

} // namespace DataModelTests
Expand Down
Loading