Skip to content

Commit

Permalink
Buffered List Chunk Reader (#11705)
Browse files Browse the repository at this point in the history
* Buffered TLV Reader

Once list chunking is implemented on the server, clients will need to be
updated to deal the the fact that lists will be delivered across
separate AttributeDataIBs and potentially, across multiple messages.

To reduce the burden of doing this on clients, this commit creates an
adapter class ('BufferedReadCallback') that sits in between the
application and the ReadClient and buffers up each list chunk before
reconsistiting them into a contiguous list and dispatching that up to
the application.

This buffering is done natively in TLV. This avoids the need to
understand the specifics of any data model types that the TLV will be
decoded into.

We cannot do this buffering at the next level up (i.e at the cluster
object level) since those objects are merely 'view' objects and not
proper containers. Consequently, there is no way to 'patch' the state in
those objects incrementally. By doing it at the TLV level, we can
preserve the existing cluster object APIs, which is a significant win.

Tests: A TestBufferedReadCallback test that exercises all possible
permutations of list and non list data to ensure the state machine is
validated.

* Review feedback + bug fix

* Review feedback

* Add better error handling

* Review feedback

* Fix ups

* Build fix

* More fixes

* Review feedback

* Fix crash due to incorrect null pointer being passed.

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
  • Loading branch information
2 people authored and pull[bot] committed Apr 14, 2022
1 parent fcc6004 commit bf76b2f
Show file tree
Hide file tree
Showing 34 changed files with 1,307 additions and 53 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class BridgedActionsAttrAccess : public AttributeAccessInterface
// Register for the Bridged Actions cluster on all endpoints.
BridgedActionsAttrAccess() : AttributeAccessInterface(Optional<EndpointId>::Missing(), BridgedActions::Id) {}

CHIP_ERROR Read(const ConcreteAttributePath & aPath, AttributeValueEncoder & aEncoder) override;
CHIP_ERROR Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder) override;

private:
static constexpr uint16_t ClusterRevision = 1;
Expand Down Expand Up @@ -73,7 +73,7 @@ CHIP_ERROR BridgedActionsAttrAccess::ReadClusterRevision(EndpointId endpoint, At

BridgedActionsAttrAccess gAttrAccess;

CHIP_ERROR BridgedActionsAttrAccess::Read(const ConcreteAttributePath & aPath, AttributeValueEncoder & aEncoder)
CHIP_ERROR BridgedActionsAttrAccess::Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder)
{
VerifyOrDie(aPath.mClusterId == BridgedActions::Id);

Expand Down
4 changes: 2 additions & 2 deletions examples/tv-app/android/include/cluster-init.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class TvAttrAccess : public app::AttributeAccessInterface
public:
TvAttrAccess() : app::AttributeAccessInterface(Optional<EndpointId>::Missing(), AttrTypeInfo::GetClusterId()) {}

CHIP_ERROR Read(const app::ConcreteAttributePath & aPath, app::AttributeValueEncoder & aEncoder) override
CHIP_ERROR Read(const app::ConcreteReadAttributePath & aPath, app::AttributeValueEncoder & aEncoder) override
{
if (aPath.mAttributeId == AttrTypeInfo::GetAttributeId())
{
Expand Down Expand Up @@ -195,7 +195,7 @@ class ContentLauncherAttrAccess : public app::AttributeAccessInterface
ContentLauncherAttrAccess() : app::AttributeAccessInterface(Optional<EndpointId>::Missing(), app::Clusters::ContentLauncher::Id)
{}

CHIP_ERROR Read(const app::ConcreteAttributePath & aPath, app::AttributeValueEncoder & aEncoder) override
CHIP_ERROR Read(const app::ConcreteReadAttributePath & aPath, app::AttributeValueEncoder & aEncoder) override
{
if (aPath.mAttributeId == app::Clusters::ContentLauncher::Attributes::AcceptsHeaderList::Id)
{
Expand Down
4 changes: 2 additions & 2 deletions examples/tv-app/linux/include/cluster-init.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class TvAttrAccess : public app::AttributeAccessInterface
public:
TvAttrAccess() : app::AttributeAccessInterface(Optional<EndpointId>::Missing(), AttrTypeInfo::GetClusterId()) {}

CHIP_ERROR Read(const app::ConcreteAttributePath & aPath, app::AttributeValueEncoder & aEncoder) override
CHIP_ERROR Read(const app::ConcreteReadAttributePath & aPath, app::AttributeValueEncoder & aEncoder) override
{
if (aPath.mAttributeId == AttrTypeInfo::GetAttributeId())
{
Expand Down Expand Up @@ -195,7 +195,7 @@ class ContentLauncherAttrAccess : public app::AttributeAccessInterface
ContentLauncherAttrAccess() : app::AttributeAccessInterface(Optional<EndpointId>::Missing(), app::Clusters::ContentLauncher::Id)
{}

CHIP_ERROR Read(const app::ConcreteAttributePath & aPath, app::AttributeValueEncoder & aEncoder) override
CHIP_ERROR Read(const app::ConcreteReadAttributePath & aPath, app::AttributeValueEncoder & aEncoder) override
{
if (aPath.mAttributeId == app::Clusters::ContentLauncher::Attributes::AcceptsHeaderList::Id)
{
Expand Down
4 changes: 2 additions & 2 deletions src/app/AttributeAccessInterface.h
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ class AttributeAccessInterface
* This may involve reading from the attribute store or external
* attribute callbacks.
*/
virtual CHIP_ERROR Read(const ConcreteAttributePath & aPath, AttributeValueEncoder & aEncoder) = 0;
virtual CHIP_ERROR Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder) = 0;

/**
* Callback for writing attributes.
Expand All @@ -159,7 +159,7 @@ class AttributeAccessInterface
* This may involve writing to the attribute store or external
* attribute callbacks.
*/
virtual CHIP_ERROR Write(const ConcreteAttributePath & aPath, AttributeValueDecoder & aDecoder) { return CHIP_NO_ERROR; }
virtual CHIP_ERROR Write(const ConcreteDataAttributePath & aPath, AttributeValueDecoder & aDecoder) { return CHIP_NO_ERROR; }

/**
* Mechanism for keeping track of a chain of AttributeAccessInterfaces.
Expand Down
2 changes: 1 addition & 1 deletion src/app/AttributePathExpandIterator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ bool AttributePathExpandIterator::Next()
}

// Reset to default, invalid value.
mOutputPath = ConcreteAttributePath();
mOutputPath = ConcreteReadAttributePath();
return false;
}
} // namespace app
Expand Down
1 change: 1 addition & 0 deletions src/app/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ static_library("app") {
"AttributePathExpandIterator.h",
"AttributePathParams.cpp",
"AttributePathParams.h",
"BufferedReadCallback.cpp",
"CASESessionManager.cpp",
"CASESessionManager.h",
"Command.cpp",
Expand Down
266 changes: 266 additions & 0 deletions src/app/BufferedReadCallback.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,266 @@
/*
*
* Copyright (c) 2021 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 "lib/core/CHIPTLV.h"
#include "lib/core/CHIPTLVTags.h"
#include "lib/core/CHIPTLVTypes.h"
#include "protocols/interaction_model/Constants.h"
#include "system/SystemPacketBuffer.h"
#include "system/TLVPacketBufferBackingStore.h"
#include <app/BufferedReadCallback.h>
#include <app/InteractionModelEngine.h>
#include <lib/support/ScopedBuffer.h>

namespace chip {
namespace app {

void BufferedReadCallback::OnReportBegin(const ReadClient * apReadClient)
{
mCallback.OnReportBegin(apReadClient);
}

void BufferedReadCallback::OnReportEnd(const ReadClient * apReadClient)
{
CHIP_ERROR err = DispatchBufferedData(apReadClient, mBufferedPath, StatusIB(), true);
if (err != CHIP_NO_ERROR)
{
mCallback.OnError(apReadClient, err);
}

mCallback.OnReportEnd(apReadClient);
}

CHIP_ERROR BufferedReadCallback::GenerateListTLV(TLV::ScopedBufferTLVReader & aReader)
{
TLV::TLVType outerType;
Platform::ScopedMemoryBuffer<uint8_t> backingBuffer;

//
// To generate the final reconstituted list, we need to allocate a contiguous
// buffer than can hold the entirety of its contents. To do so, we need to figure out
// how big a buffer to allocate. This requires walking the buffered list items and computing their TLV sizes,
// summing them all up and adding a bit of slop to account for the TLV array the list elements will go into.
//
// The alternative was to use a PacketBufferTLVWriter backed by chained packet buffers to
// write out the list - this would have removed the need for this first pass. However,
// we cannot actually back a TLVReader with a chained buffer since that violates the ability
// for us to create readers off-of readers. Each reader would assume exclusive ownership of the chained
// buffer and mutate the state within TLVPacketBufferBackingStore, preventing shared use.
//
// To avoid that, a single contiguous buffer is the best likely approach for now.
//
uint32_t totalBufSize = 0;
for (size_t i = 0; i < mBufferedList.size(); i++)
{
totalBufSize += mBufferedList[i]->TotalLength();
}

//
// Size of the start container and end container are just 1 byte each, but, let's just be safe.
//
totalBufSize += 4;

backingBuffer.Calloc(totalBufSize);
VerifyOrReturnError(backingBuffer.Get() != nullptr, CHIP_ERROR_NO_MEMORY);

TLV::ScopedBufferTLVWriter writer(std::move(backingBuffer), totalBufSize);
ReturnErrorOnFailure(writer.StartContainer(TLV::AnonymousTag, TLV::kTLVType_Array, outerType));

for (auto & bufHandle : mBufferedList)
{
System::PacketBufferTLVReader reader;

reader.Init(std::move(bufHandle));

ReturnErrorOnFailure(reader.Next());
ReturnErrorOnFailure(writer.CopyElement(TLV::AnonymousTag, reader));
}

ReturnErrorOnFailure(writer.EndContainer(outerType));

writer.Finalize(backingBuffer);

aReader.Init(std::move(backingBuffer), totalBufSize);

return CHIP_NO_ERROR;
}

CHIP_ERROR BufferedReadCallback::BufferListItem(TLV::TLVReader & reader)
{
System::PacketBufferTLVWriter writer;
System::PacketBufferHandle handle;

//
// We conservatively allocate a packet buffer as big as an IPv6 MTU (since we're buffering
// data received over the wire, which should always fit within that).
//
// We could have snapshotted the reader at its current position, advanced it past the current element
// and computed the delta in its read point to figure out the size of the element before allocating
// our target buffer. However, the reader's current position is already set past the control octet
// and the tag. Consequently, the computed size is always going to omit the sizes of these two parts of the
// TLV element. Since the tag can vary in size, for now, let's just do the safe thing. In the future, if this is a problem,
// we can improve this.
//
handle = System::PacketBufferHandle::New(chip::app::kMaxSecureSduLengthBytes);

writer.Init(std::move(handle), false);

ReturnErrorOnFailure(writer.CopyElement(TLV::AnonymousTag, reader));
ReturnErrorOnFailure(writer.Finalize(&handle));

// Compact the buffer down to a more reasonably sized packet buffer
// if we can.
//
handle.RightSize();

mBufferedList.push_back(std::move(handle));

return CHIP_NO_ERROR;
}

CHIP_ERROR BufferedReadCallback::BufferData(const ConcreteDataAttributePath & aPath, TLV::TLVReader * apData)
{

if (aPath.mListOp == ConcreteDataAttributePath::ListOperation::ReplaceAll)
{
TLV::TLVType outerContainer;

VerifyOrReturnError(apData->GetType() == TLV::kTLVType_Array, CHIP_ERROR_INVALID_TLV_ELEMENT);
mBufferedList.clear();

ReturnErrorOnFailure(apData->EnterContainer(outerContainer));

CHIP_ERROR err;

while ((err = apData->Next()) == CHIP_NO_ERROR)
{
ReturnErrorOnFailure(BufferListItem(*apData));
}

if (err == CHIP_END_OF_TLV)
{
err = CHIP_NO_ERROR;
}

ReturnErrorOnFailure(err);
ReturnErrorOnFailure(apData->ExitContainer(outerContainer));
}
else if (aPath.mListOp == ConcreteDataAttributePath::ListOperation::AppendItem)
{
ReturnErrorOnFailure(BufferListItem(*apData));
}

return CHIP_NO_ERROR;
}

CHIP_ERROR BufferedReadCallback::DispatchBufferedData(const ReadClient * apReadClient, const ConcreteAttributePath & aPath,
const StatusIB & aStatusIB, bool aEndOfReport)
{
if (aPath == mBufferedPath)
{
//
// If we encountered the same list again and it's not the last DataIB, then
// we need to continue to buffer up this list's data, so return immediately without dispatching
// the existing buffered up contents.
//
if (!aEndOfReport)
{
return CHIP_NO_ERROR;
}

//
// If we had previously buffered up data for this list and now we have encountered
// an error for this list, that error takes precedence and the buffered data is now
// rendered invalid. Return immediately without dispatching the existing buffered up contents.
//
if (aStatusIB.mStatus != Protocols::InteractionModel::Status::Success)
{
return CHIP_NO_ERROR;
}
}

if (!mBufferedPath.IsListOperation())
{
return CHIP_NO_ERROR;
}

StatusIB statusIB;
TLV::ScopedBufferTLVReader reader;

ReturnErrorOnFailure(GenerateListTLV(reader));

//
// Update the list operation to now reflect the delivery of the entire list
// i.e a replace all operation.
//
mBufferedPath.mListOp = ConcreteDataAttributePath::ListOperation::ReplaceAll;

//
// Advance the reader forward to the list itself
//
ReturnErrorOnFailure(reader.Next());

mCallback.OnAttributeData(apReadClient, mBufferedPath, &reader, statusIB);

//
// Clear out our buffered contents to free up allocated buffers, and reset the buffered path.
//
mBufferedList.clear();
mBufferedPath = ConcreteDataAttributePath();

return CHIP_NO_ERROR;
}

void BufferedReadCallback::OnAttributeData(const ReadClient * apReadClient, const ConcreteDataAttributePath & aPath,
TLV::TLVReader * apData, const StatusIB & aStatus)
{
CHIP_ERROR err;

//
// First, let's dispatch to our registered callback any buffered up list data from previous calls.
//
err = DispatchBufferedData(apReadClient, aPath, aStatus);
SuccessOrExit(err);

//
// We buffer up list data (only if the status was successful)
//
if (aPath.IsListOperation() && aStatus.mStatus == Protocols::InteractionModel::Status::Success)
{
err = BufferData(aPath, apData);
SuccessOrExit(err);
}
else
{
mCallback.OnAttributeData(apReadClient, aPath, apData, aStatus);
}

//
// Update our latched buffered path.
//
mBufferedPath = aPath;

exit:
if (err != CHIP_NO_ERROR)
{
mCallback.OnError(apReadClient, err);
}
}

} // namespace app
} // namespace chip
Loading

0 comments on commit bf76b2f

Please sign in to comment.