Skip to content

Commit

Permalink
Addressed comments except the Matter threading model ones
Browse files Browse the repository at this point in the history
  • Loading branch information
@nivi-apple
nivi-apple committed Dec 6, 2023
1 parent 5a37be1 commit 34766b0
Show file tree
Hide file tree
Showing 11 changed files with 168 additions and 137 deletions.
34 changes: 17 additions & 17 deletions examples/all-clusters-app/linux/AppOptions.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,13 @@ constexpr uint16_t kOptionCrashFilePath = 0xFF05;

static chip::Credentials::Examples::TestHarnessDACProvider mDacProvider;

chip::Optional<std::string> mEndUserSupportLogFilePath;
chip::Optional<std::string> mNetworkDiagnosticsLogFilePath;
chip::Optional<std::string> mCrashLogFilePath;
static std::optional<std::string> sEndUserSupportLogFilePath;
static std::optional<std::string> sNetworkDiagnosticsLogFilePath;
static std::optional<std::string> sCrashLogFilePath;

bool AppOptions::IsNullOrEmpty(const char * value)
bool AppOptions::IsNull(const char * value)
{
return (value == nullptr || strlen(value) == 0 || strncmp(value, "", strlen(value)) == 0);
return (value == nullptr || strlen(value) == 0);
}

bool AppOptions::HandleOptions(const char * program, OptionSet * options, int identifier, const char * name, const char * value)
Expand All @@ -61,23 +61,23 @@ bool AppOptions::HandleOptions(const char * program, OptionSet * options, int id
break;
}
case kOptionEndUserSupportFilePath: {
if (!IsNullOrEmpty(value))
if (!IsNull(value))
{
mEndUserSupportLogFilePath.SetValue(std::string{ value });
sEndUserSupportLogFilePath = std::string{ value };
}
break;
}
case kOptionNetworkDiagnosticsFilePath: {
if (!IsNullOrEmpty(value))
if (!IsNull(value))
{
mNetworkDiagnosticsLogFilePath.SetValue(std::string{ value });
sNetworkDiagnosticsLogFilePath = std::string{ value };
}
break;
}
case kOptionCrashFilePath: {
if (!IsNullOrEmpty(value))
if (!IsNull(value))
{
mCrashLogFilePath.SetValue(std::string{ value });
sCrashLogFilePath = std::string{ value };
}
break;
}
Expand Down Expand Up @@ -123,17 +123,17 @@ chip::Credentials::DeviceAttestationCredentialsProvider * AppOptions::GetDACProv
return &mDacProvider;
}

Optional<std::string> AppOptions::GetEndUserSupportLogFilePath()
std::optional<std::string> AppOptions::GetEndUserSupportLogFilePath()
{
return mEndUserSupportLogFilePath;
return sEndUserSupportLogFilePath;
}

Optional<std::string> AppOptions::GetNetworkDiagnosticsLogFilePath()
std::optional<std::string> AppOptions::GetNetworkDiagnosticsLogFilePath()
{
return mNetworkDiagnosticsLogFilePath;
return sNetworkDiagnosticsLogFilePath;
}

Optional<std::string> AppOptions::GetCrashLogFilePath()
std::optional<std::string> AppOptions::GetCrashLogFilePath()
{
return mCrashLogFilePath;
return sCrashLogFilePath;
}
8 changes: 4 additions & 4 deletions examples/all-clusters-app/linux/AppOptions.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,13 @@ class AppOptions
public:
static chip::ArgParser::OptionSet * GetOptions();
static chip::Credentials::DeviceAttestationCredentialsProvider * GetDACProvider();
static chip::Optional<std::string> GetEndUserSupportLogFilePath();
static chip::Optional<std::string> GetNetworkDiagnosticsLogFilePath();
static chip::Optional<std::string> GetCrashLogFilePath();
static std::optional<std::string> GetEndUserSupportLogFilePath();
static std::optional<std::string> GetNetworkDiagnosticsLogFilePath();
static std::optional<std::string> GetCrashLogFilePath();

private:
static bool HandleOptions(const char * program, chip::ArgParser::OptionSet * options, int identifier, const char * name,
const char * value);

static bool IsNullOrEmpty(const char * value);
static bool IsNull(const char * value);
};
46 changes: 20 additions & 26 deletions examples/all-clusters-app/linux/diagnostic-logs-provider-delegate-impl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,13 @@ LogSessionHandle LogProvider::StartLogCollection(IntentEnum logType)
return kInvalidLogSessionHandle;
}

mIsInALogCollectionSession = true;
mTotalNumberOfBytesConsumed = 0;

// Open the log file for reading.
Optional<std::string> filePath = GetLogFilePath(logType);
if (filePath.HasValue())
std::optional<std::string> filePath = GetLogFilePath(logType);
if (filePath.has_value())
{
mFileStream.open(filePath.Value().c_str(), std::ios_base::binary | std::ios_base::in);
mFileStream.open(filePath.value().c_str(), std::ios_base::binary | std::ios_base::in);
if (!mFileStream.good())
{
ChipLogError(BDX, "Failed to open the log file");
Expand All @@ -64,6 +63,7 @@ LogSessionHandle LogProvider::StartLogCollection(IntentEnum logType)
{
mLogSessionHandle = kInvalidLogSessionHandle;
}
mIsInALogCollectionSession = true;
return mLogSessionHandle;
}

Expand All @@ -79,21 +79,20 @@ CHIP_ERROR LogProvider::GetNextChunk(LogSessionHandle logSessionHandle, MutableB
ChipLogError(BDX, "File is not open");
return CHIP_ERROR_INCORRECT_STATE;
}

mFileStream.seekg(mFileStream.end);
long long fileSize = mFileStream.tellg();


mFileStream.seekg(0, mFileStream.end);
auto fileSize = mFileStream.tellg();

mFileStream.seekg(0, mFileStream.beg);

long long remainingBytesToBeRead = fileSize - static_cast<long long>(mTotalNumberOfBytesConsumed);

if ((remainingBytesToBeRead >= kMaxLogContentSize && outBuffer.size() < kMaxLogContentSize) ||
(remainingBytesToBeRead < kMaxLogContentSize && static_cast<long long>(outBuffer.size()) < remainingBytesToBeRead))
{
ChipLogError(BDX, "Buffer is too small to read the next chunk from file");
return CHIP_ERROR_INVALID_ARGUMENT;
}

long long bufferSize = static_cast< long long>(outBuffer.size());

long long bytesToRead = (remainingBytesToBeRead < bufferSize) ? remainingBytesToBeRead : bufferSize;

mFileStream.seekg(static_cast<long long>(mTotalNumberOfBytesConsumed));
mFileStream.read(reinterpret_cast<char *>(outBuffer.data()), kMaxLogContentSize);
mFileStream.read(reinterpret_cast<char *>(outBuffer.data()), bytesToRead);

if (!(mFileStream.good() || mFileStream.eof()))
{
Expand All @@ -119,12 +118,7 @@ void LogProvider::EndLogCollection(LogSessionHandle logSessionHandle)
mIsInALogCollectionSession = false;
}

uint64_t LogProvider::GetTotalNumberOfBytesConsumed(LogSessionHandle logSessionHandle)
{
return mTotalNumberOfBytesConsumed;
}

Optional<std::string> LogProvider::GetLogFilePath(IntentEnum logType)
std::optional<std::string> LogProvider::GetLogFilePath(IntentEnum logType)
{
switch (logType)
{
Expand All @@ -135,21 +129,21 @@ Optional<std::string> LogProvider::GetLogFilePath(IntentEnum logType)
case IntentEnum::kCrashLogs:
return mCrashLogFilePath;
default:
return NullOptional;
return std::nullopt;
}
}

void LogProvider::SetEndUserSupportLogFilePath(Optional<std::string> logFilePath)
void LogProvider::SetEndUserSupportLogFilePath(std::optional<std::string> logFilePath)
{
mEndUserSupportLogFilePath = logFilePath;
}

void LogProvider::SetNetworkDiagnosticsLogFilePath(Optional<std::string> logFilePath)
void LogProvider::SetNetworkDiagnosticsLogFilePath(std::optional<std::string> logFilePath)
{
mNetworkDiagnosticsLogFilePath = logFilePath;
}

void LogProvider::SetCrashLogFilePath(Optional<std::string> logFilePath)
void LogProvider::SetCrashLogFilePath(std::optional<std::string> logFilePath)
{
mCrashLogFilePath = logFilePath;
}
27 changes: 13 additions & 14 deletions examples/all-clusters-app/linux/include/diagnostic-logs-provider-delegate-impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include <app/clusters/diagnostic-logs-server/diagnostic-logs-server.h>

#include <fstream>
#include <filesystem>

namespace chip {
namespace app {
Expand All @@ -38,19 +39,17 @@ class LogProvider : public LogProviderDelegate
static LogProvider sInstance;

public:
LogSessionHandle StartLogCollection(IntentEnum logType);
LogSessionHandle StartLogCollection(IntentEnum logType) override;

CHIP_ERROR GetNextChunk(LogSessionHandle logSessionHandle, MutableByteSpan & outBuffer, bool & outIsEOF);
CHIP_ERROR GetNextChunk(LogSessionHandle logSessionHandle, MutableByteSpan & outBuffer, bool & outIsEOF) override;

void EndLogCollection(LogSessionHandle logSessionHandle);
void EndLogCollection(LogSessionHandle logSessionHandle) override;

uint64_t GetTotalNumberOfBytesConsumed(LogSessionHandle logSessionHandle);
void SetEndUserSupportLogFilePath(std::optional<std::string> logFilePath);

void SetEndUserSupportLogFilePath(Optional<std::string> logFilePath);
void SetNetworkDiagnosticsLogFilePath(std::optional<std::string> logFilePath);

void SetNetworkDiagnosticsLogFilePath(Optional<std::string> logFilePath);

void SetCrashLogFilePath(Optional<std::string> logFilePath);
void SetCrashLogFilePath(std::optional<std::string> logFilePath);

LogProvider(){};

Expand All @@ -59,17 +58,17 @@ class LogProvider : public LogProviderDelegate
static inline LogProvider & GetInstance() { return sInstance; }

private:
Optional<std::string> GetLogFilePath(IntentEnum logType);
std::optional<std::string> GetLogFilePath(IntentEnum logType);

Optional<std::string> mEndUserSupportLogFilePath;
Optional<std::string> mNetworkDiagnosticsLogFilePath;
Optional<std::string> mCrashLogFilePath;
std::optional<std::string> mEndUserSupportLogFilePath;
std::optional<std::string> mNetworkDiagnosticsLogFilePath;
std::optional<std::string> mCrashLogFilePath;

std::ifstream mFileStream;

LogSessionHandle mLogSessionHandle;
LogSessionHandle mLogSessionHandle = kInvalidLogSessionHandle;

bool mIsInALogCollectionSession;
bool mIsInALogCollectionSession = false;

uint64_t mTotalNumberOfBytesConsumed;
};
Expand Down
8 changes: 4 additions & 4 deletions examples/all-clusters-app/linux/main-common.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -268,9 +268,9 @@ using namespace chip::app::Clusters::DiagnosticLogs;
void emberAfDiagnosticLogsClusterInitCallback(chip::EndpointId endpoint)
{
ChipLogProgress(NotSpecified, "SetLogProviderDelegate");
DiagnosticLogsServer::Instance().SetLogProviderDelegate(endpoint, &LogProvider::getLogProvider());
DiagnosticLogsServer::Instance().SetLogProviderDelegate(endpoint, &LogProvider::GetInstance());

LogProvider::getLogProvider().SetEndUserSupportLogFilePath(AppOptions::GetEndUserSupportLogFilePath());
LogProvider::getLogProvider().SetNetworkDiagnosticsLogFilePath(AppOptions::GetNetworkDiagnosticsLogFilePath());
LogProvider::getLogProvider().SetCrashLogFilePath(AppOptions::GetCrashLogFilePath());
LogProvider::GetInstance().SetEndUserSupportLogFilePath(AppOptions::GetEndUserSupportLogFilePath());
LogProvider::GetInstance().SetNetworkDiagnosticsLogFilePath(AppOptions::GetNetworkDiagnosticsLogFilePath());
LogProvider::GetInstance().SetCrashLogFilePath(AppOptions::GetCrashLogFilePath());
}
51 changes: 23 additions & 28 deletions src/app/bdx/DiagnosticLogsBDXTransferHandler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,16 +55,9 @@ CHIP_ERROR DiagnosticLogsBDXTransferHandler::InitializeTransfer(Messaging::Excha
NodeId peerNodeId, LogProviderDelegate * delegate,
IntentEnum intent, CharSpan fileDesignator)
{
if (mInitialized)
{
// Return busy if we are in a transfer session already with another node.
VerifyOrReturnError(mFabricIndex.Value() == fabricIndex && mPeerNodeId.Value() == peerNodeId, CHIP_ERROR_BUSY);

// Reset stale connection from the same Node if exists.
Reset();
}

VerifyOrReturnError(exchangeMgr != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(fabricIndex != kUndefinedFabricIndex, CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(peerNodeId != kUndefinedNodeId, CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(delegate != nullptr, CHIP_ERROR_INVALID_ARGUMENT);

// Create a new exchange context to use for the BDX transfer session
Expand Down Expand Up @@ -97,6 +90,16 @@ CHIP_ERROR DiagnosticLogsBDXTransferHandler::InitializeTransfer(Messaging::Excha
return CHIP_NO_ERROR;
}


void DiagnosticLogsBDXTransferHandler::ScheduleCleanUp()
{
Reset();
DeviceLayer::SystemLayer().ScheduleLambda([this] {
delete this;
DiagnosticLogsServer::Instance().HandleBDXTransferDone();
});
}

void DiagnosticLogsBDXTransferHandler::HandleBDXError(CHIP_ERROR error)
{
VerifyOrReturn(error != CHIP_NO_ERROR);
Expand All @@ -107,11 +110,11 @@ void DiagnosticLogsBDXTransferHandler::HandleBDXError(CHIP_ERROR error)
{
DiagnosticLogsServer::Instance().SendCommandResponse(StatusEnum::kDenied);
}
Reset();
DeviceLayer::SystemLayer().ScheduleLambda([this] {
delete this;
DiagnosticLogsServer::Instance().HandleBDXTransferDone();
});
// Call Reset to clean up state and schedule an asynchronous delete for the DiagnosticLogsBDXTransferHandler object.
// Since an error occured during BDX, before we delete the DiagnosticLogsBDXTransferHandler object, we need the base class -
// TransferFacilitator to stop polling for messages and clean up. Since the HandleBDXError is called only when BDX fails
// and transfer is stopped, this will not be called more than once for a DiagnosticLogsBDXTransferHandler object.
ScheduleCleanUp();
}

void DiagnosticLogsBDXTransferHandler::HandleTransferSessionOutput(TransferSession::OutputEvent & event)
Expand All @@ -125,11 +128,10 @@ void DiagnosticLogsBDXTransferHandler::HandleTransferSessionOutput(TransferSessi
switch (event.EventType)
{
case TransferSession::OutputEventType::kAckEOFReceived:
Reset();
DeviceLayer::SystemLayer().ScheduleLambda([this] {
delete this;
DiagnosticLogsServer::Instance().HandleBDXTransferDone();
});
// Call Reset to clean up state and schedule an asynchronous delete for the DiagnosticLogsBDXTransferHandler object.
// Since BDX has completed successfully, we need the base class - TransferFacilitator to stop polling for messages and clean up
// before we can delete the sub class. This will also be called once for a DiagnosticLogsBDXTransferHandler object.
ScheduleCleanUp();
break;
case TransferSession::OutputEventType::kStatusReceived:
ChipLogError(BDX, "Got StatusReport %x", to_underlying(event.statusData.statusCode));
Expand Down Expand Up @@ -183,15 +185,8 @@ void DiagnosticLogsBDXTransferHandler::HandleTransferSessionOutput(TransferSessi
}
case TransferSession::OutputEventType::kAckReceived: {
uint16_t blockSize = mTransfer.GetTransferBlockSize();
uint16_t bytesToRead = blockSize;

if (mTransfer.GetTransferLength() > 0 && mNumBytesSent + blockSize > mTransfer.GetTransferLength())
{
// cast should be safe because of condition above
bytesToRead = static_cast<uint16_t>(mTransfer.GetTransferLength() - mNumBytesSent);
}

System::PacketBufferHandle blockBuf = System::PacketBufferHandle::New(bytesToRead);
System::PacketBufferHandle blockBuf = System::PacketBufferHandle::New(blockSize);
if (blockBuf.IsNull())
{
mTransfer.AbortTransfer(GetBdxStatusCodeFromChipError(CHIP_ERROR_NO_MEMORY));
Expand All @@ -200,7 +195,7 @@ void DiagnosticLogsBDXTransferHandler::HandleTransferSessionOutput(TransferSessi

MutableByteSpan buffer;

buffer = MutableByteSpan(blockBuf->Start(), bytesToRead);
buffer = MutableByteSpan(blockBuf->Start(), blockSize);

bool isEOF = false;

Expand Down
Loading

0 comments on commit 34766b0

Please sign in to comment.