Skip to content

Commit

Permalink
Use ConcreteAttributePath for reads below ReadSingleClusterData.
Browse files Browse the repository at this point in the history
The ReadSingleClusterData in TestReportingEngine.cpp was not being
used, because it had the wrong signature.  And making it be used
by fixing the signature apparently makes the test fail...
  • Loading branch information
bzbarsky-apple committed Oct 11, 2021
1 parent 02cbf69 commit 827aedd
Show file tree
Hide file tree
Showing 13 changed files with 57 additions and 74 deletions.
6 changes: 3 additions & 3 deletions src/app/AttributeAccessInterface.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

#pragma once

#include <app/ClusterInfo.h>
#include <app/ConcreteAttributePath.h>
#include <app/MessageDef/AttributeDataElement.h>
#include <app/data-model/Encode.h>
#include <app/data-model/List.h> // So we can encode lists
Expand Down Expand Up @@ -108,7 +108,7 @@ class AttributeAccessInterface
/**
* Callback for reading attributes.
*
* @param [in] aClusterInfo indicates which exact data is being read.
* @param [in] aPath indicates which exact data is being read.
* @param [in] aEncoder the AttributeValueEncoder to use for encoding the
* data. If this function returns scucess and no attempt is
* made to encode data using aEncoder, the
Expand All @@ -117,7 +117,7 @@ class AttributeAccessInterface
* This may involve reading from the attribute store or external
* attribute callbacks.
*/
virtual CHIP_ERROR Read(ClusterInfo & aClusterInfo, AttributeValueEncoder & aEncoder) = 0;
virtual CHIP_ERROR Read(const ConcreteAttributePath & aPath, AttributeValueEncoder & aEncoder) = 0;

/**
* Mechanism for keeping track of a chain of AttributeAccessInterfaces.
Expand Down
9 changes: 7 additions & 2 deletions src/app/InteractionModelEngine.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
#include <app/Command.h>
#include <app/CommandHandler.h>
#include <app/CommandSender.h>
#include <app/ConcreteAttributePath.h>
#include <app/ConcreteCommandPath.h>
#include <app/InteractionModelDelegate.h>
#include <app/ReadClient.h>
Expand Down Expand Up @@ -231,15 +232,19 @@ bool ServerClusterCommandExists(const ConcreteCommandPath & aCommandPath);
* This function is implemented by CHIP as a part of cluster data storage & management.
* The apWriter and apDataExists can be nullptr.
*
* @param[in] aClusterInfo The cluster info object, for the path of cluster data.
* @param[in] aPath The concrete path of the data being read.
* @param[in] apWriter The TLVWriter for holding cluster data. Can be a nullptr if the caller does not care
* the exact value of the attribute.
* @param[out] apDataExists Tell whether the cluster data exist on server. Can be a nullptr if the caller does not care
* whether the data exists.
*
* @retval CHIP_NO_ERROR on success
*/
CHIP_ERROR ReadSingleClusterData(ClusterInfo & aClusterInfo, TLV::TLVWriter * apWriter, bool * apDataExists);
CHIP_ERROR ReadSingleClusterData(const ConcreteAttributePath & aPath, TLV::TLVWriter * apWriter, bool * apDataExists);

/**
* TODO: Document.
*/
CHIP_ERROR WriteSingleClusterData(ClusterInfo & aClusterInfo, TLV::TLVReader & aReader, WriteHandler * apWriteHandler);
} // namespace app
} // namespace chip
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class EthernetDiagosticsAttrAccess : public AttributeAccessInterface
// Register for the EthernetNetworkDiagnostics cluster on all endpoints.
EthernetDiagosticsAttrAccess() : AttributeAccessInterface(Optional<EndpointId>::Missing(), EthernetNetworkDiagnostics::Id) {}

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

private:
template <typename T>
Expand All @@ -69,15 +69,15 @@ CHIP_ERROR EthernetDiagosticsAttrAccess::ReadIfSupported(CHIP_ERROR (Connectivit

EthernetDiagosticsAttrAccess gAttrAccess;

CHIP_ERROR EthernetDiagosticsAttrAccess::Read(ClusterInfo & aClusterInfo, AttributeValueEncoder & aEncoder)
CHIP_ERROR EthernetDiagosticsAttrAccess::Read(const ConcreteAttributePath & aPath, AttributeValueEncoder & aEncoder)
{
if (aClusterInfo.mClusterId != EthernetNetworkDiagnostics::Id)
if (aPath.mClusterId != EthernetNetworkDiagnostics::Id)
{
// We shouldn't have been called at all.
return CHIP_ERROR_INVALID_ARGUMENT;
}

switch (aClusterInfo.mFieldId)
switch (aPath.mAttributeId)
{
case PHYRate::Id: {
return ReadIfSupported(&ConnectivityManager::GetEthPHYRate, aEncoder);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class GeneralDiagosticsAttrAccess : public AttributeAccessInterface
// Register for the GeneralDiagnostics cluster on all endpoints.
GeneralDiagosticsAttrAccess() : AttributeAccessInterface(Optional<EndpointId>::Missing(), GeneralDiagnostics::Id) {}

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

private:
template <typename T>
Expand All @@ -62,15 +62,15 @@ CHIP_ERROR GeneralDiagosticsAttrAccess::ReadIfSupported(CHIP_ERROR (PlatformMana

GeneralDiagosticsAttrAccess gAttrAccess;

CHIP_ERROR GeneralDiagosticsAttrAccess::Read(ClusterInfo & aClusterInfo, AttributeValueEncoder & aEncoder)
CHIP_ERROR GeneralDiagosticsAttrAccess::Read(const ConcreteAttributePath & aPath, AttributeValueEncoder & aEncoder)
{
if (aClusterInfo.mClusterId != GeneralDiagnostics::Id)
if (aPath.mClusterId != GeneralDiagnostics::Id)
{
// We shouldn't have been called at all.
return CHIP_ERROR_INVALID_ARGUMENT;
}

switch (aClusterInfo.mFieldId)
switch (aPath.mAttributeId)
{
case RebootCount::Id: {
return ReadIfSupported(&PlatformManager::GetRebootCount, aEncoder);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,23 +42,23 @@ class SoftwareDiagosticsAttrAccess : public AttributeAccessInterface
// Register for the SoftwareDiagnostics cluster on all endpoints.
SoftwareDiagosticsAttrAccess() : AttributeAccessInterface(Optional<EndpointId>::Missing(), SoftwareDiagnostics::Id) {}

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

private:
CHIP_ERROR ReadIfSupported(CHIP_ERROR (PlatformManager::*getter)(uint64_t &), AttributeValueEncoder & aEncoder);
};

SoftwareDiagosticsAttrAccess gAttrAccess;

CHIP_ERROR SoftwareDiagosticsAttrAccess::Read(ClusterInfo & aClusterInfo, AttributeValueEncoder & aEncoder)
CHIP_ERROR SoftwareDiagosticsAttrAccess::Read(const ConcreteAttributePath & aPath, AttributeValueEncoder & aEncoder)
{
if (aClusterInfo.mClusterId != SoftwareDiagnostics::Id)
if (aPath.mClusterId != SoftwareDiagnostics::Id)
{
// We shouldn't have been called at all.
return CHIP_ERROR_INVALID_ARGUMENT;
}

switch (aClusterInfo.mFieldId)
switch (aPath.mAttributeId)
{
case CurrentHeapFree::Id: {
return ReadIfSupported(&PlatformManager::GetCurrentHeapFree, aEncoder);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,20 +46,20 @@ class ThreadDiagosticsAttrAccess : public AttributeAccessInterface
// Register for the ThreadNetworkDiagnostics cluster on all endpoints.
ThreadDiagosticsAttrAccess() : AttributeAccessInterface(Optional<EndpointId>::Missing(), ThreadNetworkDiagnostics::Id) {}

CHIP_ERROR Read(ClusterInfo & aClusterInfo, AttributeValueEncoder & aEncoder) override;
CHIP_ERROR Read(const ConcreteAttributePath & aPath, AttributeValueEncoder & aEncoder) override;
};

ThreadDiagosticsAttrAccess gAttrAccess;

CHIP_ERROR ThreadDiagosticsAttrAccess::Read(ClusterInfo & aClusterInfo, AttributeValueEncoder & aEncoder)
CHIP_ERROR ThreadDiagosticsAttrAccess::Read(const ConcreteAttributePath & aPath, AttributeValueEncoder & aEncoder)
{
if (aClusterInfo.mClusterId != ThreadNetworkDiagnostics::Id)
if (aPath.mClusterId != ThreadNetworkDiagnostics::Id)
{
// We shouldn't have been called at all.
return CHIP_ERROR_INVALID_ARGUMENT;
}

CHIP_ERROR err = ConnectivityMgr().WriteThreadNetworkDiagnosticAttributeToTlv(aClusterInfo.mFieldId, aEncoder);
CHIP_ERROR err = ConnectivityMgr().WriteThreadNetworkDiagnosticAttributeToTlv(aPath.mAttributeId, aEncoder);

// If it isn't a run time assigned attribute, e.g. ClusterRevision, or if
// not implemented, clear the error so we fall back to the standard read
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class WiFiDiagosticsAttrAccess : public AttributeAccessInterface
// Register for the WiFiNetworkDiagnostics cluster on all endpoints.
WiFiDiagosticsAttrAccess() : AttributeAccessInterface(Optional<EndpointId>::Missing(), WiFiNetworkDiagnostics::Id) {}

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

private:
template <typename T>
Expand All @@ -69,15 +69,15 @@ CHIP_ERROR WiFiDiagosticsAttrAccess::ReadIfSupported(CHIP_ERROR (ConnectivityMan

WiFiDiagosticsAttrAccess gAttrAccess;

CHIP_ERROR WiFiDiagosticsAttrAccess::Read(ClusterInfo & aClusterInfo, AttributeValueEncoder & aEncoder)
CHIP_ERROR WiFiDiagosticsAttrAccess::Read(const ConcreteAttributePath & aPath, AttributeValueEncoder & aEncoder)
{
if (aClusterInfo.mClusterId != WiFiNetworkDiagnostics::Id)
if (aPath.mClusterId != WiFiNetworkDiagnostics::Id)
{
// We shouldn't have been called at all.
return CHIP_ERROR_INVALID_ARGUMENT;
}

switch (aClusterInfo.mFieldId)
switch (aPath.mAttributeId)
{
case Attributes::SecurityType::Id: {
return ReadIfSupported(&ConnectivityManager::GetWiFiSecurityType, aEncoder);
Expand Down
5 changes: 3 additions & 2 deletions src/app/reporting/Engine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ EventNumber Engine::CountEvents(ReadHandler * apReadHandler, EventNumber * apIni
CHIP_ERROR
Engine::RetrieveClusterData(AttributeDataList::Builder & aAttributeDataList, ClusterInfo & aClusterInfo)
{
CHIP_ERROR err = CHIP_NO_ERROR;
CHIP_ERROR err = CHIP_NO_ERROR;
ConcreteAttributePath path(aClusterInfo.mEndpointId, aClusterInfo.mClusterId, aClusterInfo.mFieldId);
AttributeDataElement::Builder attributeDataElementBuilder = aAttributeDataList.CreateAttributeDataElementBuilder();
AttributePath::Builder attributePathBuilder = attributeDataElementBuilder.CreateAttributePathBuilder();
attributePathBuilder.NodeId(aClusterInfo.mNodeId)
Expand All @@ -78,7 +79,7 @@ Engine::RetrieveClusterData(AttributeDataList::Builder & aAttributeDataList, Clu
ChipLogDetail(DataManagement, "<RE:Run> Cluster %" PRIx32 ", Field %" PRIx32 " is dirty", aClusterInfo.mClusterId,
aClusterInfo.mFieldId);

err = ReadSingleClusterData(aClusterInfo, attributeDataElementBuilder.GetWriter(), nullptr /* data exists */);
err = ReadSingleClusterData(path, attributeDataElementBuilder.GetWriter(), nullptr /* data exists */);
SuccessOrExit(err);
attributeDataElementBuilder.MoreClusterData(false);
attributeDataElementBuilder.EndOfAttributeDataElement();
Expand Down
12 changes: 6 additions & 6 deletions src/app/tests/TestReadInteraction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
*
*/

#include <app/AttributeAccessInterface.h>
#include <app/InteractionModelEngine.h>
#include <app/MessageDef/EventDataElement.h>
#include <app/util/basic-types.h>
Expand Down Expand Up @@ -219,29 +220,28 @@ class MockInteractionModelApp : public chip::app::InteractionModelDelegate

namespace chip {
namespace app {
CHIP_ERROR ReadSingleClusterData(ClusterInfo & aClusterInfo, TLV::TLVWriter * apWriter, bool * apDataExists)
CHIP_ERROR ReadSingleClusterData(const ConcreteAttributePath & aPath, TLV::TLVWriter * apWriter, bool * apDataExists)
{
uint64_t version = 0;
ChipLogDetail(DataManagement, "TEST Cluster %" PRIx32 ", Field %" PRIx32 " is dirty", aClusterInfo.mClusterId,
aClusterInfo.mFieldId);
ChipLogDetail(DataManagement, "TEST Cluster %" PRIx32 ", Field %" PRIx32 " is dirty", aPath.mClusterId, aPath.mAttributeId);

if (apDataExists != nullptr)
{
*apDataExists = (aClusterInfo.mClusterId == kTestClusterId && aClusterInfo.mEndpointId == kTestEndpointId);
*apDataExists = (aPath.mClusterId == kTestClusterId && aPath.mEndpointId == kTestEndpointId);
}

if (apWriter == nullptr)
{
return CHIP_NO_ERROR;
}

if (!(aClusterInfo.mClusterId == kTestClusterId && aClusterInfo.mEndpointId == kTestEndpointId))
if (!(aPath.mClusterId == kTestClusterId && aPath.mEndpointId == kTestEndpointId))
{
return apWriter->Put(chip::TLV::ContextTag(AttributeDataElement::kCsTag_Status),
chip::Protocols::InteractionModel::Status::UnsupportedAttribute);
}

ReturnErrorOnFailure(apWriter->Put(TLV::ContextTag(AttributeDataElement::kCsTag_Data), kTestFieldValue1));
ReturnErrorOnFailure(AttributeValueEncoder(apWriter).Encode(kTestFieldValue1));
return apWriter->Put(TLV::ContextTag(AttributeDataElement::kCsTag_DataVersion), version);
}

Expand Down
25 changes: 1 addition & 24 deletions src/app/tests/TestReportingEngine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
*
*/

#include <app/ConcreteAttributePath.h>
#include <app/InteractionModelEngine.h>
#include <app/reporting/Engine.h>
#include <lib/core/CHIPCore.h>
Expand Down Expand Up @@ -54,32 +55,8 @@ constexpr ClusterId kTestClusterId = 6;
constexpr EndpointId kTestEndpointId = 1;
constexpr chip::AttributeId kTestFieldId1 = 1;
constexpr chip::AttributeId kTestFieldId2 = 2;
constexpr uint8_t kTestFieldValue1 = 1;
constexpr uint8_t kTestFieldValue2 = 2;

namespace app {
CHIP_ERROR ReadSingleClusterData(AttributePathParams & aAttributePathParams, TLV::TLVWriter * apWriter, bool * apDataExists)
{
CHIP_ERROR err = CHIP_NO_ERROR;
VerifyOrExit(aAttributePathParams.mClusterId == kTestClusterId && aAttributePathParams.mEndpointId == kTestEndpointId,
err = CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrExit(apWriter != nullptr, /* no op */);

if (aAttributePathParams.mFieldId == kTestFieldId1)
{
err = apWriter->Put(TLV::ContextTag(kTestFieldId1), kTestFieldValue1);
SuccessOrExit(err);
}
if (aAttributePathParams.mFieldId == kTestFieldId2)
{
err = apWriter->Put(TLV::ContextTag(kTestFieldId2), kTestFieldValue2);
SuccessOrExit(err);
}

exit:
return err;
}

namespace reporting {
class TestReportingEngine
{
Expand Down
3 changes: 2 additions & 1 deletion src/app/tests/integration/chip_im_initiator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include "system/SystemClock.h"
#include <app/CommandHandler.h>
#include <app/CommandSender.h>
#include <app/ConcreteAttributePath.h>
#include <app/InteractionModelEngine.h>
#include <app/tests/integration/common.h>
#include <chrono>
Expand Down Expand Up @@ -635,7 +636,7 @@ void DispatchSingleClusterResponseCommand(const ConcreteCommandPath & aCommandPa
gLastCommandResult = TestCommandResult::kSuccess;
}

CHIP_ERROR ReadSingleClusterData(ClusterInfo & aClusterInfo, TLV::TLVWriter * apWriter, bool * apDataExists)
CHIP_ERROR ReadSingleClusterData(const ConcreteAttributePath & aPath, TLV::TLVWriter * apWriter, bool * apDataExists)
{
// We do not really care about the value, just return a not found status code.
VerifyOrReturnError(apWriter != nullptr, CHIP_NO_ERROR);
Expand Down
8 changes: 4 additions & 4 deletions src/app/tests/integration/chip_im_responder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
*/

#include "MockEvents.h"
#include <app/AttributeAccessInterface.h>
#include <app/CommandHandler.h>
#include <app/CommandSender.h>
#include <app/EventManagement.h>
Expand Down Expand Up @@ -110,15 +111,14 @@ void DispatchSingleClusterResponseCommand(const ConcreteCommandPath & aCommandPa
// Nothing todo.
}

CHIP_ERROR ReadSingleClusterData(ClusterInfo & aClusterInfo, TLV::TLVWriter * apWriter, bool * apDataExists)
CHIP_ERROR ReadSingleClusterData(const ConcreteAttributePath & aPath, TLV::TLVWriter * apWriter, bool * apDataExists)
{
CHIP_ERROR err = CHIP_NO_ERROR;
uint64_t version = 0;
VerifyOrExit(aClusterInfo.mClusterId == kTestClusterId && aClusterInfo.mEndpointId == kTestEndpointId,
err = CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrExit(aPath.mClusterId == kTestClusterId && aPath.mEndpointId == kTestEndpointId, err = CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrExit(apWriter != nullptr, /* no op */);

err = apWriter->Put(TLV::ContextTag(AttributeDataElement::kCsTag_Data), kTestFieldValue1);
err = AttributeValueEncoder(apWriter).Encode(kTestFieldValue1);
SuccessOrExit(err);
err = apWriter->Put(TLV::ContextTag(AttributeDataElement::kCsTag_DataVersion), version);

Expand Down
23 changes: 11 additions & 12 deletions src/app/util/ember-compatibility-functions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

#include <app/ClusterInfo.h>
#include <app/Command.h>
#include <app/ConcreteAttributePath.h>
#include <app/InteractionModelEngine.h>
#include <app/reporting/Engine.h>
#include <app/util/af.h>
Expand Down Expand Up @@ -189,21 +190,19 @@ bool ServerClusterCommandExists(const ConcreteCommandPath & aCommandPath)
return emberAfContainsServer(aCommandPath.mEndpointId, aCommandPath.mClusterId);
}

CHIP_ERROR ReadSingleClusterData(ClusterInfo & aClusterInfo, TLV::TLVWriter * apWriter, bool * apDataExists)
CHIP_ERROR ReadSingleClusterData(const ConcreteAttributePath & aPath, TLV::TLVWriter * apWriter, bool * apDataExists)
{
ChipLogDetail(DataManagement,
"Received Cluster Command: Cluster=" ChipLogFormatMEI " NodeId=0x" ChipLogFormatX64 " Endpoint=%" PRIx16
" AttributeId=%" PRIx32 " ListIndex=%" PRIx16,
ChipLogValueMEI(aClusterInfo.mClusterId), ChipLogValueX64(aClusterInfo.mNodeId), aClusterInfo.mEndpointId,
aClusterInfo.mFieldId, aClusterInfo.mListIndex);
"Reading attribute: Cluster=" ChipLogFormatMEI " Endpoint=%" PRIx16 " AttributeId=" ChipLogFormatMEI,
ChipLogValueMEI(aPath.mClusterId), aPath.mEndpointId, ChipLogValueMEI(aPath.mAttributeId));

AttributeAccessInterface * attrOverride = findAttributeAccessOverride(aClusterInfo.mEndpointId, aClusterInfo.mClusterId);
AttributeAccessInterface * attrOverride = findAttributeAccessOverride(aPath.mEndpointId, aPath.mClusterId);
if (attrOverride != nullptr)
{
// TODO: We should probably clone the writer and convert failures here
// into status responses, unless our caller already does that.
AttributeValueEncoder valueEncoder(apWriter);
ReturnErrorOnFailure(attrOverride->Read(aClusterInfo, valueEncoder));
ReturnErrorOnFailure(attrOverride->Read(aPath, valueEncoder));

if (valueEncoder.TriedEncode())
{
Expand All @@ -223,8 +222,8 @@ CHIP_ERROR ReadSingleClusterData(ClusterInfo & aClusterInfo, TLV::TLVWriter * ap

EmberAfAttributeType attributeType;
EmberAfStatus status;
status = emberAfReadAttribute(aClusterInfo.mEndpointId, aClusterInfo.mClusterId, aClusterInfo.mFieldId, CLUSTER_MASK_SERVER,
attributeData, sizeof(attributeData), &attributeType);
status = emberAfReadAttribute(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId, CLUSTER_MASK_SERVER, attributeData,
sizeof(attributeData), &attributeType);

if (apDataExists != nullptr)
{
Expand Down Expand Up @@ -349,9 +348,9 @@ CHIP_ERROR ReadSingleClusterData(ClusterInfo & aClusterInfo, TLV::TLVWriter * ap
ReturnErrorOnFailure(
apWriter->StartContainer(TLV::ContextTag(AttributeDataElement::kCsTag_Data), TLV::kTLVType_List, containerType));
// TODO: Encode data in TLV, now raw buffers
ReturnErrorOnFailure(apWriter->PutBytes(
TLV::AnonymousTag, attributeData,
emberAfAttributeValueSize(aClusterInfo.mClusterId, aClusterInfo.mFieldId, attributeType, attributeData)));
ReturnErrorOnFailure(
apWriter->PutBytes(TLV::AnonymousTag, attributeData,
emberAfAttributeValueSize(aPath.mClusterId, aPath.mAttributeId, attributeType, attributeData)));
ReturnErrorOnFailure(apWriter->EndContainer(containerType));
break;
}
Expand Down

0 comments on commit 827aedd

Please sign in to comment.