Move logging to util #91
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR consolidates system-level and platform-level logging into a unified util logging module that uses a circular double-buffer, introduces explicit initialization (LOG_init) and deferred log flushing (LOG_task), replaces the legacy logging_ll implementation across code and tests, and updates build configurations to remove the old logging library. Sequence diagram for log message flow with deferred outputsequenceDiagram
participant App as Application
participant LOG as LOG_Handle (util)
participant CB as CircularBuffer
participant Stdout as stdout
App->>LOG: LOG_init()
App->>LOG: LOG_write(level, format, ...)
LOG->>CB: circular_buffer_write(entry)
App->>LOG: LOG_task()
loop Up to N entries
LOG->>CB: circular_buffer_read(entry)
LOG->>Stdout: printf(entry)
end
LOG->>Stdout: fflush(stdout)
Entity relationship diagram for logging buffer and entrieserDiagram
LOG_HANDLE ||--o{ LOG_ENTRY : contains
LOG_HANDLE {
uint8 buffer_data[LOG_BUFFER_SIZE]
bool initialized
}
LOG_ENTRY {
LOG_Level level
uint16 length
char message[LOG_MAX_MESSAGE_SIZE]
}
Class diagram for the new unified logging systemclassDiagram
class LOG_Handle {
+CircularBuffer buffer
+uint8_t buffer_data[LOG_BUFFER_SIZE]
+bool initialized
}
class LOG_Entry {
+LOG_Level level
+uint16_t length
+char message[LOG_MAX_MESSAGE_SIZE]
}
class LOG_Level {
<<enum>>
LOG_LEVEL_ERROR
LOG_LEVEL_WARN
LOG_LEVEL_INFO
LOG_LEVEL_DEBUG
}
class logging_h {
+LOG_Handle *LOG_init()
+int LOG_write(LOG_Level level, char const *format, ...)
+size_t LOG_available()
+bool LOG_read_entry(LOG_Entry *entry)
+void LOG_task()
}
LOG_Handle o-- CircularBuffer
logging_h ..> LOG_Handle
logging_h ..> LOG_Entry
logging_h ..> LOG_Level
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @bessman - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/util/logging.c:178` </location>
<code_context>
- }
-
- /* Validate length */
- if (entry->length >= sizeof(entry->message)) {
- entry->length = sizeof(entry->message) - 1;
- }
</code_context>
<issue_to_address>
Truncating entry->length may lead to inconsistent log entry parsing.
Validating entry->length before reading the message is safer. If the length is invalid, consider discarding the entry instead of truncating, to avoid misinterpreting log data.
</issue_to_address>
### Comment 2
<location> `src/util/logging.c:223` </location>
<code_context>
+ /* Process available log entries */
+ while (processed < MAX_ENTRIES_PER_CALL && read_entry(&entry)) {
+ /* Format and output the message to stdout */
+ printf("[%s] %s\r\n", g_LEVEL_NAMES[entry.level], entry.message);
+ processed++;
+ }
</code_context>
<issue_to_address>
No bounds checking on entry.level when indexing g_LEVEL_NAMES.
Validating entry.level before indexing will prevent potential undefined behavior if it is out of range.
</issue_to_address>
### Comment 3
<location> `src/util/logging.h:125` </location>
<code_context>
- } while (0)
-
-// Convenience macros for common patterns
-#define LOG_INIT(subsystem) LOG_INFO("Initializing " subsystem)
-#define LOG_DEINIT(subsystem) LOG_INFO("Deinitializing " subsystem)
-#define LOG_FUNCTION_ENTRY() LOG_DEBUG("Entering %s", __func__)
</code_context>
<issue_to_address>
String concatenation in macro may not behave as expected.
String literal concatenation in LOG_INIT only works with string literals, not variables or macros. Use a format string, e.g., LOG_INFO("Initializing %s", subsystem), for broader compatibility.
</issue_to_address>
### Comment 4
<location> `tests/test_logging.c:6` </location>
<code_context>
#include "util.h"
#include <string.h>
+#include "logging.c" // Include the implementation for testing
+
void setUp(void)
</code_context>
<issue_to_address>
Directly including the implementation file in the test.
Testing by including the implementation file can cause symbol redefinition and linking issues. Prefer testing through the public header and linking against the compiled object. For internal functions, use a test-only header or friend test pattern.
Suggested implementation:
```c
#include "logging.h" // Include the public header for logging
```
- Make sure your build system (e.g., Makefile or CMakeLists.txt) compiles `logging.c` into an object file and links it with your test executable.
- If you need to test internal (static) functions, consider exposing them via a test-only header or using the "friend test" pattern.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai summary |
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @bessman - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Truncation logic may not always null-terminate the message. (link)
General comments:
- Tests currently poke at the internal LOG_Handle and reset its buffer directly—consider adding a public reset or inspection API so tests can use only the public interface and maintain encapsulation.
- LOG_init always reinitializes the global buffer and returns the same handle—consider making it idempotent or guarding against repeated calls to avoid unexpected state resets.
- LOG_task unconditionally flushes stdout when entries are processed, which could block in tight loops—consider making the flush behavior configurable or deferred to avoid performance impact.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Tests currently poke at the internal LOG_Handle and reset its buffer directly—consider adding a public reset or inspection API so tests can use only the public interface and maintain encapsulation.
- LOG_init always reinitializes the global buffer and returns the same handle—consider making it idempotent or guarding against repeated calls to avoid unexpected state resets.
- LOG_task unconditionally flushes stdout when entries are processed, which could block in tight loops—consider making the flush behavior configurable or deferred to avoid performance impact.
## Individual Comments
### Comment 1
<location> `src/util/logging.c:87` </location>
<code_context>
+ }
+
+ /* Truncate if necessary */
+ if (content_len >= (int)sizeof(entry.message)) {
+ content_len = sizeof(entry.message) - 1;
+ }
</code_context>
<issue_to_address>
Truncation logic may not always null-terminate the message.
Setting entry.message[content_len] = '\0' risks writing out of bounds. To ensure safe null-termination, set entry.message[sizeof(entry.message) - 1] = '\0' after vsnprintf.
</issue_to_address>
### Comment 2
<location> `src/util/logging.c:157` </location>
<code_context>
+ }
+
+ /* Check if we have enough data for the message */
+ if (circular_buffer_available(&g_log_handle.buffer) < entry->length + 1) {
+ return false;
+ }
</code_context>
<issue_to_address>
Potential for buffer underflow if entry->length is corrupted.
Add a check to ensure entry->length does not exceed LOG_MAX_MESSAGE_SIZE to prevent reading beyond the buffer if entry->length is invalid.
</issue_to_address>
### Comment 3
<location> `tests/test_logging.c:197` </location>
<code_context>
}
-void test_LOG_LL_read_entry_null_pointer(void)
+void test_LOG_LOG_read_entry_null_pointer(void)
{
// Arrange
- int bytes_written = LOG_LL_write(LOG_LL_INFO, "Test message");
+ int bytes_written = LOG_write(LOG_LEVEL_INFO, "Test message");
TEST_ASSERT_GREATER_THAN(0, bytes_written);
// Act & Assert
- bool result = LOG_LL_read_entry(NULL);
+ bool result = LOG_read_entry(NULL);
TEST_ASSERT_FALSE(result);
}
</code_context>
<issue_to_address>
Consider adding a test for LOG_write with a NULL format string.
Please add a test for LOG_write with a NULL format string to ensure it returns -1 and does not crash.
</issue_to_address>
### Comment 4
<location> `tests/test_logging.c:37` </location>
<code_context>
}
-void test_LOG_LL_write(void)
+void test_LOG_write(void)
{
// Arrange
char const *test_message = "Test log message";
// Act
- int bytes_written = LOG_LL_write(LOG_LL_DEBUG, test_message);
+ int bytes_written = LOG_write(LOG_LEVEL_DEBUG, test_message);
// Assert
- LOG_LL_Entry entry;
+ LOG_Entry entry;
circular_buffer_read(
- &g_LOG_LL_buffer, (uint8_t *)&entry, sizeof(entry)
+ &g_log_handle->buffer, (uint8_t *)&entry, sizeof(entry)
);
- TEST_ASSERT_EQUAL(LOG_LL_DEBUG, entry.level);
+ TEST_ASSERT_EQUAL(LOG_LEVEL_DEBUG, entry.level);
TEST_ASSERT_EQUAL_STRING(entry.message, test_message);
TEST_ASSERT_EQUAL(strlen(test_message), entry.length);
TEST_ASSERT_EQUAL(
</code_context>
<issue_to_address>
Add a test for LOG_write with an invalid log level.
Please add a test to verify that LOG_write returns -1 and does not write to the buffer when given an invalid log level.
</issue_to_address>
### Comment 5
<location> `tests/test_logging.c:373` </location>
<code_context>
+void test_LOG_buffer_wraparound_detection(void)
</code_context>
<issue_to_address>
Consider adding a test for LOG_init called multiple times.
Add a test to ensure repeated calls to LOG_init do not corrupt state and handle multiple initializations correctly.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
// Test buffer wraparound detection
void test_LOG_buffer_wraparound_detection(void)
{
=======
/**
* Test that calling LOG_init multiple times does not corrupt state.
*/
void test_LOG_init_multiple_times(void)
{
// First initialization
TEST_ASSERT_EQUAL(0, LOG_init());
// Write an entry
TEST_ASSERT_GREATER_THAN(0, LOG_write(LOG_LEVEL_INFO, "First entry"));
// Second initialization (should not corrupt state)
TEST_ASSERT_EQUAL(0, LOG_init());
// Write another entry after re-initialization
TEST_ASSERT_GREATER_THAN(0, LOG_write(LOG_LEVEL_INFO, "Second entry after re-init"));
// Read back entries to ensure logger is still functional
char buffer[128];
int level;
int read_result = LOG_read(&level, buffer, sizeof(buffer));
TEST_ASSERT_TRUE(read_result > 0);
// Optionally, check that the buffer contains one of the expected messages
TEST_ASSERT_TRUE(
strstr(buffer, "First entry") != NULL ||
strstr(buffer, "Second entry after re-init") != NULL
);
}
// Test buffer wraparound detection
void test_LOG_buffer_wraparound_detection(void)
{
>>>>>>> REPLACE
</suggested_fix>
### Comment 6
<location> `tests/test_logging.c:350` </location>
<code_context>
+void test_LOG_buffer_partial_read(void)
</code_context>
<issue_to_address>
Consider adding a test for LOG_available when the buffer is empty.
Explicitly test that LOG_available returns 0 when the buffer is empty to ensure correct behavior.
Suggested implementation:
```c
void test_LOG_available_empty_buffer(void)
{
// Ensure the log buffer is empty (implementation may vary)
// If there is a LOG_clear or similar, call it here. Otherwise, assume buffer is empty at start.
// LOG_clear();
// Act & Assert
TEST_ASSERT_EQUAL(0, LOG_available());
}
// Test that buffer can be partially read to make space for new entries
void test_LOG_buffer_partial_read(void)
{
```
- If your logging system requires explicit clearing of the buffer, uncomment or implement a `LOG_clear()` or equivalent call in the new test.
- Make sure to register `test_LOG_available_empty_buffer` in your test runner if required by your test framework.
</issue_to_address>
### Comment 7
<location> `tests/test_logging.c:28` </location>
<code_context>
}
-void test_LOG_LL_init(void)
+void test_LOG_init(void)
{
// Check if the log buffer is initialized correctly
- TEST_ASSERT_NOT_NULL(g_LOG_LL_buffer.buffer);
- TEST_ASSERT_EQUAL(0, g_LOG_LL_buffer.head);
- TEST_ASSERT_EQUAL(0, g_LOG_LL_buffer.tail);
- TEST_ASSERT_EQUAL(LOG_LL_BUFFER_SIZE, g_LOG_LL_buffer.size);
+ TEST_ASSERT_NOT_NULL(g_log_handle->buffer.buffer);
+ TEST_ASSERT_EQUAL(0, g_log_handle->buffer.head);
+ TEST_ASSERT_EQUAL(0, g_log_handle->buffer.tail);
+ TEST_ASSERT_EQUAL(LOG_BUFFER_SIZE, g_log_handle->buffer.size);
}
</code_context>
<issue_to_address>
Consider testing LOG_task when the buffer is empty.
Adding a test for LOG_task with an empty buffer will help verify it handles this scenario gracefully without output or errors.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
void tearDown(void)
{
// Reset logging state
circular_buffer_reset(&g_log_handle->buffer);
}
=======
void tearDown(void)
{
// Reset logging state
circular_buffer_reset(&g_log_handle->buffer);
}
// Test LOG_task with an empty buffer to ensure it handles gracefully
void test_LOG_task_empty_buffer(void)
{
// Ensure buffer is empty
circular_buffer_reset(&g_log_handle->buffer);
// Optionally, capture output if LOG_task prints/logs to stdout/stderr
// For now, we just call the function to ensure no crash or error
LOG_task();
// Buffer should remain empty
TEST_ASSERT_EQUAL(0, g_log_handle->buffer.head);
TEST_ASSERT_EQUAL(0, g_log_handle->buffer.tail);
}
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the logging system by consolidating platform-level and system-level logging into a unified util module with buffered, double-buffer management.
- Introduces a new logging API (
LOG_init,LOG_write,LOG_available,LOG_read_entry,LOG_task) and level macros - Updates all initialization calls and tests to use the new API
- Removes legacy
logging_llmodules and updates build configurations accordingly
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_logging.c | Updated tests to use new LOG API and forward-declare LOG_Handle |
| tests/CMakeLists.txt | Removed linkage to pslab-logging |
| src/util/logging.h | Added unified logging interface and macros |
| src/util/logging.c | Implemented buffered logging logic |
| src/util/CMakeLists.txt | Included logging.c in pslab-util library |
| src/system/system.c | Replaced LOG_service_platform with LOG_task |
| src/system/logging.h | Removed legacy system logging header |
| src/system/logging.c | Removed legacy system logging implementation |
| src/system/CMakeLists.tests.txt | Removed legacy logging build configuration |
| src/system/CMakeLists.target.txt | Removed logging.c from firmware target |
| src/platform/logging_ll.h | Removed legacy platform logging header |
| src/platform/logging_ll.c | Removed legacy platform logging implementation |
| src/platform/h563xx/platform.c | Switched to LOG_init and updated init macro usage |
| src/platform/CMakeLists.txt | Removed legacy logging source from platform build |
| src/application/main.c | Added LOG_FUNCTION_ENTRY/EXIT and replaced service calls with LOG_task |
Comments suppressed due to low confidence (3)
tests/test_logging.c:197
- The test function name has an extra 'LOG'; rename it to test_LOG_read_entry_null_pointer for consistency.
void test_LOG_LOG_read_entry_null_pointer(void)
src/util/logging.h:75
- [nitpick] Use NULL instead of nullptr in documentation to match C conventions.
* @return Pointer to the log handle, or nullptr on error
tests/test_logging.c:14
- Use NULL instead of the C++-only nullptr in C test code.
static struct LOG_Handle *g_log_handle = nullptr;
bessman
force-pushed
the
refactor/logging
branch
from
77d1f52 to
a134857
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR refactors the logging system, moving both the system-level and platform-level logging modules into a common logging module under util. The new common module uses double-buffering, similar to the old platform-level mechanism, rather than output directly to stdout like the old system-level mechanism.
The logging system must now be initialized before it can be used, by calling LOG_init, preferably early in the boot process (currently done in PLATFORM_init, right after clock setup).
A new LOG_task function has been added, which must be called periodically to output log messages to stdout, similar to the old LOG_service_platform.
Summary by Sourcery
Unify and relocate the logging system into the util module, replacing the old platform‐ and system‐level implementations with a common circular‐buffered logger that must be initialized early and serviced periodically.
New Features:
Enhancements:
Build:
Tests: