Hello,
I am talking about nRF5_SDK_17.0.2_d674dde/components/libraries/log/src/nrf_log_frontend.c.
In the case of HEXDUMP there seems to be some inconsistency in the packet length management as per header base.hexdump.len field.
If we Iook for all the occurrence of this information element, sometimes is it a 32bits word count, and sometimes it is an octet count.
- in function
invalid_packets_omit, @ line 332, we have this code :
*p_rd_idx += (HEADER_SIZE + p_header->base.hexdump.len);
so in this place, this is a uint32_t count.
- in function
log_skip, @line we have this code :rd_idx += CEIL_DIV(header.base.hexdump.len, sizeof(uint32_t));
so in this place this is an octet count.
- In function
nrf_log_frontend_hexdump, @line 688, we have :
p_header->base.hexdump.len = length;
once again, an octet count. - In function
nrf_log_frontend_dequeue, @lines 761-764, it is handled once again like an octet count. - In function
buf_prealloc, @ line 490, it is handled like a uint32_t count.
After thinking twice about all this, I understood that when in_progress flag is raised, this it is an uint32_t count, and otherwise an octet count.
I think that this aspect of the code is obscure and quite hard to understand / maintain. You should have made something like
typedef struct{ uint32_t in_progress : 1; uint32_t allocated_len : 31;} nrf_log_in_progress_header_t
typedef struct{ uint32_t in_progress: 1; uint32_t type : 2; uint32_t data : 29;} nrf_log_generic_header_t;
typedef struct{ uint32_t in_progress: 1; uint32_t type : 2; uint32_t severity : 3; uint32_t nargs : 4; uint32_t addr : 22;} nrf_log_std_header_t;
typedef struct{ uint32_t in_progress: 1; uint32_t type : 2; uint32_t severity : 3; uint32_t offset : 10; uint32_t reserved : 6; uint32_t len : 10;} nrf_log_hexdump_header_t;
typedef union{ nrf_log_in_progress_header_t in_progress; nrf_log_generic_header_t generic; nrf_log_std_header_t std; nrf_log_hexdump_header_t hexdump; uint32_t raw;} nrf_log_main_header_t;
typedef struct{ nrf_log_main_header_t base; uint16_t module_id; uint16_t dropped; uint32_t timestamp;} nrf_log_header_t;
And then whenever you have in_progress flag set to 1, use the in_progress union choice, and in_progress.allocated_len to get the length in terms of uint32_t word count. So in buf_prealloc and invalid_packets_omit you would just use in_progress union choice.
Or maybe I misunderstood what this code is doing, and then I would welcome your feedback on the above.
