Skip to content
This repository has been archived by the owner on Dec 20, 2023. It is now read-only.

Add an option to allocate logging buffers on heap #519

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add an option to allocate logging buffers on heap #519

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 41 additions & 7 deletions src/adaptations/device-layer/EventLogging.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,21 +35,22 @@ namespace Weave {
namespace DeviceLayer {
namespace Internal {

/* BSS segment is limited on some device, provides an option to allocate large memory buffer on heap */
#ifdef WEAVE_DEVICE_CONFIG_EVENT_LOGGING_BUFFER_STATIC
uint64_t gCritEventBuffer[RoundUp(WEAVE_DEVICE_CONFIG_EVENT_LOGGING_CRIT_BUFFER_SIZE, sizeof(uint64_t)) / sizeof(uint64_t)];
static nl::Weave::PersistedCounter sCritEventIdCounter;

uint64_t gProdEventBuffer[RoundUp(WEAVE_DEVICE_CONFIG_EVENT_LOGGING_PROD_BUFFER_SIZE, sizeof(uint64_t)) / sizeof(uint64_t)];
static nl::Weave::PersistedCounter sProdEventIdCounter;

#if WEAVE_DEVICE_CONFIG_EVENT_LOGGING_INFO_BUFFER_SIZE
uint64_t gInfoEventBuffer[RoundUp(WEAVE_DEVICE_CONFIG_EVENT_LOGGING_INFO_BUFFER_SIZE, sizeof(uint64_t)) / sizeof(uint64_t)];
static nl::Weave::PersistedCounter sInfoEventIdCounter;
#endif

#if WEAVE_DEVICE_CONFIG_EVENT_LOGGING_DEBUG_BUFFER_SIZE
uint64_t gDebugEventBuffer[RoundUp(WEAVE_DEVICE_CONFIG_EVENT_LOGGING_DEBUG_BUFFER_SIZE, sizeof(uint64_t)) / sizeof(uint64_t)];
static nl::Weave::PersistedCounter sDebugEventIdCounter;
#endif
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend preprocessor label comment here.


static nl::Weave::PersistedCounter sCritEventIdCounter;
static nl::Weave::PersistedCounter sProdEventIdCounter;
static nl::Weave::PersistedCounter sInfoEventIdCounter;
static nl::Weave::PersistedCounter sDebugEventIdCounter;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These need to be inside #if / #endif blocks so that they are only defined if the corresponding importance level is enabled.


WEAVE_ERROR InitWeaveEventLogging(void)
{
Expand All @@ -62,6 +63,7 @@ WEAVE_ERROR InitWeaveEventLogging(void)
nl::Weave::Platform::PersistedStorage::Key debugEventIdCounterStorageKey = WEAVE_DEVICE_CONFIG_PERSISTED_STORAGE_DEBUG_EIDC_KEY;
#endif

#ifdef WEAVE_DEVICE_CONFIG_EVENT_LOGGING_BUFFER_STATIC
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to use #if here, rather than #ifdef.

nl::Weave::Profiles::DataManagement::LogStorageResources logStorageResources[] = {
{ static_cast<void *>(&gCritEventBuffer[0]), sizeof(gCritEventBuffer),
&critEventIdCounterStorageKey,
Expand All @@ -88,6 +90,38 @@ WEAVE_ERROR InitWeaveEventLogging(void)
nl::Weave::Profiles::DataManagement::ImportanceType::Debug },
#endif
};
#else
nl::Weave::Profiles::DataManagement::LogStorageResources logStorageResources[] = {
{ static_cast<void *>(malloc(RoundUp(WEAVE_DEVICE_CONFIG_EVENT_LOGGING_CRIT_BUFFER_SIZE, sizeof(uint64_t)))),
RoundUp(WEAVE_DEVICE_CONFIG_EVENT_LOGGING_CRIT_BUFFER_SIZE, sizeof(uint64_t)),
&critEventIdCounterStorageKey,
WEAVE_DEVICE_CONFIG_EVENT_ID_COUNTER_EPOCH,
&sCritEventIdCounter,
nl::Weave::Profiles::DataManagement::ImportanceType::ProductionCritical },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a lot of duplicated code here. I suggest having a common section of code that initializes all fields within LogStorageResources except mBuffer. Then have a section that sets mBuffer dynamically or statically, based on the configuration option.

{ static_cast<void *>(malloc(RoundUp(WEAVE_DEVICE_CONFIG_EVENT_LOGGING_PROD_BUFFER_SIZE, sizeof(uint64_t)))),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than calling malloc directly, I suggest defining a weak function, e.g. AllocEventLogBuffer(), that takes the importance type and size, and returns a pointer to a buffer. The default implementation of this function would simply call malloc. This way application developers can override the use of malloc if needed on their devices.

#if !WEAVE_DEVICE_CONFIG_EVENT_LOGGING_BUFFER_STATIC

void * __attribute__((weak)) AllocEventLogBuffer(nl::Weave::Profiles::DataManagement::ImportanceType importance, size_t size)
{
    return malloc(size);
}

#endif // !WEAVE_DEVICE_CONFIG_EVENT_LOGGING_BUFFER_STATIC

I think this will address Grant's comment.

RoundUp(WEAVE_DEVICE_CONFIG_EVENT_LOGGING_PROD_BUFFER_SIZE, sizeof(uint64_t)),
&prodEventIdCounterStorageKey,
WEAVE_DEVICE_CONFIG_EVENT_ID_COUNTER_EPOCH,
&sProdEventIdCounter,
nl::Weave::Profiles::DataManagement::ImportanceType::Production },
#if WEAVE_DEVICE_CONFIG_EVENT_LOGGING_INFO_BUFFER_SIZE
{ static_cast<void *>(malloc(RoundUp(WEAVE_DEVICE_CONFIG_EVENT_LOGGING_INFO_BUFFER_SIZE, sizeof(uint64_t)))),
RoundUp(WEAVE_DEVICE_CONFIG_EVENT_LOGGING_INFO_BUFFER_SIZE, sizeof(uint64_t)),
&infoEventIdCounterStorageKey,
WEAVE_DEVICE_CONFIG_EVENT_ID_COUNTER_EPOCH,
&sInfoEventIdCounter,
nl::Weave::Profiles::DataManagement::ImportanceType::Info },
#endif
#if WEAVE_DEVICE_CONFIG_EVENT_LOGGING_DEBUG_BUFFER_SIZE
{ static_cast<void *>(malloc(RoundUp(WEAVE_DEVICE_CONFIG_EVENT_LOGGING_DEBUG_BUFFER_SIZE, sizeof(uint64_t)))),
RoundUp(WEAVE_DEVICE_CONFIG_EVENT_LOGGING_DEBUG_BUFFER_SIZE, sizeof(uint64_t)),
&debugEventIdCounterStorageKey,
WEAVE_DEVICE_CONFIG_EVENT_ID_COUNTER_EPOCH,
&sDebugEventIdCounter,
nl::Weave::Profiles::DataManagement::ImportanceType::Debug },
#endif
};
#endif

nl::Weave::Profiles::DataManagement::LoggingManagement::CreateLoggingManagement(
&nl::Weave::DeviceLayer::ExchangeMgr,
Expand Down
11 changes: 11 additions & 0 deletions src/adaptations/device-layer/include/Weave/DeviceLayer/WeaveDeviceConfig.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -744,6 +744,17 @@
#endif

// -------------------- Event Logging Configuration --------------------
/**
* @def WEAVE_DEVICE_CONFIG_EVENT_LOGGING_BUFFER_STATIC
*
* @brief
* Allocate logging buffer on data segment if defined, or allocate on
* heap dynamically if not defined
*/
#ifndef WEAVE_DEVICE_CONFIG_EVENT_LOGGING_BUFFER_STATIC
#define WEAVE_DEVICE_CONFIG_EVENT_LOGGING_BUFFER_STATIC 1
#endif


/**
* @def WEAVE_DEVICE_CONFIG_EVENT_LOGGING_CRIT_BUFFER_SIZE
Expand Down