Skip to content

Commit

Permalink
address comments
Browse files Browse the repository at this point in the history
  • Loading branch information
yunhanw-google committed Apr 30, 2021
1 parent b590c55 commit 1633218
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 66 deletions.
72 changes: 26 additions & 46 deletions src/app/EventManagement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,8 @@ struct EventEnvelopeContext

size_t mNumFieldsToRead =
2 /* PriorityLevel and DeltaSystemTimestamp are there if that is not first event when putting events in report*/;
uint32_t mDeltaTime = 0;
int64_t mDeltaUtc = 0;
Timestamp mDeltaSystemTime = Timestamp::UTC(0);
Timestamp mDeltaUtc = Timestamp::System(0);
PriorityLevel mPriority = PriorityLevel::First;
};

Expand All @@ -126,38 +126,24 @@ void EventManagement::Init(Messaging::ExchangeManager * apExchangeManager, int a
next = (bufferIndex < aNumBuffers - 1) ? &apCircularEventBuffer[bufferIndex + 1] : nullptr;

current = &apCircularEventBuffer[bufferIndex];
current->Init(static_cast<uint8_t *>(apLogStorageResources[bufferIndex].mpBuffer),
(uint32_t)(apLogStorageResources[bufferIndex].mBufferSize), prev, next);
current->Init(apLogStorageResources[bufferIndex].mpBuffer, apLogStorageResources[bufferIndex].mBufferSize, prev, next);

prev = current;

current->mProcessEvictedElement = AlwaysFail;
current->mAppData = nullptr;
current->mPriority = apLogStorageResources[bufferIndex].mPriority;
if ((apLogStorageResources[bufferIndex].mpCounterStorage != nullptr) &&
(apLogStorageResources[bufferIndex].mCounterKey != nullptr) && (apLogStorageResources[bufferIndex].mCounterEpoch != 0))
{
// We have been provided storage for a counter for this priority level.
CHIP_ERROR err = apLogStorageResources[bufferIndex].mpCounterStorage->Init(
*(apLogStorageResources[bufferIndex].mCounterKey), apLogStorageResources[bufferIndex].mCounterEpoch);
if (err != CHIP_NO_ERROR)
{
ChipLogError(EventLogging, "PersistedCounter[%d]->Init() failed with %d", bufferIndex, err);
}
current->mpEventNumberCounter = apLogStorageResources[bufferIndex].mpCounterStorage;
}
else
current->mpEventNumberCounter = apLogStorageResources[bufferIndex].InitializeCounter();

if (current->mpEventNumberCounter == nullptr)
{
// No counter has been provided, so we'll use our
// "built-in" non-persisted counter.
current->mNonPersistedCounter.Init(1);
current->mpEventNumberCounter = &(current->mNonPersistedCounter);
}

current->mFirstEventNumber = static_cast<chip::EventNumber>(current->mpEventNumberCounter->GetValue());
}

mpEventBuffer = &apCircularEventBuffer[0];
mpEventBuffer = apCircularEventBuffer;
mState = EventManagementStates::Idle;
mBytesWritten = 0;
}
Expand Down Expand Up @@ -196,7 +182,7 @@ CHIP_ERROR EventManagement::CopyToNextBuffer(CircularEventBuffer * apEventBuffer
return err;
}

CHIP_ERROR EventManagement::EnsureSpace(size_t aRequiredSpace)
CHIP_ERROR EventManagement::EnsureSpaceInCircularBuffer(size_t aRequiredSpace)
{
CHIP_ERROR err = CHIP_NO_ERROR;
size_t requiredSpace = aRequiredSpace;
Expand Down Expand Up @@ -255,11 +241,11 @@ CHIP_ERROR EventManagement::EnsureSpace(size_t aRequiredSpace)
eventBuffer->mAppData = reinterpret_cast<void *>(requiredSpace);
eventBuffer = eventBuffer->mpNext;

// Sanity check: Die here on null event buffer. If
// Sanity check: reuten error here on null event buffer. If
// eventBuffer->mpNext were null, then the `EvictBuffer`
// in line 130 would have succeeded -- the event was
// would have succeeded -- the event was
// already in the final buffer.
VerifyOrDie(eventBuffer != nullptr);
VerifyOrExit(eventBuffer != nullptr, err = CHIP_ERROR_INCORRECT_STATE);

requiredSpace = ctx.mSpaceNeededForEvent;
}
Expand All @@ -279,10 +265,11 @@ CHIP_ERROR EventManagement::EnsureSpace(size_t aRequiredSpace)
mpEventBuffer->mAppData = nullptr;

exit:
ChipLogFunctError(err);
return err;
}

CHIP_ERROR EventManagement::BlitEvent(EventLoadOutContext * apContext, EventLoggingDelegate * apDelegate,
CHIP_ERROR EventManagement::ConstructEvent(EventLoadOutContext * apContext, EventLoggingDelegate * apDelegate,
const EventOptions * apOptions)
{

Expand All @@ -300,15 +287,17 @@ CHIP_ERROR EventManagement::BlitEvent(EventLoadOutContext * apContext, EventLogg

eventDataElementBuilder.Init(&(apContext->mWriter));
eventPathBuilder = eventDataElementBuilder.CreateEventPathBuilder();
SuccessOrExit(eventPathBuilder.GetError());
err = eventPathBuilder.GetError();
SuccessOrExit(err);

// TODO: Revisit NodeId since the the encoding spec and the IM seem to disagree on how this stuff works
eventPathBuilder.NodeId(apOptions->mpEventSchema->mNodeId)
.EndpointId(apOptions->mpEventSchema->mEndpointId)
.ClusterId(apOptions->mpEventSchema->mClusterId)
.EventId(apOptions->mpEventSchema->mEventId)
.EndOfEventPath();
SuccessOrExit(eventPathBuilder.GetError());
err = eventPathBuilder.GetError();
SuccessOrExit(err);

eventDataElementBuilder.PriorityLevel(static_cast<uint8_t>(apContext->mPriority));

Expand All @@ -330,8 +319,8 @@ CHIP_ERROR EventManagement::BlitEvent(EventLoadOutContext * apContext, EventLogg
eventDataElementBuilder.DeltaSystemTimestamp(deltatime);
}
}

SuccessOrExit(eventDataElementBuilder.GetError());
err = eventDataElementBuilder.GetError()
SuccessOrExit(err);

err = apContext->mWriter.StartContainer(ContextTag(chip::app::EventDataElement::kCsTag_Data), chip::TLV::kTLVType_Structure,
dataContainerType);
Expand All @@ -349,11 +338,7 @@ CHIP_ERROR EventManagement::BlitEvent(EventLoadOutContext * apContext, EventLogg
err = apContext->mWriter.Finalize();
SuccessOrExit(err);

// only update mFirst if an event was successfully written.
if (apContext->mFirst)
{
apContext->mFirst = false;
}
apContext->mFirst = false;

exit:
if (err != CHIP_NO_ERROR)
Expand All @@ -362,7 +347,7 @@ CHIP_ERROR EventManagement::BlitEvent(EventLoadOutContext * apContext, EventLogg
}
else
{
// update these variables since BlitEvent can be used to track the
// update these variables since ConstructEvent can be used to track the
// state of a set of events over multiple calls.
apContext->mCurrentEventNumber++;
if (apContext->mCurrentSystemTime.mType == Timestamp::Type::kSystem)
Expand Down Expand Up @@ -532,12 +517,7 @@ CHIP_ERROR EventManagement::LogEventPrivate(EventLoggingDelegate * apDelegate, c
while (!didWriteEvent)
{
// Ensure we have space in the in-memory logging queues
err = EnsureSpace(requestSize);
// If we fail to ensure the initial reserve size, then the
// logging subsystem will never be able to make progress. In
// that case, it is best to assert
if ((requestSize == kEventSizeRezerve) && (err != CHIP_NO_ERROR))
chipDie();
err = EnsureSpaceInCircularBuffer(requestSize);
SuccessOrExit(err);

// save a checkpoint for the underlying buffer. Note that with
Expand All @@ -549,7 +529,7 @@ CHIP_ERROR EventManagement::LogEventPrivate(EventLoggingDelegate * apDelegate, c
// Start the event container (anonymous structure) in the circular buffer
writer.Init(*mpEventBuffer);

err = BlitEvent(&ctxt, apDelegate, &opts);
err = ConstructEvent(&ctxt, apDelegate, &opts);

if (err == CHIP_ERROR_NO_MEMORY)
{
Expand Down Expand Up @@ -657,7 +637,7 @@ CHIP_ERROR EventManagement::EventIterator(const TLVReader & aReader, size_t aDep

if (event.mPriority == loadOutContext->mPriority)
{
loadOutContext->mCurrentSystemTime.mValue += event.mDeltaTime;
loadOutContext->mCurrentSystemTime.mValue += event.mDeltaSystemTime.mValue;
VerifyOrExit(loadOutContext->mCurrentEventNumber < loadOutContext->mStartingEventNumber, err = CHIP_EVENT_ID_FOUND);
loadOutContext->mCurrentEventNumber++;
}
Expand Down Expand Up @@ -759,7 +739,7 @@ CHIP_ERROR EventManagement::FetchEventParameters(const TLVReader & aReader, size

if (reader.GetTag() == chip::TLV::ContextTag(chip::app::EventDataElement::kCsTag_DeltaSystemTimestamp))
{
err = reader.Get(envelope->mDeltaTime);
err = reader.Get(envelope->mDeltaSystemTime.mValue);
SuccessOrExit(err);

envelope->mNumFieldsToRead--;
Expand Down Expand Up @@ -800,7 +780,7 @@ CHIP_ERROR EventManagement::EvictEvent(CHIPCircularTLVBuffer & apBuffer, void *
uint16_t numEventsToDrop = 1;

eventBuffer->RemoveEvent(numEventsToDrop);
eventBuffer->mFirstEventSystemTimestamp.mValue += context.mDeltaTime;
eventBuffer->mFirstEventSystemTimestamp.mValue += context.mDeltaSystemTime.mValue;
ChipLogProgress(EventLogging,
"Dropped events from buffer with priority %d due to overflow: { event priority_level: %d, count: %d };",
eventBuffer->mPriority, imp, numEventsToDrop);
Expand Down
37 changes: 17 additions & 20 deletions src/app/EventManagement.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,18 +51,6 @@ class CircularEventBuffer : public TLV::CHIPCircularTLVBuffer
/**
* @brief
* A constructor for the CircularEventBuffer (internal API).
*
* @param[in] apBuffer The actual storage to use for event storage.
*
* @param[in] aBufferLength The length of the \c apBuffer in bytes.
*
* @param[in] apPrev The pointer to CircularEventBuffer storing
* events of lesser priority.
*
* @param[in] apNext The pointer to CircularEventBuffer storing
* events of greater priority.
*
* @return CircularEventBuffer
*/
CircularEventBuffer();

Expand Down Expand Up @@ -189,19 +177,28 @@ enum class EventManagementStates

struct LogStorageResources
{
void * mpBuffer =
//TODO: Update CHIPCircularTLVBuffer with size_t for buffer size, then use ByteSpan
uint8_t * mpBuffer =
nullptr; // Buffer to be used as a storage at the particular priority level and shared with more important events.
// Must not be nullptr. Must be large enough to accommodate the largest event emitted by the system.
size_t mBufferSize = 0; //< The size, in bytes, of the `mBuffer`.
chip::Platform::PersistedStorage::Key * mCounterKey =
uint32_t mBufferSize = 0; //< The size, in bytes, of the `mBuffer`.
Platform::PersistedStorage::Key * mCounterKey =
nullptr; // Name of the key naming persistent counter for events of this priority. When NULL, the persistent
// counters will not be used for this priority level.
uint32_t mCounterEpoch = 0; // The interval used in incrementing persistent counters. When 0, the persistent counters will not
// be used for this priority level.
chip::PersistedCounter * mpCounterStorage =
PersistedCounter * mpCounterStorage =
nullptr; // application provided storage for persistent counter for this priority level.
PriorityLevel mPriority =
PriorityLevel::Invalid; // Log priority level associated with the resources provided in this structure.
PersistedCounter * InitializeCounter() const
{
if (mpCounterStorage != nullptr && mCounterKey != nullptr && mCounterEpoch != 0)
{
return (mpCounterStorage->Init(*mCounterKey, mCounterEpoch) != CHIP_NO_ERROR) ? mpCounterStorage : nullptr;
}
return nullptr;
}
};

/**
Expand Down Expand Up @@ -437,23 +434,23 @@ class EventManagement
*
* @param[inout] apContext EventLoadOutContext, initialized with stateful
* information for the buffer. State is updated
* and preserved by BlitEvent using this context.
* and preserved by ConstructEvent using this context.
*
* @param[in] apDelegate The EventLoggingDelegate to serialize the event data
*
* @param[in] apOptions EventOptions describing timestamp and other tags
* relevant to this event.
*
*/
CHIP_ERROR BlitEvent(EventLoadOutContext * apContext, EventLoggingDelegate * apDelegate, const EventOptions * apOptions);
CHIP_ERROR ConstructEvent(EventLoadOutContext * apContext, EventLoggingDelegate * apDelegate, const EventOptions * apOptions);

/**
* @brief Helper function to skip writing an event corresponding to an allocated
* event id.
*
* @param[inout] apContext EventLoadOutContext, initialized with stateful
* information for the buffer. State is updated
* and preserved by BlitEvent using this context.
* and preserved by ConstructEvent using this context.
*
*/
void SkipEvent(EventLoadOutContext * apContext);
Expand All @@ -477,7 +474,7 @@ class EventManagement
* @param[in] aRequiredSpace require space
*
*/
CHIP_ERROR EnsureSpace(size_t aRequiredSpace);
CHIP_ERROR EnsureSpaceInCircularBuffer(size_t aRequiredSpace);

/**
* @brief
Expand Down

0 comments on commit 1633218

Please sign in to comment.