Post-merge updates for IM/DM separation (#33730)

* Some cleanup logic for event generation - naming and return values as eventid is not the same as event number

* Comment fix

* More naming updates

* Several comment updates and renamed RequestContext to ActionContext

* Restyle

* Rename to InteractionModelContext

* one more rename

* Fix typo

* Fix tests to compile

* More renames of actions to context

* One more comment added

* Restyle

* make clang-tidy happy

* Operator== exists on optional ... make use of that directly

* Update src/app/interaction-model/Events.h

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

* Update src/app/interaction-model/IterationTypes.h

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

* Update src/app/interaction-model/Paths.h

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

* Comment update

---------

Co-authored-by: Andrei Litvin <andreilitvin@google.com>
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

Author: 3 people

src/app/interaction-model/RequestContext.h src/app/interaction-model/ActionContext.h

@@ -22,13 +22,13 @@ namespace chip {
 namespace app {
 namespace InteractionModel {
 
-// Context for a currently executing request
-class RequestContext
+// Context for a currently executing action
+class ActionContext
 {
 public:
-    virtual ~RequestContext() = default;
+    virtual ~ActionContext() = default;
 
-    /// Valid ONLY during synchronous handling of a Read/Write/Invoke
+    /// Valid ONLY during synchronous handling of an action.
     ///
     /// Used sparingly, however some operations will require these. An example
     /// usage is "Operational Credentials aborting communications on removed fabrics"

src/app/interaction-model/BUILD.gn

@@ -15,14 +15,14 @@ import("//build_overrides/chip.gni")
 
 source_set("interaction-model") {
   sources = [
-    "Actions.h",
+    "ActionContext.h",
+    "Context.h",
     "Events.h",
     "InvokeResponder.h",
     "IterationTypes.h",
     "Model.h",
     "OperationTypes.h",
     "Paths.h",
-    "RequestContext.h",
   ]
 
   public_deps = [

src/app/interaction-model/Actions.h src/app/interaction-model/Context.h

@@ -16,20 +16,24 @@
  */
 #pragma once
 
+#include <app/interaction-model/ActionContext.h>
 #include <app/interaction-model/Events.h>
 #include <app/interaction-model/Paths.h>
-#include <app/interaction-model/RequestContext.h>
 
 namespace chip {
 namespace app {
 namespace InteractionModel {
 
 /// Data provided to data models in order to interface with the interaction model environment.
-struct InteractionModelActions
+///
+/// Provides callback-style functionality to notify the interaction model of changes
+/// (e.g. using paths to notify of attribute data changes or events to generate events)
+/// as well as fetching current state (via actionContext)
+struct InteractionModelContext
 {
     Events * events;
     Paths * paths;
-    RequestContext * requestContext;
+    ActionContext * actionContext;
 };
 
 } // namespace InteractionModel

src/app/interaction-model/Events.h

@@ -24,6 +24,7 @@
 #include <lib/core/CHIPError.h>
 #include <lib/support/logging/CHIPLogging.h>
 
+#include <optional>
 #include <type_traits>
 
 namespace chip {
@@ -32,10 +33,10 @@ namespace InteractionModel {
 
 namespace internal {
 template <typename T>
-class SimpleEventLoggingDelegate : public EventLoggingDelegate
+class SimpleEventPayloadWriter : public EventLoggingDelegate
 {
 public:
-    SimpleEventLoggingDelegate(const T & aEventData) : mEventData(aEventData){};
+    SimpleEventPayloadWriter(const T & aEventData) : mEventData(aEventData){};
     CHIP_ERROR WriteEvent(chip::TLV::TLVWriter & aWriter) final override
     {
         return DataModel::Encode(aWriter, TLV::ContextTag(EventDataIB::Tag::kData), mEventData);
@@ -45,22 +46,22 @@ class SimpleEventLoggingDelegate : public EventLoggingDelegate
     const T & mEventData;
 };
 
-template <typename E, typename T, std::enable_if_t<DataModel::IsFabricScoped<T>::value, bool> = true>
-EventNumber GenerateEvent(E & emittor, const T & aEventData, EndpointId aEndpoint)
+template <typename G, typename T, std::enable_if_t<DataModel::IsFabricScoped<T>::value, bool> = true>
+std::optional<EventNumber> GenerateEvent(G & generator, const T & aEventData, EndpointId aEndpoint)
 {
-    internal::SimpleEventLoggingDelegate<T> eventData(aEventData);
+    internal::SimpleEventPayloadWriter<T> eventPayloadWriter(aEventData);
     ConcreteEventPath path(aEndpoint, aEventData.GetClusterId(), aEventData.GetEventId());
     EventOptions eventOptions;
     eventOptions.mPath        = path;
     eventOptions.mPriority    = aEventData.GetPriorityLevel();
     eventOptions.mFabricIndex = aEventData.GetFabricIndex();
 
-    // this skips logging the event if it's fabric-scoped but no fabric association exists yet.
-
+    // this skips generating the event if it is fabric-scoped but the provided event data is not
+    // associated with any fabric.
     if (eventOptions.mFabricIndex == kUndefinedFabricIndex)
     {
         ChipLogError(EventLogging, "Event encode failure: no fabric index for fabric scoped event");
-        return kInvalidEventId;
+        return std::nullopt;
     }
 
     //
@@ -72,37 +73,41 @@ EventNumber GenerateEvent(E & emittor, const T & aEventData, EndpointId aEndpoin
     // and used to match against the accessing fabric.
     //
     EventNumber eventNumber;
-    CHIP_ERROR err = emittor.GenerateEvent(&eventData, eventOptions, eventNumber);
+    CHIP_ERROR err = generator.GenerateEvent(&eventPayloadWriter, eventOptions, eventNumber);
     if (err != CHIP_NO_ERROR)
     {
-        ChipLogError(EventLogging, "Failed to log event: %" CHIP_ERROR_FORMAT, err.Format());
-        return kInvalidEventId;
+        ChipLogError(EventLogging, "Failed to generate event: %" CHIP_ERROR_FORMAT, err.Format());
+        return std::nullopt;
     }
 
     return eventNumber;
 }
 
-template <typename E, typename T, std::enable_if_t<!DataModel::IsFabricScoped<T>::value, bool> = true>
-EventNumber GenerateEvent(E & emittor, const T & aEventData, EndpointId endpointId)
+template <typename G, typename T, std::enable_if_t<!DataModel::IsFabricScoped<T>::value, bool> = true>
+std::optional<EventNumber> GenerateEvent(G & generator, const T & aEventData, EndpointId endpointId)
 {
-    internal::SimpleEventLoggingDelegate<T> eventData(aEventData);
+    internal::SimpleEventPayloadWriter<T> eventPayloadWriter(aEventData);
     ConcreteEventPath path(endpointId, aEventData.GetClusterId(), aEventData.GetEventId());
     EventOptions eventOptions;
     eventOptions.mPath     = path;
     eventOptions.mPriority = aEventData.GetPriorityLevel();
     EventNumber eventNumber;
-    CHIP_ERROR err = emittor.GenerateEvent(&eventData, eventOptions, eventNumber);
+    CHIP_ERROR err = generator.GenerateEvent(&eventPayloadWriter, eventOptions, eventNumber);
     if (err != CHIP_NO_ERROR)
     {
-        ChipLogError(EventLogging, "Failed to log event: %" CHIP_ERROR_FORMAT, err.Format());
-        return kInvalidEventId;
+        ChipLogError(EventLogging, "Failed to generate event: %" CHIP_ERROR_FORMAT, err.Format());
+        return std::nullopt;
     }
 
     return eventNumber;
 }
 
 } // namespace internal
 
+/// Exposes event access capabilities.
+///
+/// Allows callers to "generate events" which effectively notifies of an event having
+/// ocurred.
 class Events
 {
 public:
@@ -113,13 +118,14 @@ class Events
     /// Events are generally expected to be sent to subscribed clients and also
     /// be available for read later until they get overwritten by new events
     /// that are being generated.
-    virtual CHIP_ERROR GenerateEvent(EventLoggingDelegate * eventContentWriter, const EventOptions & options,
+    virtual CHIP_ERROR GenerateEvent(EventLoggingDelegate * eventPayloadWriter, const EventOptions & options,
                                      EventNumber & generatedEventNumber) = 0;
 
     // Convenience methods for event logging using cluster-object structures
-    // On error, these log and return kInvalidEventId
+    //
+    // On error, these log and return nullopt.
     template <typename T>
-    EventNumber GenerateEvent(const T & eventData, EndpointId endpointId)
+    std::optional<EventNumber> GenerateEvent(const T & eventData, EndpointId endpointId)
     {
         return internal::GenerateEvent(*this, eventData, endpointId);
     }

src/app/interaction-model/InvokeResponder.h

@@ -26,7 +26,7 @@ namespace InteractionModel {
 
 /// Handles encoding of an invoke response for a specific invoke request.
 ///
-/// This class handles a single response (i.e. a CommandDataIB within the
+/// This class handles a single request (i.e. a CommandDataIB within the
 /// matter protocol) and is responsible for constructing its corresponding
 /// response (i.e. a InvokeResponseIB within the matter protocol)
 ///

src/app/interaction-model/IterationTypes.h

@@ -70,8 +70,13 @@ struct AttributeEntry
 /// Iteration rules:
 ///   - kInvalidEndpointId will be returned when iteration ends (or generally kInvalid* for paths)
 ///   - Any internal iteration errors are just logged (callers do not handle iteration CHIP_ERROR)
-///   - Iteration order is NOT guaranteed, however uniqueness and completeness is (must iterate
-///     over all possible distinct values as long as no internal structural changes occur)
+///   - Iteration order is NOT guaranteed globally. Only the following is guaranteed:
+///     - when iterating over an endpoint, ALL clusters of that endpoint will be iterated first, before
+///       switching the endpoint (order of clusters themselves not guaranteed)
+///     - when iterating over a cluster, ALL attributes of that cluster will be iterated first, before
+///       switching to a new cluster
+///     - uniqueness and completeness (iterate over all possible distinct values as long as no
+///       internal structural changes occur)
 class AttributeTreeIterator
 {
 public:

src/app/interaction-model/Model.h

@@ -22,7 +22,7 @@
 #include <app/AttributeValueDecoder.h>
 #include <app/AttributeValueEncoder.h>
 
-#include <app/interaction-model/Actions.h>
+#include <app/interaction-model/Context.h>
 #include <app/interaction-model/InvokeResponder.h>
 #include <app/interaction-model/IterationTypes.h>
 #include <app/interaction-model/OperationTypes.h>
@@ -43,17 +43,17 @@ class Model : public AttributeTreeIterator
 public:
     virtual ~Model() = default;
 
-    // `actions` pointers  will be guaranteed valid until Shutdown is called()
-    virtual CHIP_ERROR Startup(InteractionModelActions actions)
+    // `context` pointers  will be guaranteed valid until Shutdown is called()
+    virtual CHIP_ERROR Startup(InteractionModelContext context)
     {
-        mActions = actions;
+        mContext = context;
         return CHIP_NO_ERROR;
     }
     virtual CHIP_ERROR Shutdown() = 0;
 
     // During the transition phase, we expect a large subset of code to require access to
     // event emitting, path marking and other operations
-    virtual InteractionModelActions CurrentActions() { return mActions; }
+    virtual InteractionModelContext CurrentContext() const { return mContext; }
 
     /// List reading has specific handling logic:
     ///   `state` contains in/out data about the current list reading. MUST start with kInvalidListIndex on first call
@@ -94,14 +94,14 @@ class Model : public AttributeTreeIterator
     ///         - `NeedsTimedInteraction` for writes that are not timed however are required to be so
     virtual CHIP_ERROR WriteAttribute(const WriteAttributeRequest & request, AttributeValueDecoder & decoder) = 0;
 
-    /// `responder` is used to send back the reply.
+    /// `reply` is used to send back the reply.
     ///    - calling Reply() or ReplyAsync() will let the application control the reply
     ///    - returning a CHIP_NO_ERROR without reply/reply_async implies a Status::Success reply without data
-    ///    - returning a CHIP_*_ERROR implies an error reply (error and data are mutually exclusive)
+    ///    - returning a value other than CHIP_NO_ERROR implies an error reply (error and data are mutually exclusive)
     ///
     /// See InvokeReply/AutoCompleteInvokeResponder for details on how to send back replies and expected
-    /// error handling. If you require knowledge if a response was successfully sent, use the underlying
-    /// `reply` object instead of returning an error codes from Invoke.
+    /// error handling. If you need to know weather a response was successfully sent, use the underlying
+    /// `reply` object instead of returning an error code from Invoke.
     ///
     /// Return codes
     ///   CHIP_IM_GLOBAL_STATUS(code):
@@ -115,7 +115,7 @@ class Model : public AttributeTreeIterator
     virtual CHIP_ERROR Invoke(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments, InvokeReply & reply) = 0;
 
 private:
-    InteractionModelActions mActions = { nullptr };
+    InteractionModelContext mContext = { nullptr };
 };
 
 } // namespace InteractionModel

src/app/interaction-model/OperationTypes.h

@@ -66,8 +66,8 @@ struct ReadState
 
 enum class WriteFlags : uint32_t
 {
-    kTimed     = 0x0001, // Received as a 2nd command after a timed invoke
-    kListBegin = 0x0002, // This is the FIRST list data element in a series of data
+    kTimed     = 0x0001, // Write is a timed write (i.e. a Timed Request Action preceeded it)
+    kListBegin = 0x0002, // This is the FIRST list of data elements
     kListEnd   = 0x0004, // This is the LAST list element to write
 };
 
@@ -79,7 +79,7 @@ struct WriteAttributeRequest : OperationRequest
 
 enum class InvokeFlags : uint32_t
 {
-    kTimed = 0x0001, // Received as a 2nd command after a timed invoke
+    kTimed = 0x0001, // Command received as part of a timed invoke interaction.
 };
 
 struct InvokeRequest : OperationRequest

src/app/interaction-model/Paths.h

@@ -22,23 +22,21 @@ namespace chip {
 namespace app {
 namespace InteractionModel {
 
-/// Handles path attributes for interaction models.
+/// Notification listener for attribute changes.
 ///
-/// It allows a user of the class to mark specific paths
-/// as having changed. The intended use is for some listener to
-/// perform operations as a result of something having changed,
-/// usually by forwarding updates (e.g. in case of subscriptions
-/// that cover that path).
+/// Used to notify that a specific attribute path (or several attributes
+/// via wildcards) have changed their underlying content.
 ///
-/// Methods on this class MUCH be called from within the matter
+/// Methods on this class MUST be called from within the matter
 /// main loop as they will likely trigger interaction model
-/// internal updates and subscription event updates.
+/// internal updates and subscription data reporting.
 class Paths
 {
 public:
     virtual ~Paths() = 0;
 
-    /// Mark some specific attributes dirty.
+    /// Mark all attributes matching the given path (which may be a wildcard) dirty.
+    ///
     /// Wildcards are supported.
     virtual void MarkDirty(const AttributePathParams & path) = 0;
 };

src/app/interaction-model/tests/TestEventEmitting.cpp

@@ -100,7 +100,8 @@ TEST(TestInteractionModelEventEmitting, TestBasicType)
 
     StartUpEventType event{ kFakeSoftwareVersion };
 
-    EventNumber n1 = events->GenerateEvent(event, 0 /* EndpointId */);
+    std::optional<EventNumber> n1 = events->GenerateEvent(event, 0 /* EndpointId */);
+
     ASSERT_EQ(n1, logOnlyEvents.CurrentEventNumber());
     ASSERT_EQ(logOnlyEvents.LastOptions().mPath,
               ConcreteEventPath(0 /* endpointId */, StartUpEventType::GetClusterId(), StartUpEventType::GetEventId()));
@@ -115,9 +116,8 @@ TEST(TestInteractionModelEventEmitting, TestBasicType)
     ASSERT_EQ(err, CHIP_NO_ERROR);
     ASSERT_EQ(decoded_event.softwareVersion, kFakeSoftwareVersion);
 
-    EventNumber n2 = events->GenerateEvent(event, /* endpointId = */ 1);
+    std::optional<EventNumber> n2 = events->GenerateEvent(event, /* endpointId = */ 1);
     ASSERT_EQ(n2, logOnlyEvents.CurrentEventNumber());
-    ASSERT_NE(n1, logOnlyEvents.CurrentEventNumber());
 
     ASSERT_EQ(logOnlyEvents.LastOptions().mPath,
               ConcreteEventPath(1 /* endpointId */, StartUpEventType::GetClusterId(), StartUpEventType::GetEventId()));
@@ -137,14 +137,13 @@ TEST(TestInteractionModelEventEmitting, TestFabricScoped)
     event.adminNodeID     = chip::app::DataModel::MakeNullable(kTestNodeId);
     event.adminPasscodeID = chip::app::DataModel::MakeNullable(kTestPasscode);
 
-    EventNumber n1 = events->GenerateEvent(event, 0 /* EndpointId */);
+    std::optional<EventNumber> n1 = events->GenerateEvent(event, 0 /* EndpointId */);
     // encoding without a fabric ID MUST fail for fabric events
-    ASSERT_EQ(n1, kInvalidEventId);
+    ASSERT_FALSE(n1.has_value());
 
     event.fabricIndex = kTestFabricIndex;
     n1                = events->GenerateEvent(event, /* endpointId = */ 0);
 
-    ASSERT_NE(n1, kInvalidEventId);
     ASSERT_EQ(n1, logOnlyEvents.CurrentEventNumber());
     ASSERT_EQ(logOnlyEvents.LastOptions().mPath,
               ConcreteEventPath(0 /* endpointId */, AccessControlEntryChangedType::GetClusterId(),