From af4df10b655b48146433a5a13d0d7ed09b5d43e0 Mon Sep 17 00:00:00 2001 From: Konstantin Kondrashov Date: Wed, 17 Apr 2024 10:24:29 +0300 Subject: [PATCH] feat(log): Refactoring buffer log APIs (2) --- .../log/host_test/log_test/main/log_test.cpp | 49 +++++ components/log/include/esp_private/log_util.h | 30 +-- components/log/src/buffer/log_buffers.c | 193 +++++++++--------- components/log/src/util.c | 66 +++--- 4 files changed, 189 insertions(+), 149 deletions(-) diff --git a/components/log/host_test/log_test/main/log_test.cpp b/components/log/host_test/log_test/main/log_test.cpp index 28fd21520d..c81282dcb0 100644 --- a/components/log/host_test/log_test/main/log_test.cpp +++ b/components/log/host_test/log_test/main/log_test.cpp @@ -10,6 +10,7 @@ #include #include #include "esp_log.h" +#include "esp_private/log_util.h" #include @@ -321,3 +322,51 @@ TEST_CASE("changing early log level") ESP_EARLY_LOGI(TEST_TAG, "must indeed be printed"); CHECK(regex_search(fix.get_print_buffer_string(), test_print) == true); } + +TEST_CASE("esp_log_util_cvt") +{ + char buf[128]; + CHECK(esp_log_util_cvt_dec(123456, 0, buf) == 6); + CHECK(strcmp(buf, "123456") == 0); + memset(buf, 0, sizeof(buf)); + + CHECK(esp_log_util_cvt_dec(123456, 2, buf) == 6); + CHECK(strcmp(buf, "123456") == 0); + memset(buf, 0, sizeof(buf)); + + CHECK(esp_log_util_cvt_dec(123456, 8, buf) == 8); + CHECK(strcmp(buf, "00123456") == 0); + memset(buf, 0, sizeof(buf)); + + CHECK(esp_log_util_cvt_dec(1, 0, buf) == 1); + CHECK(strcmp(buf, "1") == 0); + memset(buf, 0, sizeof(buf)); + + CHECK(esp_log_util_cvt_dec(0, 0, buf) == 1); + CHECK(strcmp(buf, "0") == 0); + memset(buf, 0, sizeof(buf)); + + CHECK(esp_log_util_cvt_dec(0, 4, buf) == 4); + CHECK(strcmp(buf, "0000") == 0); + memset(buf, 0, sizeof(buf)); + + CHECK(esp_log_util_cvt_dec(1, 2, buf) == 2); + CHECK(strcmp(buf, "01") == 0); + memset(buf, 0, sizeof(buf)); + + CHECK(esp_log_util_cvt_hex(0x73f, -1, buf) == 3); + CHECK(strcmp(buf, "73f") == 0); + memset(buf, 0, sizeof(buf)); + + CHECK(esp_log_util_cvt_hex(0x73f, 2, buf) == 3); + CHECK(strcmp(buf, "73f") == 0); + memset(buf, 0, sizeof(buf)); + + CHECK(esp_log_util_cvt_hex(0x73f, 3, buf) == 3); + CHECK(strcmp(buf, "73f") == 0); + memset(buf, 0, sizeof(buf)); + + CHECK(esp_log_util_cvt_hex(0x73f, 4, buf) == 4); + CHECK(strcmp(buf, "073f") == 0); + memset(buf, 0, sizeof(buf)); +} diff --git a/components/log/include/esp_private/log_util.h b/components/log/include/esp_private/log_util.h index f361056337..18f2b5d93e 100644 --- a/components/log/include/esp_private/log_util.h +++ b/components/log/include/esp_private/log_util.h @@ -16,16 +16,19 @@ extern "C" { * This function converts the given unsigned integer value to a string representation in the specified radix. * The resulting string is stored in the provided character buffer `buf`. * - * @param val The unsigned integer value to be converted. - * @param buf Pointer to the character buffer where the resulting string will be stored. - * The buffer must have enough space to accommodate the entire converted string, - * including the null-terminator. - * @param radix The base of the numeral system to be used for the conversion. - * It determines the number of unique digits in the numeral system - * (e.g., 2 for binary, 10 for decimal, 16 for hexadecimal). - * @param digits Pointer to a character array representing the digits of the numeral system. - * The array must contain characters in the order of increasing values, - * corresponding to the digits of the radix. For example, "0123456789ABCDEF" for hexadecimal. + * @param[in] val The unsigned integer value to be converted. + * @param[in] radix The base of the numeral system to be used for the conversion. + * It determines the number of unique digits in the numeral system + * (e.g., 2 for binary, 10 for decimal, 16 for hexadecimal). + * @param[in] pad The optional padding width (0 - unused) for the resulting string. It adds zero-padding. + * (val=123, pad=6 -> result=000123). + * @param[in] digits Pointer to a character array representing the digits of the + * numeral system. The array must contain characters in the order of increasing + * values, corresponding to the digits of the radix. For example, "0123456789ABCDEF" + * or hexadecimal. + * @param[out] buf Pointer to the character buffer where the resulting string will + * be stored. The buffer must have enough space to accommodate the entire converted + * string, including the null-terminator. * * @return The length of the resulting string (excluding the null-terminator). * @@ -33,14 +36,13 @@ extern "C" { * The caller is responsible for ensuring the buffer's size is large enough to prevent buffer overflow. * @note The provided `digits` array must have enough elements to cover the entire radix used for conversion. Otherwise, undefined behavior may occur. */ - -int esp_log_util_cvt(unsigned long long val, char *buf, long radix, const char *digits); +int esp_log_util_cvt(unsigned long long val, long radix, int pad, const char *digits, char *buf); /** * @brief Convert an unsigned integer to a hexadecimal string with optional padding. * * This function converts an unsigned integer value to a hexadecimal string representation. - * This function calls esp_log_util_cvt(val, buf, 16, pad, "0123456789abcdef") inside. + * This function calls esp_log_util_cvt(val, 16, pad, "0123456789abcdef", buf) inside. * * @param val The unsigned integer value to be converted. * @param pad The optional padding width for the resulting string. @@ -53,7 +55,7 @@ int esp_log_util_cvt_hex(unsigned long long val, int pad, char *buf); * @brief Convert an unsigned integer to a decimal string with optional padding. * * This function converts an unsigned integer value to a decimal string representation. - * This function calls esp_log_util_cvt(val, buf, 10, pad, "0123456789") inside. + * This function calls esp_log_util_cvt(val, 10, pad, "0123456789", buf) inside. * * @param val The unsigned integer value to be converted. * @param pad The optional padding width for the resulting string. diff --git a/components/log/src/buffer/log_buffers.c b/components/log/src/buffer/log_buffers.c index beec5e978d..88887ed667 100644 --- a/components/log/src/buffer/log_buffers.c +++ b/components/log/src/buffer/log_buffers.c @@ -4,145 +4,134 @@ * SPDX-License-Identifier: Apache-2.0 */ -#include -#include #include #include #include "esp_log.h" +#include "esp_private/log_util.h" #ifndef CONFIG_IDF_TARGET_LINUX #include "esp_memory_utils.h" // for esp_ptr_byte_accessible -#else +#else // !CONFIG_IDF_TARGET_LINUX static inline bool esp_ptr_byte_accessible(const void* ptr) { (void) ptr; return true; } -#endif // CONFIG_IDF_TARGET_LINUX +#endif // !CONFIG_IDF_TARGET_LINUX -//print number of bytes per line for esp_log_buffer_char and esp_log_buffer_hex +/* It represents the number of bytes printed per line when displaying the + * contents of a buffer in hexadecimal format. It determines how many bytes of + * the buffer will be shown on each line, making it easier to read and interpret + * the data. */ #define BYTES_PER_LINE 16 -void esp_log_buffer_hex_internal(const char *tag, const void *buffer, uint16_t buff_len, - esp_log_level_t log_level) +/* Checks if a character is a printable ASCII character. + * If the provided character falls within the printable ASCII range, + * which includes characters with ASCII values from 32 (space) to 126 (tilde). */ +#define IS_CHAR_PRINTABLE(character) ((character) >= 32 && (character) <= 126) + +typedef void (*print_line_t)(uintptr_t, const void *, char *, int); + +static void log_buffer_hex_line(uintptr_t orig_buff, const void *ptr_line, char *output_str, int buff_len); +static void log_buffer_char_line(uintptr_t orig_buff, const void *ptr_line, char *output_str, int buff_len); +static void log_buffer_hexdump_line(uintptr_t orig_buff, const void *ptr_line, char *output_str, int buff_len); +static void print_buffer(const char *tag, const void *buffer, uint16_t buff_len, esp_log_level_t log_level, char *output_str, print_line_t print_line_func); + +void esp_log_buffer_hex_internal(const char *tag, const void *buffer, uint16_t buff_len, esp_log_level_t log_level) { - if (buff_len == 0) { - return; - } - char temp_buffer[BYTES_PER_LINE + 3]; //for not-byte-accessible memory - char hex_buffer[3 * BYTES_PER_LINE + 1]; - const char *ptr_line; - int bytes_cur_line; - - do { - if (buff_len > BYTES_PER_LINE) { - bytes_cur_line = BYTES_PER_LINE; - } else { - bytes_cur_line = buff_len; - } - if (!esp_ptr_byte_accessible(buffer)) { - //use memcpy to get around alignment issue - memcpy(temp_buffer, buffer, (bytes_cur_line + 3) / 4 * 4); - ptr_line = temp_buffer; - } else { - ptr_line = buffer; - } - - for (int i = 0; i < bytes_cur_line; i ++) { - sprintf(hex_buffer + 3 * i, "%02x ", (unsigned char) ptr_line[i]); - } - ESP_LOG_LEVEL(log_level, tag, "%s", hex_buffer); - buffer += bytes_cur_line; - buff_len -= bytes_cur_line; - } while (buff_len); + // I (954) log_example: 54 68 65 20 77 61 79 20 74 6f 20 67 65 74 20 73 + // I (962) log_example: 74 61 72 74 65 64 20 69 73 20 74 6f 20 71 75 69 + // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + char output_str[3 * BYTES_PER_LINE]; + print_buffer(tag, buffer, buff_len, log_level, output_str, log_buffer_hex_line); } -void esp_log_buffer_char_internal(const char *tag, const void *buffer, uint16_t buff_len, - esp_log_level_t log_level) +void esp_log_buffer_char_internal(const char *tag, const void *buffer, uint16_t buff_len, esp_log_level_t log_level) { - if (buff_len == 0) { - return; - } - char temp_buffer[BYTES_PER_LINE + 3]; //for not-byte-accessible memory - char char_buffer[BYTES_PER_LINE + 1]; - const char *ptr_line; - int bytes_cur_line; - - do { - if (buff_len > BYTES_PER_LINE) { - bytes_cur_line = BYTES_PER_LINE; - } else { - bytes_cur_line = buff_len; - } - if (!esp_ptr_byte_accessible(buffer)) { - //use memcpy to get around alignment issue - memcpy(temp_buffer, buffer, (bytes_cur_line + 3) / 4 * 4); - ptr_line = temp_buffer; - } else { - ptr_line = buffer; - } - - for (int i = 0; i < bytes_cur_line; i ++) { - sprintf(char_buffer + i, "%c", ptr_line[i]); - } - ESP_LOG_LEVEL(log_level, tag, "%s", char_buffer); - buffer += bytes_cur_line; - buff_len -= bytes_cur_line; - } while (buff_len); + // I (980) log_example: The way to get s + // I (985) log_example: tarted is to qui + // ^^^^^^^^^^^^^^^^^ + char output_str[BYTES_PER_LINE + 1]; + print_buffer(tag, buffer, buff_len, log_level, output_str, log_buffer_char_line); } void esp_log_buffer_hexdump_internal(const char *tag, const void *buffer, uint16_t buff_len, esp_log_level_t log_level) { + // I (1013) log_example: 0x3ffb5bc0 54 68 65 20 77 61 79 20 74 6f 20 67 65 74 20 73 |The way to get s| + // I (1024) log_example: 0x3ffb5bd0 74 61 72 74 65 64 20 69 73 20 74 6f 20 71 75 69 |tarted is to qui| + // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + char output_str[(2 + sizeof(void *) * 2) + 3 + (BYTES_PER_LINE * 3) + 2 + (1 + BYTES_PER_LINE + 1) + 1]; + print_buffer(tag, buffer, buff_len, log_level, output_str, log_buffer_hexdump_line); +} +static void print_buffer(const char *tag, const void *buffer, uint16_t buff_len, esp_log_level_t log_level, char *output_str, print_line_t print_line_func) +{ if (buff_len == 0) { return; } char temp_buffer[BYTES_PER_LINE + 3]; //for not-byte-accessible memory - const char *ptr_line; - //format: field[length] - // ADDR[10]+" "+DATA_HEX[8*3]+" "+DATA_HEX[8*3]+" |"+DATA_CHAR[8]+"|" - char hd_buffer[2 + sizeof(void *) * 2 + 3 + BYTES_PER_LINE * 3 + 1 + 3 + BYTES_PER_LINE + 1 + 1]; - char *ptr_hd; - int bytes_cur_line; do { - if (buff_len > BYTES_PER_LINE) { - bytes_cur_line = BYTES_PER_LINE; - } else { - bytes_cur_line = buff_len; - } + const char *ptr_line = buffer; + int bytes_cur_line = (buff_len > BYTES_PER_LINE) ? BYTES_PER_LINE : buff_len; if (!esp_ptr_byte_accessible(buffer)) { //use memcpy to get around alignment issue memcpy(temp_buffer, buffer, (bytes_cur_line + 3) / 4 * 4); ptr_line = temp_buffer; - } else { - ptr_line = buffer; } - ptr_hd = hd_buffer; - ptr_hd += sprintf(ptr_hd, "%p ", buffer); - for (int i = 0; i < BYTES_PER_LINE; i ++) { - if ((i & 7) == 0) { - ptr_hd += sprintf(ptr_hd, " "); - } - if (i < bytes_cur_line) { - ptr_hd += sprintf(ptr_hd, " %02x", (unsigned char) ptr_line[i]); - } else { - ptr_hd += sprintf(ptr_hd, " "); - } - } - ptr_hd += sprintf(ptr_hd, " |"); - for (int i = 0; i < bytes_cur_line; i ++) { - if (isprint((int)ptr_line[i])) { - ptr_hd += sprintf(ptr_hd, "%c", ptr_line[i]); - } else { - ptr_hd += sprintf(ptr_hd, "."); - } - } - ptr_hd += sprintf(ptr_hd, "|"); + print_line_func((uintptr_t)buffer, ptr_line, output_str, bytes_cur_line); - ESP_LOG_LEVEL(log_level, tag, "%s", hd_buffer); + ESP_LOG_LEVEL(log_level, tag, "%s", output_str); buffer += bytes_cur_line; buff_len -= bytes_cur_line; } while (buff_len); } + +static void log_buffer_hex_line(uintptr_t orig_buff, const void *ptr_line, char *output_str, int buff_len) +{ + (void) orig_buff; + const unsigned char *ptr = (unsigned char *)ptr_line; + for (int i = 0; i < buff_len; i++) { + output_str += esp_log_util_cvt_hex(ptr[i], 2, output_str); + *output_str++ = ' '; + } + *--output_str = 0; +} + +static void log_buffer_char_line(uintptr_t orig_buff, const void *ptr_line, char *output_str, int buff_len) +{ + (void) orig_buff; + const char *ptr = (char *)ptr_line; + memcpy(output_str, ptr, buff_len); + output_str[buff_len] = 0; +} + +static void log_buffer_hexdump_line(uintptr_t orig_buff, const void *ptr_line, char *output_str, int buff_len) +{ + const unsigned char *ptr = (unsigned char *)ptr_line; + *output_str++ = '0'; + *output_str++ = 'x'; + output_str += esp_log_util_cvt_hex(orig_buff, sizeof(uintptr_t), output_str); + *output_str++ = ' '; + for (int i = 0; i < BYTES_PER_LINE; i++) { + if ((i & 7) == 0) { + *output_str++ = ' '; + } + *output_str++ = ' '; + if (i < buff_len) { + output_str += esp_log_util_cvt_hex(ptr[i], 2, output_str); + } else { + *output_str++ = ' '; + *output_str++ = ' '; + } + } + *output_str++ = ' '; + *output_str++ = ' '; + *output_str++ = '|'; + for (int i = 0; i < buff_len; i++) { + *output_str++ = IS_CHAR_PRINTABLE(ptr[i]) ? ptr[i] : '.'; + } + *output_str++ = '|'; + *output_str = 0; +} diff --git a/components/log/src/util.c b/components/log/src/util.c index d1db007441..675ee09b97 100644 --- a/components/log/src/util.c +++ b/components/log/src/util.c @@ -6,53 +6,53 @@ #include -int esp_log_util_cvt(unsigned long long val, char *buf, long radix, int pad, const char *digits) +int esp_log_util_cvt(unsigned long long val, long radix, int pad, const char *digits, char *buf) { -#ifdef SUPPORT_LITTLE_RADIX - char temp[64]; -#else - char temp[32]; -#endif - char *buf_or = buf; - char *cp = temp; + char *orig_buf = buf; int length = 0; - if (val == 0) { - /* Special case */ - *cp++ = '0'; - } else { - while (val) { - *cp++ = digits[val % radix]; - val /= radix; - } - } - while (cp != temp) { - *buf++ = *--cp; + // val = 123 + do { + *buf++ = digits[val % radix]; + val /= radix; + length++; + } while (val); + // 3 2 1 + // buf = [0] [1] [2] [3] + + // length = 3, pad = 6 + while (pad > 0 && pad > length) { + *buf++ = '0'; length++; } *buf = '\0'; - if (length < pad) { - pad = pad - length; - int i = 0; - while (pad-- > 0) { - temp[i] = buf_or[i]; - buf_or[i] = '0'; - i++; - length++; - } - for (int k = 0; k < i; k++) { - buf_or[i + k] = temp[k]; - } + // length = 6 + // 3 2 1 0 0 0 \0 + // buf = [0] [1] [2] [3] [4] [5] [6] + + --buf; + // reverse the order of characters + // 3 2 1 0 0 0 \0 + // [0] [1] [2] [3] [4] [5] [6] + // orig_buf -- ^ ^ ----- buf + while (orig_buf < buf) { + char first_char = *orig_buf; + char last_char = *buf; + *buf-- = first_char; + *orig_buf++ = last_char; } + // 0 0 0 1 2 3 \0 + // buf = [0] [1] [2] [3] [4] [5] [6] + return (length); } int esp_log_util_cvt_hex(unsigned long long val, int pad, char *buf) { - return esp_log_util_cvt(val, buf, 16, pad, "0123456789abcdef"); + return esp_log_util_cvt(val, 16, pad, "0123456789abcdef", buf); } int esp_log_util_cvt_dec(unsigned long long val, int pad, char *buf) { - return esp_log_util_cvt(val, buf, 10, pad, "0123456789"); + return esp_log_util_cvt(val, 10, pad, "0123456789", buf); }