From 106898491e4b1c41e6932bf570b3a0cbb4fba19c Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Sat, 8 Jun 2024 01:31:40 -0400 Subject: [PATCH] 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 * Update src/app/interaction-model/IterationTypes.h Co-authored-by: Boris Zbarsky * Update src/app/interaction-model/Paths.h Co-authored-by: Boris Zbarsky * Comment update --------- Co-authored-by: Andrei Litvin Co-authored-by: Boris Zbarsky --- .../{RequestContext.h => ActionContext.h} | 8 ++-- src/app/interaction-model/BUILD.gn | 4 +- .../{Actions.h => Context.h} | 10 ++-- src/app/interaction-model/Events.h | 46 +++++++++++-------- src/app/interaction-model/InvokeResponder.h | 2 +- src/app/interaction-model/IterationTypes.h | 9 +++- src/app/interaction-model/Model.h | 20 ++++---- src/app/interaction-model/OperationTypes.h | 6 +-- src/app/interaction-model/Paths.h | 16 +++---- .../tests/TestEventEmitting.cpp | 11 ++--- 10 files changed, 72 insertions(+), 60 deletions(-) rename src/app/interaction-model/{RequestContext.h => ActionContext.h} (87%) rename src/app/interaction-model/{Actions.h => Context.h} (74%) diff --git a/src/app/interaction-model/RequestContext.h b/src/app/interaction-model/ActionContext.h similarity index 87% rename from src/app/interaction-model/RequestContext.h rename to src/app/interaction-model/ActionContext.h index 74fa9af9da0fc8..bfc2555870c780 100644 --- a/src/app/interaction-model/RequestContext.h +++ b/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" diff --git a/src/app/interaction-model/BUILD.gn b/src/app/interaction-model/BUILD.gn index c91c2aedac633e..a0967289c6c65c 100644 --- a/src/app/interaction-model/BUILD.gn +++ b/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 = [ diff --git a/src/app/interaction-model/Actions.h b/src/app/interaction-model/Context.h similarity index 74% rename from src/app/interaction-model/Actions.h rename to src/app/interaction-model/Context.h index 62021fbf7ccd32..5f7f442664ff20 100644 --- a/src/app/interaction-model/Actions.h +++ b/src/app/interaction-model/Context.h @@ -16,20 +16,24 @@ */ #pragma once +#include #include #include -#include 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 diff --git a/src/app/interaction-model/Events.h b/src/app/interaction-model/Events.h index 255a55e97ee465..0fc3fc56b974bd 100644 --- a/src/app/interaction-model/Events.h +++ b/src/app/interaction-model/Events.h @@ -24,6 +24,7 @@ #include #include +#include #include namespace chip { @@ -32,10 +33,10 @@ namespace InteractionModel { namespace internal { template -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 ::value, bool> = true> -EventNumber GenerateEvent(E & emittor, const T & aEventData, EndpointId aEndpoint) +template ::value, bool> = true> +std::optional GenerateEvent(G & generator, const T & aEventData, EndpointId aEndpoint) { - internal::SimpleEventLoggingDelegate eventData(aEventData); + internal::SimpleEventPayloadWriter 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,30 +73,30 @@ 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 ::value, bool> = true> -EventNumber GenerateEvent(E & emittor, const T & aEventData, EndpointId endpointId) +template ::value, bool> = true> +std::optional GenerateEvent(G & generator, const T & aEventData, EndpointId endpointId) { - internal::SimpleEventLoggingDelegate eventData(aEventData); + internal::SimpleEventPayloadWriter 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; @@ -103,6 +104,10 @@ EventNumber GenerateEvent(E & emittor, const T & aEventData, EndpointId endpoint } // 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 - EventNumber GenerateEvent(const T & eventData, EndpointId endpointId) + std::optional GenerateEvent(const T & eventData, EndpointId endpointId) { return internal::GenerateEvent(*this, eventData, endpointId); } diff --git a/src/app/interaction-model/InvokeResponder.h b/src/app/interaction-model/InvokeResponder.h index 0a399b24564e96..54ae9ba0f51a08 100644 --- a/src/app/interaction-model/InvokeResponder.h +++ b/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) /// diff --git a/src/app/interaction-model/IterationTypes.h b/src/app/interaction-model/IterationTypes.h index 32ad8bc42b73be..441dd3acb7b81f 100644 --- a/src/app/interaction-model/IterationTypes.h +++ b/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: diff --git a/src/app/interaction-model/Model.h b/src/app/interaction-model/Model.h index b3a127c074aaa1..151065f4c540d2 100644 --- a/src/app/interaction-model/Model.h +++ b/src/app/interaction-model/Model.h @@ -22,7 +22,7 @@ #include #include -#include +#include #include #include #include @@ -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 diff --git a/src/app/interaction-model/OperationTypes.h b/src/app/interaction-model/OperationTypes.h index 115dc11a1ff3bc..57499a8dbafb49 100644 --- a/src/app/interaction-model/OperationTypes.h +++ b/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 diff --git a/src/app/interaction-model/Paths.h b/src/app/interaction-model/Paths.h index 2bf9f0c4158011..9241222a990cb5 100644 --- a/src/app/interaction-model/Paths.h +++ b/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; }; diff --git a/src/app/interaction-model/tests/TestEventEmitting.cpp b/src/app/interaction-model/tests/TestEventEmitting.cpp index cb49dc25caa068..3196636f621653 100644 --- a/src/app/interaction-model/tests/TestEventEmitting.cpp +++ b/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 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 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 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(),