Skip to content

Commit

Permalink
RFC: Remove the ReadClient* parameter to ReadClient::Callbacks
Browse files Browse the repository at this point in the history 
Currently each ReadClient callback takes as a parameter a pointer to the
ReadClient itself. This is not very useful and introduces undesirable
coupling: It prevents creating another class (say, MockReadClient or
NextGenReadClient) that is compatible with existing callbacks
without the same concrete implementation class.

This is showing up very concretely in tests that are passing "nullptr"
since their test double cannot be passed through the callback.
  • Loading branch information
@mspang
mspang committed Jan 26, 2022
1 parent 5429dca commit c7aa9a1
Show file tree
Hide file tree
Showing 16 changed files with 170 additions and 187 deletions.
13 changes: 6 additions & 7 deletions src/app/AttributeCache.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,14 +66,14 @@ CHIP_ERROR AttributeCache::UpdateCache(const ConcreteDataAttributePath & aPath,
return CHIP_NO_ERROR;
}

void AttributeCache::OnReportBegin(const ReadClient * apReadClient)
void AttributeCache::OnReportBegin()
{
mChangedAttributeSet.clear();
mAddedEndpoints.clear();
mCallback.OnReportBegin(apReadClient);
mCallback.OnReportBegin();
}

void AttributeCache::OnReportEnd(const ReadClient * apReadClient)
void AttributeCache::OnReportEnd()
{
std::set<std::tuple<EndpointId, ClusterId>> changedClusters;

Expand All @@ -97,11 +97,10 @@ void AttributeCache::OnReportEnd(const ReadClient * apReadClient)
mCallback.OnEndpointAdded(this, endpoint);
}

mCallback.OnReportEnd(apReadClient);
mCallback.OnReportEnd();
}

void AttributeCache::OnAttributeData(const ReadClient * apReadClient, const ConcreteDataAttributePath & aPath,
TLV::TLVReader * apData, const StatusIB & aStatus)
void AttributeCache::OnAttributeData(const ConcreteDataAttributePath & aPath, TLV::TLVReader * apData, const StatusIB & aStatus)
{
//
// Since the cache itself is a ReadClient::Callback, it may be incorrectly passed in directly when registering with the
Expand All @@ -121,7 +120,7 @@ void AttributeCache::OnAttributeData(const ReadClient * apReadClient, const Conc
//
// Forward the call through.
//
mCallback.OnAttributeData(apReadClient, aPath, apData, aStatus);
mCallback.OnAttributeData(aPath, apData, aStatus);
}

CHIP_ERROR AttributeCache::Get(const ConcreteAttributePath & path, TLV::TLVReader & reader)
Expand Down
20 changes: 9 additions & 11 deletions src/app/AttributeCache.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -339,20 +339,18 @@ class AttributeCache : protected ReadClient::Callback
//
// ReadClient::Callback
//
void OnReportBegin(const ReadClient * apReadClient) override;
void OnReportEnd(const ReadClient * apReadClient) override;
void OnAttributeData(const ReadClient * apReadClient, const ConcreteDataAttributePath & aPath, TLV::TLVReader * apData,
const StatusIB & aStatus) override;
void OnError(const ReadClient * apReadClient, CHIP_ERROR aError) override { return mCallback.OnError(apReadClient, aError); }

void OnEventData(const ReadClient * apReadClient, const EventHeader & aEventHeader, TLV::TLVReader * apData,
const StatusIB * apStatus) override
void OnReportBegin() override;
void OnReportEnd() override;
void OnAttributeData(const ConcreteDataAttributePath & aPath, TLV::TLVReader * apData, const StatusIB & aStatus) override;
void OnError(CHIP_ERROR aError) override { return mCallback.OnError(aError); }

void OnEventData(const EventHeader & aEventHeader, TLV::TLVReader * apData, const StatusIB * apStatus) override
{
return mCallback.OnEventData(apReadClient, aEventHeader, apData, apStatus);
return mCallback.OnEventData(aEventHeader, apData, apStatus);
}

void OnDone(ReadClient * apReadClient) override { return mCallback.OnDone(apReadClient); }
void OnSubscriptionEstablished(const ReadClient * apReadClient) override { mCallback.OnSubscriptionEstablished(apReadClient); }
void OnDone() override { return mCallback.OnDone(); }
void OnSubscriptionEstablished(uint64_t aSubscriptionId) override { mCallback.OnSubscriptionEstablished(aSubscriptionId); }

private:
Callback & mCallback;
Expand Down
28 changes: 14 additions & 14 deletions src/app/BufferedReadCallback.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,20 +29,20 @@
namespace chip {
namespace app {

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

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

mCallback.OnReportEnd(apReadClient);
mCallback.OnReportEnd();
}

CHIP_ERROR BufferedReadCallback::GenerateListTLV(TLV::ScopedBufferTLVReader & aReader)
Expand Down Expand Up @@ -168,8 +168,8 @@ CHIP_ERROR BufferedReadCallback::BufferData(const ConcreteDataAttributePath & aP
return CHIP_NO_ERROR;
}

CHIP_ERROR BufferedReadCallback::DispatchBufferedData(const ReadClient * apReadClient, const ConcreteAttributePath & aPath,
const StatusIB & aStatusIB, bool aEndOfReport)
CHIP_ERROR BufferedReadCallback::DispatchBufferedData(const ConcreteAttributePath & aPath, const StatusIB & aStatusIB,
bool aEndOfReport)
{
if (aPath == mBufferedPath)
{
Expand Down Expand Up @@ -215,7 +215,7 @@ CHIP_ERROR BufferedReadCallback::DispatchBufferedData(const ReadClient * apReadC
//
ReturnErrorOnFailure(reader.Next());

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

//
// Clear out our buffered contents to free up allocated buffers, and reset the buffered path.
Expand All @@ -226,15 +226,15 @@ CHIP_ERROR BufferedReadCallback::DispatchBufferedData(const ReadClient * apReadC
return CHIP_NO_ERROR;
}

void BufferedReadCallback::OnAttributeData(const ReadClient * apReadClient, const ConcreteDataAttributePath & aPath,
TLV::TLVReader * apData, const StatusIB & aStatus)
void BufferedReadCallback::OnAttributeData(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);
err = DispatchBufferedData(aPath, aStatus);
SuccessOrExit(err);

//
Expand All @@ -247,7 +247,7 @@ void BufferedReadCallback::OnAttributeData(const ReadClient * apReadClient, cons
}
else
{
mCallback.OnAttributeData(apReadClient, aPath, apData, aStatus);
mCallback.OnAttributeData(aPath, apData, aStatus);
}

//
Expand All @@ -258,7 +258,7 @@ void BufferedReadCallback::OnAttributeData(const ReadClient * apReadClient, cons
exit:
if (err != CHIP_NO_ERROR)
{
mCallback.OnError(apReadClient, err);
mCallback.OnError(err);
}
}

Expand Down
21 changes: 9 additions & 12 deletions src/app/BufferedReadCallback.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,7 @@ class BufferedReadCallback : public ReadClient::Callback
* 2. The path provided in aPath is similar to what is buffered but we've hit the end of the report.
*
*/
CHIP_ERROR DispatchBufferedData(const ReadClient * apReadClient, const ConcreteAttributePath & aPath, const StatusIB & aStatus,
bool aEndOfReport = false);
CHIP_ERROR DispatchBufferedData(const ConcreteAttributePath & aPath, const StatusIB & aStatus, bool aEndOfReport = false);

/*
* Buffer up list data as they arrive.
Expand All @@ -68,19 +67,17 @@ class BufferedReadCallback : public ReadClient::Callback
//
// ReadClient::Callback
//
void OnReportBegin(const ReadClient * apReadClient) override;
void OnReportEnd(const ReadClient * apReadClient) override;
void OnAttributeData(const ReadClient * apReadClient, const ConcreteDataAttributePath & aPath, TLV::TLVReader * apData,
const StatusIB & aStatus) override;
void OnError(const ReadClient * apReadClient, CHIP_ERROR aError) override { return mCallback.OnError(apReadClient, aError); }
void OnEventData(const ReadClient * apReadClient, const EventHeader & aEventHeader, TLV::TLVReader * apData,
const StatusIB * apStatus) override
void OnReportBegin() override;
void OnReportEnd() override;
void OnAttributeData(const ConcreteDataAttributePath & aPath, TLV::TLVReader * apData, const StatusIB & aStatus) override;
void OnError(CHIP_ERROR aError) override { return mCallback.OnError(aError); }
void OnEventData(const EventHeader & aEventHeader, TLV::TLVReader * apData, const StatusIB * apStatus) override
{
return mCallback.OnEventData(apReadClient, aEventHeader, apData, apStatus);
return mCallback.OnEventData(aEventHeader, apData, apStatus);
}

void OnDone(ReadClient * apReadClient) override { return mCallback.OnDone(apReadClient); }
void OnSubscriptionEstablished(const ReadClient * apReadClient) override { mCallback.OnSubscriptionEstablished(apReadClient); }
void OnDone() override { return mCallback.OnDone(); }
void OnSubscriptionEstablished(uint64_t aSubscriptionId) override { mCallback.OnSubscriptionEstablished(aSubscriptionId); }

private:
/*
Expand Down
16 changes: 8 additions & 8 deletions src/app/ReadClient.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,10 @@ void ReadClient::Close(CHIP_ERROR aError)

if (aError != CHIP_NO_ERROR)
{
mpCallback.OnError(this, aError);
mpCallback.OnError(aError);
}

mpCallback.OnDone(this);
mpCallback.OnDone();
}

const char * ReadClient::GetStateStr() const
Expand Down Expand Up @@ -404,7 +404,7 @@ CHIP_ERROR ReadClient::ProcessReportData(System::PacketBufferHandle && aPayload)

if (mIsInitialReport)
{
mpCallback.OnReportBegin(this);
mpCallback.OnReportBegin();
mIsInitialReport = false;
}

Expand All @@ -413,7 +413,7 @@ CHIP_ERROR ReadClient::ProcessReportData(System::PacketBufferHandle && aPayload)

if (!mPendingMoreChunks)
{
mpCallback.OnReportEnd(this);
mpCallback.OnReportEnd();
mIsInitialReport = true;
}
}
Expand Down Expand Up @@ -510,7 +510,7 @@ CHIP_ERROR ReadClient::ProcessAttributeReportIBs(TLV::TLVReader & aAttributeRepo
ReturnErrorOnFailure(ProcessAttributePath(path, attributePath));
ReturnErrorOnFailure(status.GetErrorStatus(&errorStatus));
ReturnErrorOnFailure(errorStatus.DecodeStatusIB(statusIB));
mpCallback.OnAttributeData(this, attributePath, nullptr, statusIB);
mpCallback.OnAttributeData(attributePath, nullptr, statusIB);
}
else if (CHIP_END_OF_TLV == err)
{
Expand All @@ -526,7 +526,7 @@ CHIP_ERROR ReadClient::ProcessAttributeReportIBs(TLV::TLVReader & aAttributeRepo
attributePath.mListOp = ConcreteDataAttributePath::ListOperation::ReplaceAll;
}

mpCallback.OnAttributeData(this, attributePath, &dataReader, statusIB);
mpCallback.OnAttributeData(attributePath, &dataReader, statusIB);
}
}

Expand Down Expand Up @@ -559,7 +559,7 @@ CHIP_ERROR ReadClient::ProcessEventReportIBs(TLV::TLVReader & aEventReportIBsRea
mEventMin = header.mEventNumber + 1;
ReturnErrorOnFailure(data.GetData(&dataReader));

mpCallback.OnEventData(this, header, &dataReader, nullptr);
mpCallback.OnEventData(header, &dataReader, nullptr);
}

if (CHIP_END_OF_TLV == err)
Expand Down Expand Up @@ -635,7 +635,7 @@ CHIP_ERROR ReadClient::ProcessSubscribeResponse(System::PacketBufferHandle && aP
ReturnLogErrorOnFailure(subscribeResponse.GetMinIntervalFloorSeconds(&mMinIntervalFloorSeconds));
ReturnLogErrorOnFailure(subscribeResponse.GetMaxIntervalCeilingSeconds(&mMaxIntervalCeilingSeconds));

mpCallback.OnSubscriptionEstablished(this);
mpCallback.OnSubscriptionEstablished(subscriptionId);

MoveToState(ClientState::SubscriptionActive);

Expand Down
24 changes: 8 additions & 16 deletions src/app/ReadClient.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ class ReadClient : public Messaging::ExchangeDelegate
* receives an OnDone call to destroy the object.
*
*/
virtual void OnReportBegin(const ReadClient * apReadClient) {}
virtual void OnReportBegin() {}

/**
* Used to signal the completion of processing of the last attribute report in a given exchange.
Expand All @@ -81,7 +81,7 @@ class ReadClient : public Messaging::ExchangeDelegate
* receives an OnDone call to destroy the object.
*
*/
virtual void OnReportEnd(const ReadClient * apReadClient) {}
virtual void OnReportEnd() {}

/**
* Used to deliver event data received through the Read and Subscribe interactions
Expand All @@ -91,15 +91,12 @@ class ReadClient : public Messaging::ExchangeDelegate
* This object MUST continue to exist after this call is completed. The application shall wait until it
* receives an OnDone call to destroy the object.
*
* @param[in] apReadClient: The read client object that initiated the read or subscribe transaction.
* @param[in] aEventHeader: The event header in report response.
* @param[in] apData: A TLVReader positioned right on the payload of the event.
* @param[in] apStatus: Event-specific status, containing an InteractionModel::Status code as well as an optional
* cluster-specific status code.
*/
virtual void OnEventData(const ReadClient * apReadClient, const EventHeader & aEventHeader, TLV::TLVReader * apData,
const StatusIB * apStatus)
{}
virtual void OnEventData(const EventHeader & aEventHeader, TLV::TLVReader * apData, const StatusIB * apStatus) {}

/**
* Used to deliver attribute data received through the Read and Subscribe interactions.
Expand All @@ -112,25 +109,22 @@ class ReadClient : public Messaging::ExchangeDelegate
* This object MUST continue to exist after this call is completed. The application shall wait until it
* receives an OnDone call to destroy the object.
*
* @param[in] apReadClient The read client object that initiated the read or subscribe transaction.
* @param[in] aPath The attribute path field in report response.
* @param[in] apData The attribute data of the given path, will be a nullptr if status is not Success.
* @param[in] aStatus Attribute-specific status, containing an InteractionModel::Status code as well as an
* optional cluster-specific status code.
*/
virtual void OnAttributeData(const ReadClient * apReadClient, const ConcreteDataAttributePath & aPath,
TLV::TLVReader * apData, const StatusIB & aStatus)
{}
virtual void OnAttributeData(const ConcreteDataAttributePath & aPath, TLV::TLVReader * apData, const StatusIB & aStatus) {}

/**
* OnSubscriptionEstablished will be called when a subscription is established for the given subscription transaction.
*
* This object MUST continue to exist after this call is completed. The application shall wait until it
* receives an OnDone call to destroy the object.
*
* @param[in] apReadClient The read client object that initiated the read transaction.
* @param[in] aSubscriptionId The identifier of the subscription that was established.
*/
virtual void OnSubscriptionEstablished(const ReadClient * apReadClient) {}
virtual void OnSubscriptionEstablished(uint64_t aSubscriptionId) {}

/**
* OnError will be called when an error occurs *after* a successful call to SendRequest(). The following
Expand All @@ -146,10 +140,9 @@ class ReadClient : public Messaging::ExchangeDelegate
* This object MUST continue to exist after this call is completed. The application shall wait until it
* receives an OnDone call to destroy the object.
*
* @param[in] apReadClient The read client object that initiated the attribute read transaction.
* @param[in] aError A system error code that conveys the overall error code.
*/
virtual void OnError(const ReadClient * apReadClient, CHIP_ERROR aError) {}
virtual void OnError(CHIP_ERROR aError) {}

/**
* OnDone will be called when ReadClient has finished all work and is safe to destroy and free the
Expand All @@ -161,9 +154,8 @@ class ReadClient : public Messaging::ExchangeDelegate
* - Only be called after a successful call to SendRequest has been
* made, when the read completes or the subscription is shut down.
*
* @param[in] apReadClient The read client object of the terminated read or subscribe interaction.
*/
virtual void OnDone(ReadClient * apReadClient) = 0;
virtual void OnDone() = 0;
};

enum class InteractionType : uint8_t
Expand Down
12 changes: 6 additions & 6 deletions src/app/tests/TestAttributeCache.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ void DataSeriesGenerator::Generate()
ReadClient::Callback * callback = mReadCallback;
StatusIB status;

callback->OnReportBegin(nullptr);
callback->OnReportBegin();

uint8_t index = 0;
for (auto & instruction : mInstructionList)
Expand Down Expand Up @@ -198,19 +198,19 @@ void DataSeriesGenerator::Generate()
writer.Finalize(&handle);
reader.Init(std::move(handle));
NL_TEST_ASSERT(gSuite, reader.Next() == CHIP_NO_ERROR);
callback->OnAttributeData(nullptr, path, &reader, status);
callback->OnAttributeData(path, &reader, status);
}
else
{
ChipLogProgress(DataManagement, "\t -- Generating Status");
status.mStatus = Protocols::InteractionModel::Status::Failure;
callback->OnAttributeData(nullptr, path, nullptr, status);
callback->OnAttributeData(path, nullptr, status);
}

index++;
}

callback->OnReportEnd(nullptr);
callback->OnReportEnd();
}

class CacheValidator : public AttributeCache::Callback
Expand All @@ -221,7 +221,7 @@ class CacheValidator : public AttributeCache::Callback
Clusters::TestCluster::Attributes::TypeInfo::DecodableType clusterValue;

private:
void OnDone(ReadClient * apReadClient) override {}
void OnDone() override {}
void DecodeAttribute(const AttributeInstruction & instruction, const ConcreteAttributePath & path, AttributeCache * cache)
{
CHIP_ERROR err;
Expand Down Expand Up @@ -417,7 +417,7 @@ class CacheValidator : public AttributeCache::Callback
mExpectedEndpoints.erase(iter);
}

void OnReportEnd(const ReadClient * apReadClient) override
void OnReportEnd() override
{
NL_TEST_ASSERT(gSuite, mExpectedAttributes.size() == 0);
NL_TEST_ASSERT(gSuite, mExpectedClusters.size() == 0);
Expand Down
Loading

0 comments on commit c7aa9a1

Please sign in to comment.