Use ProviderChangeListener directly to report attrbiute change #36756
Conversation
|
Review changes with |
yufengwangca
force-pushed
the
pr/ember/report
branch
from
4a66e63 to
f65773e
Compare
|
PR #36756: Size comparison from 84fb78f to f65773e Increases above 0.2%:
Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
| @@ -80,6 +80,17 @@ class EnumeratorCommandFinder | |||
| } | |||
| }; | |||
|
|
|||
| class DefaultProviderChangeListener : public DataModel::ProviderChangeListener | |||
There was a problem hiding this comment.
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.
| /// | ||
| /// 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; |
There was a problem hiding this comment.
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.
| @@ -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; | |||
There was a problem hiding this comment.
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.
| @@ -88,6 +88,26 @@ std::optional<CHIP_ERROR> TryWriteViaAccessInterface(const ConcreteDataAttribute | |||
|
|
|||
| } // namespace | |||
|
|
|||
| namespace detail { | |||
|
|
|||
| void DefaultProviderChangeListener::MarkDirty(const AttributePathParams & path) | |||
There was a problem hiding this comment.
Please add unit tests for this.
| /// | ||
| /// 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; |
There was a problem hiding this comment.
I still don't understand what the actual long-term plan here is. Specifically:
- Who owns the data that might actually change? Presumably they are responsible for changing it as needed.
- 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?
- 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.
There was a problem hiding this comment.
We can define some design doc ... maybe we should start in releases. In my mind:
-
DataModel::Providerowns everything and it notifies outside about "something dirty". This includesDataModel::Providerowns 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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 globalemberAfAttributeChangedcallback 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::Providerhowever 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.
Currently, MarkDirty in ProviderChangeListener does exactly what Temporary_ReportAttributeChanged does.
Instead of calling Temporary_ReportAttributeChanged or interacting with Provider, consumers would now use:
Consumers call Provider::GetAttributeChangeReporter() to obtain a ProviderChangeListener reference. For testing, they can mock just the ProviderChangeListener.
This approach simplifies unit testing and mocking. By focusing on the ProviderChangeListener, which has a single method, we avoid the complexity of mocking the entire DataModel::Provider interface with its approximately 30 methods.
Unit Test:
A mock ProviderChangeListener can now be used for simplified testing.
Fix: #36661