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.

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

Implement DataModel based read support in the interaction model engine. #34419

Merged
merged 38 commits into from
Jul 24, 2024
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
9fff27d
Implement DataModel based read support in the interacton model engine.
andreilitvin Jul 19, 2024
118bcce
Comment update and logic update: we MAY get different sizes if data s…
andreilitvin Jul 19, 2024
df4bd20
Fix typo for ember implementation while renaming things
andreilitvin Jul 19, 2024
b4c9809
Fix override markers
andreilitvin Jul 19, 2024
5d597d5
run_tv_casting_test should be executable
andy31415 Jul 19, 2024
970c356
Merge branch 'master' into imdm/7-use-dm-reads
andy31415 Jul 19, 2024
3d3791a
Do not report errors on chunking
andy31415 Jul 22, 2024
05f714e
Typo fix
andy31415 Jul 22, 2024
2bcbe27
move chipDie for tests only
andy31415 Jul 22, 2024
a186d3b
move all chipdie to unit test only
andy31415 Jul 22, 2024
3670a77
fix comment and formatting
andy31415 Jul 22, 2024
b61814f
Update encoderstate logic a bit - code is cleaner, will have to see i…
andy31415 Jul 22, 2024
0d2e9c3
Restyle
andy31415 Jul 22, 2024
3a838cc
Merge branch 'master' into imdm/7-use-dm-reads
andy31415 Jul 22, 2024
6b98f28
Enable tracing of messages for tv tests, to see what is going on in CI
andy31415 Jul 22, 2024
8732d88
Restyle
andy31415 Jul 22, 2024
075c01d
Start adding some log line processing for the tv tests, to have propp…
andy31415 Jul 22, 2024
334a525
Significant reformat and start refactoring the tv casting test
andreilitvin Jul 23, 2024
d8bd693
TV tests pass
andreilitvin Jul 23, 2024
fb69c49
Restyle
andreilitvin Jul 23, 2024
83c2420
Fix ruff
andreilitvin Jul 23, 2024
056500e
Review comment update: set state in all cases
andreilitvin Jul 23, 2024
e85095a
Added a TODO regarding the awkward "callback on success only"
andreilitvin Jul 23, 2024
0d18be2
Merge branch 'master' into imdm/7-use-dm-reads
andreilitvin Jul 23, 2024
952e6cc
Merge fix
andreilitvin Jul 23, 2024
584bfeb
Merge branch 'master' into imdm/7-use-dm-reads
andreilitvin Jul 23, 2024
d4dd842
Update src/app/reporting/Read-Checked.cpp
andy31415 Jul 23, 2024
dd92d3e
Review updates
andreilitvin Jul 23, 2024
cbd0748
Fix placement of dm state
andreilitvin Jul 23, 2024
008e44e
Restyle
andreilitvin Jul 23, 2024
56143e8
Code review comments: double-check that IM not active when setting mo…
andreilitvin Jul 24, 2024
f00dd06
Code review: comment why we did not re-use code
andreilitvin Jul 24, 2024
d3e3c2b
Code review feedback: warn if running in checked mode
andreilitvin Jul 24, 2024
5ea3356
Restyle
andreilitvin Jul 24, 2024
276bbdd
Merge branch 'master' into imdm/7-use-dm-reads
andreilitvin Jul 24, 2024
c91c21b
Avoid loop of err/out empty output
andreilitvin Jul 24, 2024
262d08f
Support a log directory argument for the casting tests, so I can debu…
andreilitvin Jul 24, 2024
6019d0e
Better debuggability and error reporting support for shell - this is …
andreilitvin Jul 24, 2024
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
1 change: 1 addition & 0 deletions src/app/AttributeValueEncoder.h
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ class AttributeValueEncoder
private:
// We made EncodeListItem() private, and ListEncoderHelper will expose it by Encode()
friend class ListEncodeHelper;
friend class TestOnlyAttributeValueEncoderAccessor;

template <typename... Ts>
CHIP_ERROR EncodeListItem(Ts &&... aArgs)
Expand Down
34 changes: 24 additions & 10 deletions src/app/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -58,16 +58,6 @@ declare_args() {
chip_im_static_global_interaction_model_engine =
current_os != "linux" && current_os != "mac" && current_os != "ios" &&
current_os != "android"

# Data model interface usage:
# - disabled: does not use data model interface at all
# - check: runs BOTH datamodel and non-data-model (if possible) functionality and compares results
# - enabled: runs only the data model interface (does not use the legacy code)
if (current_os == "linux") {
chip_use_data_model_interface = "check"
} else {
chip_use_data_model_interface = "disabled"
}
}

buildconfig_header("app_buildconfig") {
Expand Down Expand Up @@ -219,6 +209,7 @@ static_library("interaction-model") {
"WriteClient.h",
"reporting/Engine.cpp",
"reporting/Engine.h",
"reporting/Read.h",
"reporting/ReportScheduler.h",
"reporting/ReportSchedulerImpl.cpp",
"reporting/ReportSchedulerImpl.h",
Expand Down Expand Up @@ -260,6 +251,29 @@ static_library("interaction-model") {

public_configs = [ "${chip_root}/src:includes" ]

if (chip_use_data_model_interface == "disabled") {
sources += [
"reporting/Read-Ember.cpp",
"reporting/Read-Ember.h",
]
} else if (chip_use_data_model_interface == "check") {
andy31415 marked this conversation as resolved.
Show resolved Hide resolved
sources += [
"reporting/Read-Checked.cpp",
"reporting/Read-Checked.h",
"reporting/Read-DataModel.cpp",
"reporting/Read-DataModel.h",
"reporting/Read-Ember.cpp",
"reporting/Read-Ember.h",
]
public_deps += [ "${chip_root}/src/app/data-model-interface" ]
} else { # enabled
sources += [
"reporting/Read-DataModel.cpp",
"reporting/Read-DataModel.h",
]
public_deps += [ "${chip_root}/src/app/data-model-interface" ]
}

if (chip_enable_read_client) {
sources += [ "ReadClient.cpp" ]
}
Expand Down
7 changes: 7 additions & 0 deletions src/app/InteractionModelEngine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1697,6 +1697,13 @@ Protocols::InteractionModel::Status InteractionModelEngine::CommandExists(const
return ServerClusterCommandExists(aCommandPath);
}

InteractionModel::DataModel * InteractionModelEngine::SetDataModel(InteractionModel::DataModel * model)
andy31415 marked this conversation as resolved.
Show resolved Hide resolved
{
InteractionModel::DataModel * oldModel = GetDataModel();
mDataModel = model;
return oldModel;
}

InteractionModel::DataModel * InteractionModelEngine::GetDataModel() const
{
#if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE
Expand Down
7 changes: 7 additions & 0 deletions src/app/InteractionModelEngine.h
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,13 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,

InteractionModel::DataModel * GetDataModel() const;

// MUST NOT be used while the interaction model engine is running as interaction
andy31415 marked this conversation as resolved.
Show resolved Hide resolved
// model functionality (e.g. active reads/writes/subscriptions) rely on data model
// state
//
// Returns the old data model value.
InteractionModel::DataModel * SetDataModel(InteractionModel::DataModel * model);

private:
friend class reporting::Engine;
friend class TestCommandInteraction;
Expand Down
10 changes: 10 additions & 0 deletions src/app/common_flags.gni
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,14 @@ declare_args() {
# Flag that controls whether the time-to-wait from BUSY responses is
# communicated to OperationalSessionSetup API consumers.
chip_enable_busy_handling_for_operational_session_setup = true

# Data model interface usage:
# - disabled: does not use data model interface at all
# - check: runs BOTH datamodel and non-data-model (if possible) functionality and compares results
# - enabled: runs only the data model interface (does not use the legacy code)
if (current_os == "linux") {
chip_use_data_model_interface = "check"
} else {
chip_use_data_model_interface = "disabled"
}
}
1 change: 0 additions & 1 deletion src/app/data-model-interface/OperationTypes.h
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ enum class ReadFlags : uint32_t
struct ReadAttributeRequest : OperationRequest
{
ConcreteAttributePath path;
std::optional<DataVersion> dataVersion;
BitFlags<ReadFlags> readFlags;
};

Expand Down
24 changes: 3 additions & 21 deletions src/app/reporting/Engine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include <app/InteractionModelEngine.h>
#include <app/RequiredPrivilege.h>
#include <app/reporting/Engine.h>
#include <app/reporting/Read.h>
#include <app/util/MatterCallbacks.h>
#include <app/util/ember-compatibility-functions.h>

Expand Down Expand Up @@ -79,25 +80,6 @@ bool Engine::IsClusterDataVersionMatch(const SingleLinkedListNode<DataVersionFil
return existPathMatch && !existVersionMismatch;
}

CHIP_ERROR
Engine::RetrieveClusterData(const SubjectDescriptor & aSubjectDescriptor, bool aIsFabricFiltered,
AttributeReportIBs::Builder & aAttributeReportIBs, const ConcreteReadAttributePath & aPath,
AttributeEncodeState * aEncoderState)
{
ChipLogDetail(DataManagement, "<RE:Run> Cluster %" PRIx32 ", Attribute %" PRIx32 " is dirty", aPath.mClusterId,
aPath.mAttributeId);

DataModelCallbacks::GetInstance()->AttributeOperation(DataModelCallbacks::OperationType::Read,
DataModelCallbacks::OperationOrder::Pre, aPath);

ReturnErrorOnFailure(ReadSingleClusterData(aSubjectDescriptor, aIsFabricFiltered, aPath, aAttributeReportIBs, aEncoderState));

DataModelCallbacks::GetInstance()->AttributeOperation(DataModelCallbacks::OperationType::Read,
DataModelCallbacks::OperationOrder::Post, aPath);

return CHIP_NO_ERROR;
}

static bool IsOutOfWriterSpaceError(CHIP_ERROR err)
{
return err == CHIP_ERROR_NO_MEMORY || err == CHIP_ERROR_BUFFER_TOO_SMALL;
Expand Down Expand Up @@ -200,8 +182,8 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu
ConcreteReadAttributePath pathForRetrieval(readPath);
// Load the saved state from previous encoding session for chunking of one single attribute (list chunking).
AttributeEncodeState encodeState = apReadHandler->GetAttributeEncodeState();
err = RetrieveClusterData(apReadHandler->GetSubjectDescriptor(), apReadHandler->IsFabricFiltered(), attributeReportIBs,
pathForRetrieval, &encodeState);
err = Impl::RetrieveClusterData(mpImEngine->GetDataModel(), apReadHandler->GetSubjectDescriptor(),
apReadHandler->IsFabricFiltered(), attributeReportIBs, pathForRetrieval, &encodeState);
if (err != CHIP_NO_ERROR)
{
// If error is not an "out of writer space" error, rollback and encode status.
Expand Down
3 changes: 0 additions & 3 deletions src/app/reporting/Engine.h
Original file line number Diff line number Diff line change
Expand Up @@ -168,9 +168,6 @@ class Engine
bool * apHasMoreChunks, bool * apHasEncodedData);
CHIP_ERROR BuildSingleReportDataEventReports(ReportDataMessage::Builder & reportDataBuilder, ReadHandler * apReadHandler,
bool aBufferIsUsed, bool * apHasMoreChunks, bool * apHasEncodedData);
CHIP_ERROR RetrieveClusterData(const Access::SubjectDescriptor & aSubjectDescriptor, bool aIsFabricFiltered,
AttributeReportIBs::Builder & aAttributeReportIBs,
const ConcreteReadAttributePath & aClusterInfo, AttributeEncodeState * apEncoderState);
CHIP_ERROR CheckAccessDeniedEventPaths(TLV::TLVWriter & aWriter, bool & aHasEncodedData, ReadHandler * apReadHandler);

// If version match, it means don't send, if version mismatch, it means send.
Expand Down
160 changes: 160 additions & 0 deletions src/app/reporting/Read-Checked.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
/*
* Copyright (c) 2024 Project CHIP Authors
* All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
#include <app/reporting/Read-Checked.h>

#include <app/reporting/Read-DataModel.h>
#include <app/reporting/Read-Ember.h>
#include <app/util/MatterCallbacks.h>

namespace chip {
namespace app {
namespace reporting {
namespace CheckedImpl {
namespace {

/// Checkpoints and saves the state (including error state) for a
/// AttributeReportIBs::Builder
class ScopedAttributeReportIBsBuilderState
{
public:
ScopedAttributeReportIBsBuilderState(AttributeReportIBs::Builder & builder) : mBuilder(builder), mError(mBuilder.GetError())
{
mBuilder.Checkpoint(mCheckpoint);
}

~ScopedAttributeReportIBsBuilderState()
{
mBuilder.Rollback(mCheckpoint);
mBuilder.ResetError(mError);
}

private:
AttributeReportIBs::Builder & mBuilder;
chip::TLV::TLVWriter mCheckpoint;
CHIP_ERROR mError;
};

} // namespace

CHIP_ERROR RetrieveClusterData(InteractionModel::DataModel * dataModel, const Access::SubjectDescriptor & subjectDescriptor,
bool isFabricFiltered, AttributeReportIBs::Builder & reportBuilder,
const ConcreteReadAttributePath & path, AttributeEncodeState * encoderState)
{
ChipLogDetail(DataManagement, "<RE:Run> Cluster %" PRIx32 ", Attribute %" PRIx32 " is dirty", path.mClusterId,
path.mAttributeId);
DataModelCallbacks::GetInstance()->AttributeOperation(DataModelCallbacks::OperationType::Read,
DataModelCallbacks::OperationOrder::Pre, path);

CHIP_ERROR errEmber = CHIP_NO_ERROR;
uint32_t lengthWrittenEmber = 0;

// a copy for DM logic only. Ember changes state directly
// IMPORTANT: the copy MUST be taken BEFORE ember processes/changes encoderState inline.
AttributeEncodeState stateDm(encoderState);

{
ScopedAttributeReportIBsBuilderState builderState(reportBuilder); // temporary only
errEmber =
EmberImpl::RetrieveClusterData(dataModel, subjectDescriptor, isFabricFiltered, reportBuilder, path, encoderState);
lengthWrittenEmber = reportBuilder.GetWriter()->GetLengthWritten();
}

CHIP_ERROR errDM = DataModelImpl::RetrieveClusterData(dataModel, subjectDescriptor, isFabricFiltered, reportBuilder, path,
encoderState != nullptr ? &stateDm : nullptr);

if (errEmber != errDM)
{
// Note log + chipDie instead of VerifyOrDie so that breakpoints (and usage of rr)
// is easier to debug.
ChipLogError(Test, "Different return codes between ember and DM");
ChipLogError(Test, " Ember error: %" CHIP_ERROR_FORMAT, errEmber.Format());
ChipLogError(Test, " DM error: %" CHIP_ERROR_FORMAT, errDM.Format());

// For time-dependent data, we may have size differences here: one data fitting in buffer
// while another not, resulting in different errors (success vs out of space).
//
// Make unit tests strict; otherwise allow it with potentially odd mismatch errors
// (in which case logs will be odd, however we also expect Checked versions to only
// run for a short period until we switch over to either ember or DM completely).
#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
chipDie();
#endif
}

// data should be identical for most cases EXCEPT that for time-deltas (e.g. seconds since boot or similar)
// it may actually differ. As a result, the amount of data written in bytes MUST be the same, however if the rest of the
andy31415 marked this conversation as resolved.
Show resolved Hide resolved
// data is not the same, we just print it out as a warning for manual inspection
//
// We have no direct access to TLV buffer data (especially given backing store splits)
// so for now we check that data length was identical.
//
// NOTE: RetrieveClusterData is responsible for encoding StatusIB errors in case of failures
// so we validate length written requirements for BOTH success and failure.
//
// NOTE: data length is NOT reliable if the data content differs in encoding length. E.g. numbers changing
// from 0xFF to 0x100 or similar will use up more space.
// For unit tests we make the validation strict, however for runtime we just report an
// error for different sizes.
if (lengthWrittenEmber != reportBuilder.GetWriter()->GetLengthWritten())
{
ChipLogError(Test, "Different written length: %" PRIu32 " (Ember) vs %" PRIu32 " (DataModel)", lengthWrittenEmber,
reportBuilder.GetWriter()->GetLengthWritten());
#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
chipDie();
#endif
}

// For chunked reads, the encoder state MUST be identical (since this is what controls
// where chunking resumes).
if ((errEmber == CHIP_ERROR_NO_MEMORY) || (errEmber == CHIP_ERROR_BUFFER_TOO_SMALL))
{
// Encoder state MUST match on partial reads (used by chunking)
// specifically ReadViaAccessInterface in ember-compatibility-functions only
// sets the encoder state in case an error occurs.
if (encoderState != nullptr)
{
if (encoderState->AllowPartialData() != stateDm.AllowPartialData())
{
ChipLogError(Test, "Different partial data");
// NOTE: die on unit tests only, since partial data size may differ across
// time-dependent data (very rarely because fast code, but still possible)
#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
chipDie();
#endif
}
if (encoderState->CurrentEncodingListIndex() != stateDm.CurrentEncodingListIndex())
{
ChipLogError(Test, "Different partial data");
// NOTE: die on unit tests only, since partial data size may differ across
// time-dependent data (very rarely because fast code, but still possible)
#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
chipDie();
andy31415 marked this conversation as resolved.
Show resolved Hide resolved
#endif
}
}
}

DataModelCallbacks::GetInstance()->AttributeOperation(DataModelCallbacks::OperationType::Read,
DataModelCallbacks::OperationOrder::Post, path);

return errDM;
}

} // namespace CheckedImpl
} // namespace reporting
} // namespace app
} // namespace chip
37 changes: 37 additions & 0 deletions src/app/reporting/Read-Checked.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* Copyright (c) 2024 Project CHIP Authors
* All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
#pragma once

#include <access/SubjectDescriptor.h>
#include <app/AttributeEncodeState.h>
#include <app/MessageDef/AttributeReportIBs.h>
#include <app/data-model-interface/DataModel.h>
#include <lib/core/CHIPError.h>

namespace chip {
namespace app {
namespace reporting {
namespace CheckedImpl {

CHIP_ERROR RetrieveClusterData(InteractionModel::DataModel * dataModel, const Access::SubjectDescriptor & subjectDescriptor,
bool isFabricFiltered, AttributeReportIBs::Builder & reportBuilder,
const ConcreteReadAttributePath & path, AttributeEncodeState * encoderState);

} // namespace CheckedImpl
} // namespace reporting
} // namespace app
} // namespace chip
Loading
Loading