Add an option to allocate logging buffers on heap #519
Conversation
There is limit data segment on some FreeRTOS platform, because the logging buffer is very large, we may want to move logging buffers from static data segment to dynamic allocated heap. The patch introduced an new define option: WEAVE_DEVICE_CONFIG_EVENT_LOGGING_BUFFER_STATIC When it is defined, the logging buffer will be allocated statically on data segment; when it is not defined, the logging buffers will be allocated dynamically on heap. Signed-off-by: Zang MingJie <mingjiez@google.com>
| #endif | ||
| #endif |
There was a problem hiding this comment.
Recommend preprocessor label comment here.
There was a problem hiding this comment.
Generally, Weave tends to avoid direct calls to malloc or free and, instead, delegates those calls out to a device- or platform-specific allocator wrapper that provides the ability for platforms to use malloc or free or to make context-specific pool allocators.
I would recommend using those facilities here.
| @@ -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 | |||
There was a problem hiding this comment.
You need to use #if here, rather than #ifdef.
| &critEventIdCounterStorageKey, | ||
| WEAVE_DEVICE_CONFIG_EVENT_ID_COUNTER_EPOCH, | ||
| &sCritEventIdCounter, | ||
| nl::Weave::Profiles::DataManagement::ImportanceType::ProductionCritical }, |
There was a problem hiding this comment.
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.
| WEAVE_DEVICE_CONFIG_EVENT_ID_COUNTER_EPOCH, | ||
| &sCritEventIdCounter, | ||
| nl::Weave::Profiles::DataManagement::ImportanceType::ProductionCritical }, | ||
| { static_cast<void *>(malloc(RoundUp(WEAVE_DEVICE_CONFIG_EVENT_LOGGING_PROD_BUFFER_SIZE, sizeof(uint64_t)))), |
There was a problem hiding this comment.
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.
| static nl::Weave::PersistedCounter sCritEventIdCounter; | ||
| static nl::Weave::PersistedCounter sProdEventIdCounter; | ||
| static nl::Weave::PersistedCounter sInfoEventIdCounter; | ||
| static nl::Weave::PersistedCounter sDebugEventIdCounter; |
There was a problem hiding this comment.
These need to be inside #if / #endif blocks so that they are only defined if the corresponding importance level is enabled.
There is limit data segment on some FreeRTOS platform, because the
logging buffer is very large, we may want to move logging buffers
from static data segment to dynamic allocated heap.
The patch introduced an new define option:
WEAVE_DEVICE_CONFIG_EVENT_LOGGING_BUFFER_STATIC
When it is defined, the logging buffer will be allocated statically
on data segment; when it is not defined, the logging buffers will be
allocated dynamically on heap.
Signed-off-by: Zang MingJie mingjiez@google.com