Skip to content

Commit

Permalink
Add read attribute positive and negative unit tests (#9317)
Browse files Browse the repository at this point in the history 
Summary of changes:
--Add read attribute postive test
--Add read attribute negative test and fix the exchange context close
bug when failing to read attribute in server.
--Add more test for clusterInfo to check if A include B.
--Minor cleanup for awaiting respone and onreadrequest rename and print this is first
report
  • Loading branch information
@yunhanw-google
yunhanw-google authored Sep 1, 2021
1 parent f67c711 commit 3bbbc69
Show file tree
Hide file tree
Showing 14 changed files with 259 additions and 106 deletions.
44 changes: 42 additions & 2 deletions src/app/ClusterInfo.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,13 @@

#include <app/AttributePathParams.h>
#include <app/util/basic-types.h>
#include <assert.h>
#include <core/Optional.h>

namespace chip {
namespace app {
static constexpr AttributeId kRootAttributeId = 0xFFFFFFFF;

struct ClusterInfo
{
enum class Flags : uint8_t
Expand All @@ -39,15 +43,51 @@ struct ClusterInfo
return false;
}

if (mFlags.Has(Flags::kFieldIdValid) && (mFieldId == 0xFFFFFFFF))
Optional<AttributeId> myFieldId =
mFlags.Has(Flags::kFieldIdValid) ? Optional<AttributeId>::Value(mFieldId) : Optional<AttributeId>::Missing();

Optional<AttributeId> otherFieldId = other.mFlags.Has(Flags::kFieldIdValid) ? Optional<AttributeId>::Value(other.mFieldId)
: Optional<AttributeId>::Missing();

Optional<ListIndex> myListIndex =
mFlags.Has(Flags::kListIndexValid) ? Optional<ListIndex>::Value(mListIndex) : Optional<ListIndex>::Missing();

Optional<ListIndex> otherListIndex = other.mFlags.Has(Flags::kListIndexValid) ? Optional<ListIndex>::Value(other.mListIndex)
: Optional<ListIndex>::Missing();

// If list index exists, field index must exist
// Field 0xFFFFFFF (any) & listindex set is invalid
assert(!(myListIndex.HasValue() && !myFieldId.HasValue()));
assert(!(otherListIndex.HasValue() && !otherFieldId.HasValue()));
assert(!(myFieldId == Optional<AttributeId>::Value(kRootAttributeId) && myListIndex.HasValue()));
assert(!(otherFieldId == Optional<AttributeId>::Value(kRootAttributeId) && otherListIndex.HasValue()));

if (myFieldId == Optional<AttributeId>::Value(kRootAttributeId))
{
return true;
}

if (mFlags.Has(Flags::kFieldIdValid) && other.mFlags.Has(Flags::kFieldIdValid) && (mFieldId == other.mFieldId))
if (myFieldId != otherFieldId)
{
return false;
}

// We only support top layer for attribute representation, either FieldId or FieldId + ListIndex
// Combination: if myFieldId == otherFieldId, ListIndex cannot exist without FieldId
// 1. myListIndex and otherListIndex both missing or both exactly the same, then current is superset of other
// 2. myListIndex is missing, no matter if otherListIndex is missing or not, then current is superset of other
if (myListIndex == otherListIndex)
{
// either both missing or both exactly the same
return true;
}

if (!myListIndex.HasValue())
{
// difference is ok only if myListIndex is missing
return true;
}

return false;
}

Expand Down
11 changes: 6 additions & 5 deletions src/app/InteractionModelEngine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -229,8 +229,9 @@ CHIP_ERROR InteractionModelEngine::OnInvokeCommandRequest(Messaging::ExchangeCon
return err;
}

CHIP_ERROR InteractionModelEngine::OnReadRequest(Messaging::ExchangeContext * apExchangeContext, const PacketHeader & aPacketHeader,
const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload)
CHIP_ERROR InteractionModelEngine::OnReadInitialRequest(Messaging::ExchangeContext * apExchangeContext,
const PacketHeader & aPacketHeader, const PayloadHeader & aPayloadHeader,
System::PacketBufferHandle && aPayload)
{
CHIP_ERROR err = CHIP_NO_ERROR;

Expand All @@ -240,9 +241,9 @@ CHIP_ERROR InteractionModelEngine::OnReadRequest(Messaging::ExchangeContext * ap
{
if (readHandler.IsFree())
{
err = readHandler.Init(mpDelegate, apExchangeContext);
err = readHandler.Init(mpExchangeMgr, mpDelegate, apExchangeContext);
SuccessOrExit(err);
err = readHandler.OnReadRequest(std::move(aPayload));
err = readHandler.OnReadInitialRequest(std::move(aPayload));
apExchangeContext = nullptr;
break;
}
Expand Down Expand Up @@ -299,7 +300,7 @@ CHIP_ERROR InteractionModelEngine::OnMessageReceived(Messaging::ExchangeContext
}
else if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::ReadRequest))
{
err = OnReadRequest(apExchangeContext, aPacketHeader, aPayloadHeader, std::move(aPayload));
err = OnReadInitialRequest(apExchangeContext, aPacketHeader, aPayloadHeader, std::move(aPayload));
}
else if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::WriteRequest))
{
Expand Down
5 changes: 2 additions & 3 deletions src/app/InteractionModelEngine.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ namespace chip {
namespace app {

static constexpr size_t kMaxSecureSduLengthBytes = 1024;
static constexpr FieldId kRootFieldId = 0;

/**
* @class InteractionModelEngine
Expand Down Expand Up @@ -157,8 +156,8 @@ class InteractionModelEngine : public Messaging::ExchangeDelegate
* Called when Interaction Model receives a Read Request message. Errors processing
* the Read Request are handled entirely within this function.
*/
CHIP_ERROR OnReadRequest(Messaging::ExchangeContext * apExchangeContext, const PacketHeader & aPacketHeader,
const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload);
CHIP_ERROR OnReadInitialRequest(Messaging::ExchangeContext * apExchangeContext, const PacketHeader & aPacketHeader,
const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload);

/**
* Called when Interaction Model receives a Write Request message. Errors processing
Expand Down
47 changes: 22 additions & 25 deletions src/app/ReadClient.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,13 @@ CHIP_ERROR ReadClient::Init(Messaging::ExchangeManager * apExchangeMgr, Interact
CHIP_ERROR err = CHIP_NO_ERROR;
// Error if already initialized.
VerifyOrExit(apExchangeMgr != nullptr, err = CHIP_ERROR_INCORRECT_STATE);
VerifyOrExit(mpExchangeMgr == nullptr, err = CHIP_ERROR_INCORRECT_STATE);
VerifyOrExit(mpExchangeMgr == nullptr, err = CHIP_ERROR_INVALID_ARGUMENT);

mpExchangeMgr = apExchangeMgr;
mpDelegate = apDelegate;
mState = ClientState::Initialized;
mAppIdentifier = aAppIdentifier;

mInitialReport = true;
AbortExistingExchangeContext();

exit:
Expand All @@ -65,6 +65,7 @@ void ReadClient::ShutdownInternal(CHIP_ERROR aError)
mpDelegate->ReadDone(this, aError);
mpDelegate = nullptr;
}
mInitialReport = true;
MoveToState(ClientState::Uninitialized);
}

Expand All @@ -77,8 +78,8 @@ const char * ReadClient::GetStateStr() const
return "UNINIT";
case ClientState::Initialized:
return "INIT";
case ClientState::AwaitingResponse:
return "AwaitingResponse";
case ClientState::AwaitingInitialReport:
return "AwaitingInitialReport";
}
#endif // CHIP_DETAIL_LOGGING
return "N/A";
Expand Down Expand Up @@ -154,7 +155,7 @@ CHIP_ERROR ReadClient::SendReadRequest(ReadPrepareParams & aReadPrepareParams)
err = mpExchangeCtx->SendMessage(Protocols::InteractionModel::MsgType::ReadRequest, std::move(msgBuf),
Messaging::SendFlags(Messaging::SendMessageFlags::kExpectResponse));
SuccessOrExit(err);
MoveToState(ClientState::AwaitingResponse);
MoveToState(ClientState::AwaitingInitialReport);

exit:
ChipLogFunctError(err);
Expand All @@ -167,11 +168,12 @@ CHIP_ERROR ReadClient::SendReadRequest(ReadPrepareParams & aReadPrepareParams)
return err;
}

CHIP_ERROR ReadClient::SendStatusReport(CHIP_ERROR aError, bool aExpectResponse)
CHIP_ERROR ReadClient::SendStatusReport(CHIP_ERROR aError)
{
Protocols::SecureChannel::GeneralStatusCode generalCode = Protocols::SecureChannel::GeneralStatusCode::kSuccess;
uint32_t protocolId = Protocols::InteractionModel::Id.ToFullyQualifiedSpecForm();
uint16_t protocolCode = to_underlying(Protocols::InteractionModel::ProtocolCode::Success);
bool expectResponse = false;
VerifyOrReturnLogError(mpExchangeCtx != nullptr, CHIP_ERROR_INCORRECT_STATE);

if (aError != CHIP_NO_ERROR)
Expand All @@ -180,24 +182,16 @@ CHIP_ERROR ReadClient::SendStatusReport(CHIP_ERROR aError, bool aExpectResponse)
protocolCode = to_underlying(Protocols::InteractionModel::ProtocolCode::InvalidSubscription);
}

ChipLogProgress(DataManagement, "SendStatusReport with error %s", ErrorStr(aError));
Protocols::SecureChannel::StatusReport report(generalCode, protocolId, protocolCode);

Encoding::LittleEndian::PacketBufferWriter buf(System::PacketBufferHandle::New(kMaxSecureSduLengthBytes));
report.WriteToBuffer(buf);
System::PacketBufferHandle msgBuf = buf.Finalize();
VerifyOrReturnLogError(!msgBuf.IsNull(), CHIP_ERROR_NO_MEMORY);

if (aExpectResponse)
{
ReturnLogErrorOnFailure(mpExchangeCtx->SendMessage(Protocols::SecureChannel::MsgType::StatusReport, std::move(msgBuf),
Messaging::SendFlags(Messaging::SendMessageFlags::kExpectResponse)));
}
else
{
ReturnLogErrorOnFailure(mpExchangeCtx->SendMessage(Protocols::SecureChannel::MsgType::StatusReport, std::move(msgBuf)));
}

ReturnLogErrorOnFailure(mpExchangeCtx->SendMessage(
Protocols::SecureChannel::MsgType::StatusReport, std::move(msgBuf),
Messaging::SendFlags(expectResponse ? Messaging::SendMessageFlags::kExpectResponse : Messaging::SendMessageFlags::kNone)));
return CHIP_NO_ERROR;
}

Expand Down Expand Up @@ -265,16 +259,12 @@ CHIP_ERROR ReadClient::OnMessageReceived(Messaging::ExchangeContext * apExchange
{
err = ProcessReportData(std::move(aPayload));
SuccessOrExit(err);
mpDelegate->ReportProcessed(this);
}
else
{
err = CHIP_ERROR_INVALID_MESSAGE_TYPE;
}

SuccessOrExit(err);
err = SendStatusReport(err, false);

exit:
ChipLogFunctError(err);
if (err != CHIP_NO_ERROR)
Expand Down Expand Up @@ -329,6 +319,10 @@ CHIP_ERROR ReadClient::ProcessReportData(System::PacketBufferHandle && aPayload)
}
SuccessOrExit(err);

if (IsInitialReport())
{
ChipLogProgress(DataManagement, "ProcessReportData handles the initial report");
}
err = report.GetMoreChunkedMessages(&moreChunkedMessages);
if (CHIP_END_OF_TLV == err)
{
Expand Down Expand Up @@ -373,7 +367,13 @@ CHIP_ERROR ReadClient::ProcessReportData(System::PacketBufferHandle && aPayload)
// are multiple reports
}

if (err == CHIP_NO_ERROR)
{
mpDelegate->ReportProcessed(this);
}
exit:
SendStatusReport(err);
ClearInitialReport();
ChipLogFunctError(err);
return err;
}
Expand All @@ -384,10 +384,7 @@ void ReadClient::OnResponseTimeout(Messaging::ExchangeContext * apExchangeContex
apExchangeContext->GetExchangeId());
if (nullptr != mpDelegate)
{
if (ClientState::AwaitingResponse == mState)
{
mpDelegate->ReadError(this, CHIP_ERROR_TIMEOUT);
}
mpDelegate->ReadError(this, CHIP_ERROR_TIMEOUT);
}
ShutdownInternal(CHIP_ERROR_TIMEOUT);
}
Expand Down
14 changes: 8 additions & 6 deletions src/app/ReadClient.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,17 +80,17 @@ class ReadClient : public Messaging::ExchangeDelegate

uint64_t GetAppIdentifier() const { return mAppIdentifier; }
Messaging::ExchangeContext * GetExchangeContext() const { return mpExchangeCtx; }
CHIP_ERROR SendStatusReport(CHIP_ERROR aError, bool aExpectResponse);
CHIP_ERROR SendStatusReport(CHIP_ERROR aError);

private:
friend class TestReadInteraction;
friend class InteractionModelEngine;

enum class ClientState
{
Uninitialized = 0, ///< The client has not been initialized
Initialized, ///< The client has been initialized and is ready for a SendReadRequest
AwaitingResponse, ///< The client has sent out the read request message
Uninitialized = 0, ///< The client has not been initialized
Initialized, ///< The client has been initialized and is ready for a SendReadRequest
AwaitingInitialReport, ///< The client is waiting for initial report
};

/**
Expand Down Expand Up @@ -119,7 +119,7 @@ class ReadClient : public Messaging::ExchangeDelegate
*
*/
bool IsFree() const { return mState == ClientState::Uninitialized; }

bool IsAwaitingInitialReport() const { return mState == ClientState::AwaitingInitialReport; }
CHIP_ERROR GenerateEventPathList(EventPathList::Builder & aEventPathListBuilder, EventPathParams * apEventPathParamsList,
size_t aEventPathParamsListSize);
CHIP_ERROR GenerateAttributePathList(AttributePathList::Builder & aAttributeathListBuilder,
Expand All @@ -139,12 +139,14 @@ class ReadClient : public Messaging::ExchangeDelegate
* our exchange and don't need to manually close it.
*/
void ShutdownInternal(CHIP_ERROR aError);

void ClearInitialReport() { mInitialReport = false; }
bool IsInitialReport() { return mInitialReport; }
Messaging::ExchangeManager * mpExchangeMgr = nullptr;
Messaging::ExchangeContext * mpExchangeCtx = nullptr;
InteractionModelDelegate * mpDelegate = nullptr;
ClientState mState = ClientState::Uninitialized;
uint64_t mAppIdentifier = 0;
bool mInitialReport = true;
};

}; // namespace app
Expand Down
Loading

0 comments on commit 3bbbc69

Please sign in to comment.