Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit 326689d

Browse files
andy31415andreilitvinbzbarsky-applerestyled-commitstcarmelveilleux
authored andcommittedDec 11, 2024
Make use of DataModel::Provider in writes (project-chip#34754)
* Implement DM::Provider::Write usage * Fix compile * Fix java builds * Update src/app/InteractionModelEngine.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Make codegen data model call increasing the cluster data version * Restyle * Make sure that attribute changed information can be propagated out of ember * Optimize storage size of WriteHandler * Restyle * Make the code more obvious identical with what was there before * Change Provider to not double-call the dirty and version increase * Update src/app/InteractionModelEngine.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Cleaner usage: no need of a separate function that is used in one place only * Attempt an API update * Fix typos in the Accessors src * Fix typo and regen * More fixes on accessors * Update signature for emAfWriteAttributeExternal * Add a comment about all the checks being vague * Update src/app/util/af-types.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/util/af-types.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/util/attribute-storage.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/zap-templates/templates/app/attributes/Accessors-src.zapt Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/util/mock/CodegenEmberMocks.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/zap-templates/templates/app/attributes/Accessors-src.zapt Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/zap-templates/templates/app/attributes/Accessors-src.zapt Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * zap regen and restyle * Update src/app/zap-templates/templates/app/attributes/Accessors-src.zapt Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/util/attribute-table.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/util/attribute-storage.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/util/attribute-storage.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/util/attribute-table.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/util/attribute-table.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/util/attribute-storage.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/util/attribute-table.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Rename ChangedPathListener to AttributesChangedListener * Remove chip:: and chip::app * Update constructors of AttributePathParams and add nodiscard according to the linter to never call const methods without considering their return value * Restyled by clang-format * Remove auto-inserted include * Update again and zap regen: removed extra namespace prefixes in accessors.h/cpp * Add comment about uint8_t non-const usage... * Another rename given that the listener is now an attributes and not a path listener * Update src/app/util/attribute-table.h Co-authored-by: Tennessee Carmel-Veilleux <tennessee.carmelveilleux@gmail.com> * Update after merge to have more things compile * Everything compiles now * Restyle * Make unit tests pass: mocks also have to call the change listeners * Comment update to talk more about AttributesChangedListener * Restyle * Add support for a previous path write ... this is similar to what ACL caching and tokenizing currently does, but in a explicit manner describing the ACL use case * Remove some extra added includes * Another include fix * Follow the comment and do not restrict the cache to ACL cluster, since this is what current ember does * More self code review changes * Make tests pass, more code cleanup * Match ordering to ember-compatibility functions. This is ODD because we check attribute existance before access! * Adjust test ordering and add a large note that we are probably doing the wrong thing, however tests force us to do the wrong thing * Fix unit test * Remove odd comment * Add a few ending endifs * Renamed ScopedExchangeContext * Update src/app/InteractionModelEngine.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/WriteHandler.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/WriteHandler.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/data-model-provider/OperationTypes.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/data-model-provider/OperationTypes.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/data-model-provider/OperationTypes.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Restyle to reorder includes * Add issue link * Update equality logic * Rename member to mLastSuccessfullyWrittenPath * Update argument logic * Make ActionContext private in IME so it is not such a public API * Clean up comments * fix up compares * Restyle --------- Co-authored-by: Andrei Litvin <andreilitvin@google.com> Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> Co-authored-by: Restyled.io <commits@restyled.io> Co-authored-by: Tennessee Carmel-Veilleux <tennessee.carmelveilleux@gmail.com>
1 parent a803c25 commit 326689d

20 files changed

+513
-77
lines changed
 

‎src/app/AttributeValueDecoder.h

+2
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,8 @@ class AttributeValueDecoder
6565
const Access::SubjectDescriptor & GetSubjectDescriptor() const { return mSubjectDescriptor; }
6666

6767
private:
68+
friend class TestOnlyAttributeValueDecoderAccessor;
69+
6870
TLV::TLVReader & mReader;
6971
bool mTriedDecode = false;
7072
const Access::SubjectDescriptor mSubjectDescriptor;

‎src/app/EventManagement.cpp

+7
Original file line numberDiff line numberDiff line change
@@ -867,6 +867,12 @@ void EventManagement::SetScheduledEventInfo(EventNumber & aEventNumber, uint32_t
867867
aInitialWrittenEventBytes = mBytesWritten;
868868
}
869869

870+
CHIP_ERROR EventManagement::GenerateEvent(EventLoggingDelegate * eventPayloadWriter, const EventOptions & options,
871+
EventNumber & generatedEventNumber)
872+
{
873+
return LogEvent(eventPayloadWriter, options, generatedEventNumber);
874+
}
875+
870876
void CircularEventBuffer::Init(uint8_t * apBuffer, uint32_t aBufferLength, CircularEventBuffer * apPrev,
871877
CircularEventBuffer * apNext, PriorityLevel aPriorityLevel)
872878
{
@@ -926,5 +932,6 @@ CHIP_ERROR CircularEventBufferWrapper::GetNextBuffer(TLVReader & aReader, const
926932
exit:
927933
return err;
928934
}
935+
929936
} // namespace app
930937
} // namespace chip

‎src/app/EventManagement.h

+7-1
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
#include <app/EventScheduler.h>
3333
#include <app/MessageDef/EventDataIB.h>
3434
#include <app/MessageDef/StatusIB.h>
35+
#include <app/data-model-provider/EventsGenerator.h>
3536
#include <app/util/basic-types.h>
3637
#include <lib/core/TLVCircularBuffer.h>
3738
#include <lib/support/CHIPCounter.h>
@@ -197,7 +198,7 @@ struct LogStorageResources
197198
* more space for new events.
198199
*/
199200

200-
class EventManagement
201+
class EventManagement : public DataModel::EventsGenerator
201202
{
202203
public:
203204
/**
@@ -391,6 +392,10 @@ class EventManagement
391392
*/
392393
void SetScheduledEventInfo(EventNumber & aEventNumber, uint32_t & aInitialWrittenEventBytes) const;
393394

395+
/* EventsGenerator implementation */
396+
CHIP_ERROR GenerateEvent(EventLoggingDelegate * eventPayloadWriter, const EventOptions & options,
397+
EventNumber & generatedEventNumber) override;
398+
394399
private:
395400
/**
396401
* @brief
@@ -565,5 +570,6 @@ class EventManagement
565570

566571
EventScheduler * mpEventScheduler = nullptr;
567572
};
573+
568574
} // namespace app
569575
} // namespace chip

‎src/app/InteractionModelEngine.cpp

+37-6
Original file line numberDiff line numberDiff line change
@@ -896,7 +896,7 @@ Protocols::InteractionModel::Status InteractionModelEngine::OnWriteRequest(Messa
896896
{
897897
if (writeHandler.IsFree())
898898
{
899-
VerifyOrReturnError(writeHandler.Init(this) == CHIP_NO_ERROR, Status::Busy);
899+
VerifyOrReturnError(writeHandler.Init(GetDataModelProvider(), this) == CHIP_NO_ERROR, Status::Busy);
900900
return writeHandler.OnWriteRequest(apExchangeContext, std::move(aPayload), aIsTimedWrite);
901901
}
902902
}
@@ -997,6 +997,9 @@ CHIP_ERROR InteractionModelEngine::OnMessageReceived(Messaging::ExchangeContext
997997

998998
Protocols::InteractionModel::Status status = Status::Failure;
999999

1000+
// Ensure that DataModel::Provider has access to the exchange the message was received on.
1001+
CurrentExchangeValueScope scopedExchangeContext(*this, apExchangeContext);
1002+
10001003
// Group Message can only be an InvokeCommandRequest or WriteRequest
10011004
if (apExchangeContext->IsGroupExchangeContext() &&
10021005
!aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::InvokeCommandRequest) &&
@@ -1750,16 +1753,44 @@ DataModel::Provider * InteractionModelEngine::SetDataModelProvider(DataModel::Pr
17501753
// Alternting data model should not be done while IM is actively handling requests.
17511754
VerifyOrDie(mReadHandlers.begin() == mReadHandlers.end());
17521755

1753-
DataModel::Provider * oldModel = GetDataModelProvider();
1754-
mDataModelProvider = model;
1756+
DataModel::Provider * oldModel = mDataModelProvider;
1757+
if (oldModel != nullptr)
1758+
{
1759+
CHIP_ERROR err = oldModel->Shutdown();
1760+
if (err != CHIP_NO_ERROR)
1761+
{
1762+
ChipLogError(InteractionModel, "Failure on interaction model shutdown: %" CHIP_ERROR_FORMAT, err.Format());
1763+
}
1764+
}
1765+
1766+
mDataModelProvider = model;
1767+
if (mDataModelProvider != nullptr)
1768+
{
1769+
DataModel::InteractionModelContext context;
1770+
1771+
context.eventsGenerator = &EventManagement::GetInstance();
1772+
context.dataModelChangeListener = &mReportingEngine;
1773+
context.actionContext = this;
1774+
1775+
CHIP_ERROR err = mDataModelProvider->Startup(context);
1776+
if (err != CHIP_NO_ERROR)
1777+
{
1778+
ChipLogError(InteractionModel, "Failure on interaction model startup: %" CHIP_ERROR_FORMAT, err.Format());
1779+
}
1780+
}
1781+
17551782
return oldModel;
17561783
}
17571784

1758-
DataModel::Provider * InteractionModelEngine::GetDataModelProvider() const
1785+
DataModel::Provider * InteractionModelEngine::GetDataModelProvider()
17591786
{
17601787
#if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE
1761-
// TODO: this should be temporary, we should fully inject the data model
1762-
VerifyOrReturnValue(mDataModelProvider != nullptr, CodegenDataModelProviderInstance());
1788+
if (mDataModelProvider == nullptr)
1789+
{
1790+
// These should be called within the CHIP processing loop.
1791+
assertChipStackLockedByCurrentThread();
1792+
SetDataModelProvider(CodegenDataModelProviderInstance());
1793+
}
17631794
#endif
17641795
return mDataModelProvider;
17651796
}

‎src/app/InteractionModelEngine.h

+25-2
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@ namespace app {
8686
*/
8787
class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
8888
public Messaging::ExchangeDelegate,
89+
private DataModel::ActionContext,
8990
public CommandResponseSender::Callback,
9091
public CommandHandlerImpl::Callback,
9192
public ReadHandler::ManagementCallback,
@@ -404,7 +405,10 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
404405
}
405406
#endif
406407

407-
DataModel::Provider * GetDataModelProvider() const;
408+
// Temporarily NOT const because the data model provider will be auto-set
409+
// to codegen on first usage. This behaviour will be changed once each
410+
// application must explicitly set the data model provider.
411+
DataModel::Provider * GetDataModelProvider();
408412

409413
// MUST NOT be used while the interaction model engine is running as interaction
410414
// model functionality (e.g. active reads/writes/subscriptions) rely on data model
@@ -414,6 +418,9 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
414418
DataModel::Provider * SetDataModelProvider(DataModel::Provider * model);
415419

416420
private:
421+
/* DataModel::ActionContext implementation */
422+
Messaging::ExchangeContext * CurrentExchange() override { return mCurrentExchange; }
423+
417424
friend class reporting::Engine;
418425
friend class TestCommandInteraction;
419426
friend class TestInteractionModelEngine;
@@ -700,7 +707,23 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
700707

701708
SubscriptionResumptionStorage * mpSubscriptionResumptionStorage = nullptr;
702709

703-
DataModel::Provider * mDataModelProvider = nullptr;
710+
DataModel::Provider * mDataModelProvider = nullptr;
711+
Messaging::ExchangeContext * mCurrentExchange = nullptr;
712+
713+
// Changes the current exchange context of a InteractionModelEngine to a given context
714+
class CurrentExchangeValueScope
715+
{
716+
public:
717+
CurrentExchangeValueScope(InteractionModelEngine & engine, Messaging::ExchangeContext * context) : mEngine(engine)
718+
{
719+
mEngine.mCurrentExchange = context;
720+
}
721+
722+
~CurrentExchangeValueScope() { mEngine.mCurrentExchange = nullptr; }
723+
724+
private:
725+
InteractionModelEngine & mEngine;
726+
};
704727
};
705728

706729
} // namespace app

‎src/app/WriteHandler.cpp

+48-4
Original file line numberDiff line numberDiff line change
@@ -18,30 +18,40 @@
1818

1919
#include <app/AppConfig.h>
2020
#include <app/AttributeAccessInterfaceRegistry.h>
21+
#include <app/AttributeValueDecoder.h>
2122
#include <app/InteractionModelEngine.h>
2223
#include <app/MessageDef/EventPathIB.h>
2324
#include <app/MessageDef/StatusIB.h>
2425
#include <app/StatusResponse.h>
2526
#include <app/WriteHandler.h>
27+
#include <app/data-model-provider/OperationTypes.h>
2628
#include <app/reporting/Engine.h>
2729
#include <app/util/MatterCallbacks.h>
2830
#include <app/util/ember-compatibility-functions.h>
2931
#include <credentials/GroupDataProvider.h>
32+
#include <lib/core/CHIPError.h>
3033
#include <lib/support/CodeUtils.h>
3134
#include <lib/support/TypeTraits.h>
35+
#include <messaging/ExchangeContext.h>
3236
#include <protocols/interaction_model/StatusCode.h>
3337

38+
#include <optional>
39+
3440
namespace chip {
3541
namespace app {
3642

3743
using namespace Protocols::InteractionModel;
3844
using Status = Protocols::InteractionModel::Status;
3945
constexpr uint8_t kListAttributeType = 0x48;
4046

41-
CHIP_ERROR WriteHandler::Init(WriteHandlerDelegate * apWriteHandlerDelegate)
47+
CHIP_ERROR WriteHandler::Init(DataModel::Provider * apProvider, WriteHandlerDelegate * apWriteHandlerDelegate)
4248
{
4349
VerifyOrReturnError(!mExchangeCtx, CHIP_ERROR_INCORRECT_STATE);
4450
VerifyOrReturnError(apWriteHandlerDelegate, CHIP_ERROR_INVALID_ARGUMENT);
51+
#if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE
52+
VerifyOrReturnError(apProvider, CHIP_ERROR_INVALID_ARGUMENT);
53+
mDataModelProvider = apProvider;
54+
#endif // CHIP_CONFIG_USE_DATA_MODEL_INTERFACE
4555

4656
mDelegate = apWriteHandlerDelegate;
4757
MoveToState(State::Initialized);
@@ -63,6 +73,9 @@ void WriteHandler::Close()
6373
DeliverFinalListWriteEnd(false /* wasSuccessful */);
6474
mExchangeCtx.Release();
6575
mStateFlags.Clear(StateBits::kSuppressResponse);
76+
#if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE
77+
mDataModelProvider = nullptr;
78+
#endif // CHIP_CONFIG_USE_DATA_MODEL_INTERFACE
6679
MoveToState(State::Uninitialized);
6780
}
6881

@@ -354,7 +367,7 @@ CHIP_ERROR WriteHandler::ProcessAttributeDataIBs(TLV::TLVReader & aAttributeData
354367
err = CHIP_NO_ERROR;
355368
}
356369
SuccessOrExit(err);
357-
err = WriteSingleClusterData(subjectDescriptor, dataAttributePath, dataReader, this);
370+
err = WriteClusterData(subjectDescriptor, dataAttributePath, dataReader);
358371
if (err != CHIP_NO_ERROR)
359372
{
360373
mWriteResponseBuilder.GetWriteResponses().Rollback(backup);
@@ -501,7 +514,7 @@ CHIP_ERROR WriteHandler::ProcessGroupAttributeDataIBs(TLV::TLVReader & aAttribut
501514

502515
DataModelCallbacks::GetInstance()->AttributeOperation(DataModelCallbacks::OperationType::Write,
503516
DataModelCallbacks::OperationOrder::Pre, dataAttributePath);
504-
err = WriteSingleClusterData(subjectDescriptor, dataAttributePath, tmpDataReader, this);
517+
err = WriteClusterData(subjectDescriptor, dataAttributePath, tmpDataReader);
505518
if (err != CHIP_NO_ERROR)
506519
{
507520
ChipLogError(DataManagement,
@@ -552,14 +565,18 @@ Status WriteHandler::ProcessWriteRequest(System::PacketBufferHandle && aPayload,
552565
// our callees hand out Status as well.
553566
Status status = Status::InvalidAction;
554567

568+
#if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE
569+
mLastSuccessfullyWrittenPath = std::nullopt;
570+
#endif // CHIP_CONFIG_USE_DATA_MODEL_INTERFACE
571+
555572
reader.Init(std::move(aPayload));
556573

557574
err = writeRequestParser.Init(reader);
558575
SuccessOrExit(err);
559576

560577
#if CHIP_CONFIG_IM_PRETTY_PRINT
561578
writeRequestParser.PrettyPrint();
562-
#endif
579+
#endif // CHIP_CONFIG_IM_PRETTY_PRINT
563580
bool boolValue;
564581

565582
boolValue = mStateFlags.Has(StateBits::kSuppressResponse);
@@ -703,5 +720,32 @@ void WriteHandler::MoveToState(const State aTargetState)
703720
ChipLogDetail(DataManagement, "IM WH moving to [%s]", GetStateStr());
704721
}
705722

723+
CHIP_ERROR WriteHandler::WriteClusterData(const Access::SubjectDescriptor & aSubject, const ConcreteDataAttributePath & aPath,
724+
TLV::TLVReader & aData)
725+
{
726+
// Writes do not have a checked-path. If data model interface is enabled (both checked and only version)
727+
// the write is done via the DataModel interface
728+
#if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE
729+
VerifyOrReturnError(mDataModelProvider != nullptr, CHIP_ERROR_INCORRECT_STATE);
730+
731+
DataModel::WriteAttributeRequest request;
732+
733+
request.path = aPath;
734+
request.subjectDescriptor = aSubject;
735+
request.previousSuccessPath = mLastSuccessfullyWrittenPath;
736+
request.writeFlags.Set(DataModel::WriteFlags::kTimed, IsTimedWrite());
737+
738+
AttributeValueDecoder decoder(aData, aSubject);
739+
740+
DataModel::ActionReturnStatus status = mDataModelProvider->WriteAttribute(request, decoder);
741+
742+
mLastSuccessfullyWrittenPath = status.IsSuccess() ? std::make_optional(aPath) : std::nullopt;
743+
744+
return AddStatusInternal(aPath, StatusIB(status.GetStatusCode()));
745+
#else
746+
return WriteSingleClusterData(aSubject, aPath, aData, this);
747+
#endif // CHIP_CONFIG_USE_DATA_MODEL_INTERFACE
748+
}
749+
706750
} // namespace app
707751
} // namespace chip

‎src/app/WriteHandler.h

+13-1
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,10 @@
2121
#include <app/AppConfig.h>
2222
#include <app/AttributeAccessToken.h>
2323
#include <app/AttributePathParams.h>
24+
#include <app/ConcreteAttributePath.h>
2425
#include <app/InteractionModelDelegatePointers.h>
2526
#include <app/MessageDef/WriteResponseMessage.h>
27+
#include <app/data-model-provider/Provider.h>
2628
#include <lib/core/CHIPCore.h>
2729
#include <lib/core/TLVDebug.h>
2830
#include <lib/support/BitFlags.h>
@@ -69,6 +71,7 @@ class WriteHandler : public Messaging::ExchangeDelegate
6971
* construction until a call to Close is made to terminate the
7072
* instance.
7173
*
74+
* @param[in] apProvider A valid pointer to the model used to forward writes towards
7275
* @param[in] apWriteHandlerDelegate A Valid pointer to the WriteHandlerDelegate.
7376
*
7477
* @retval #CHIP_ERROR_INVALID_ARGUMENT on invalid pointers
@@ -77,7 +80,7 @@ class WriteHandler : public Messaging::ExchangeDelegate
7780
* @retval #CHIP_NO_ERROR On success.
7881
*
7982
*/
80-
CHIP_ERROR Init(WriteHandlerDelegate * apWriteHandlerDelegate);
83+
CHIP_ERROR Init(DataModel::Provider * apProvider, WriteHandlerDelegate * apWriteHandlerDelegate);
8184

8285
/**
8386
* Process a write request. Parts of the processing may end up being asynchronous, but the WriteHandler
@@ -182,11 +185,20 @@ class WriteHandler : public Messaging::ExchangeDelegate
182185
System::PacketBufferHandle && aPayload) override;
183186
void OnResponseTimeout(Messaging::ExchangeContext * apExchangeContext) override;
184187

188+
// Write the given data to the given path
189+
CHIP_ERROR WriteClusterData(const Access::SubjectDescriptor & aSubject, const ConcreteDataAttributePath & aPath,
190+
TLV::TLVReader & aData);
191+
185192
Messaging::ExchangeHolder mExchangeCtx;
186193
WriteResponseMessage::Builder mWriteResponseBuilder;
187194
Optional<ConcreteAttributePath> mProcessingAttributePath;
188195
Optional<AttributeAccessToken> mACLCheckCache = NullOptional;
189196

197+
#if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE
198+
DataModel::Provider * mDataModelProvider = nullptr;
199+
std::optional<ConcreteAttributePath> mLastSuccessfullyWrittenPath;
200+
#endif
201+
190202
// This may be a "fake" pointer or a real delegate pointer, depending
191203
// on CHIP_CONFIG_STATIC_GLOBAL_INTERACTION_MODEL_ENGINE setting.
192204
//

0 commit comments

Comments
 (0)
Please sign in to comment.