Logging rewrite#92
Conversation
|
Consider renaming this to just 'Rewriting logging' or 'Logging rewrite' and just update the status with a checklist or comments |
Kamilosok
added
documentation
frogrammer9
added
rework
Krzychuo
force-pushed
the
ring-buffer-logging
branch
10 times, most recently
from
d86385d to
51f9739
Compare
…ring-buffer-logging
Krzychuo
force-pushed
the
ring-buffer-logging
branch
from
51f9739 to
f8261f5
Compare
frogrammer9
left a comment
frogrammer9
left a comment
There was a problem hiding this comment.
UART is a specific protocol, while ‘serial’ is the more generic term for this type of communication interface. I would prefer the latter name to be used.
| for (u32 i = 0; i < g_indent_level && written < TEMP_BUF_SIZE - 1; i++) temp_buffer[written++] = ' '; | ||
|
|
||
| const char* prefix = g_prefixes[loglvl]; | ||
| for (size_t i = 0; prefix[i] && written < TEMP_BUF_SIZE - 1; i++) temp_buffer[written++] = prefix[i]; |
There was a problem hiding this comment.
There should be a space after the loglvl prefix.
|
|
||
| static u32 g_indent_level = 0; | ||
| static const char* g_prefixes[] = {"[ERROR]", "[WARNING]", "[ ]", "[~]"}; | ||
| static const char* g_overflow_warning = "[WARNING]Some of the logs were lost due to overflow"; |
There was a problem hiding this comment.
There should be a space after the loglvl prefix.
| static const char* g_overflow_warning = "[WARNING]Some of the logs were lost due to overflow"; | ||
|
|
||
| #ifdef __DEBUG__ | ||
| #define RING_BUF_SIZE (1 << 20) |
There was a problem hiding this comment.
There is no point of using macros, just use const size_t RING_BUF_SIZE = ...;
| i32 write; | ||
| bool full; | ||
| bool overflown; | ||
| void (*uart_tx)(char c); |
There was a problem hiding this comment.
I don't think this should be a part of the ring buffer structure.
|
|
||
| static ring_buffer_t g_buffer; | ||
|
|
||
| static ring_buffer_t* get_buffer() { |
There was a problem hiding this comment.
Is this function necessary?
| while (str[it]) put_char(rb, str[it++]); | ||
| } | ||
|
|
||
| static void put_overflow_warning(ring_buffer_t* rb) { |
There was a problem hiding this comment.
There is no point in copying this message to the ring buffer since it is stored already in static memory.
When serial or tty is being initialized simply check if ring buffer has overflown and display this message before dumping the ring buffer contents.
| DEBUG_PRINTF("%s ", g_prefixes[loglvl]); | ||
| DEBUG_VPRINTF(fmt, va); | ||
| size_t written = 0; | ||
| for (u32 i = 0; i < g_indent_level && written < TEMP_BUF_SIZE - 1; i++) temp_buffer[written++] = ' '; |
There was a problem hiding this comment.
Storing '\t' chars in the ring buffer can be potentially wasteful. ASCII values between 0 and 8 are not and will be not important for logger so we can store those values and when printing interpret then as the size of the indentation.
| KLSL_TRACE = 3, | ||
| } klog_severity_level_t; | ||
|
|
||
| char ring_buffer_get_char(); |
There was a problem hiding this comment.
The existence of the ring buffer is some unimportant implementation detail for the logger user, there is no point in providing this information. Those function should be named something like "klog_flush_serial" "klog_set_serial_handler", the functionality of getting chars from the buffer is unnecessary.
3 participants
Work in progress.
Remaining things to do: