Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,17 @@ C_INCLUDES := $(addprefix -I,$(C_INCLUDES))
# compile gcc flags
ASFLAGS = $(MCU) $(AS_DEFS) $(AS_INCLUDES) $(OPT) -Wall -Werror -Wfatal-errors -fdata-sections -ffunction-sections

CFLAGS += $(MCU) $(C_DEFS) $(C_INCLUDES) $(OPT) -Wall -Werror -Wfatal-errors -fdata-sections -ffunction-sections
CFLAGS += \
$(MCU) \
$(C_DEFS) \
$(C_INCLUDES) \
$(OPT) \
-Wall \
-Werror \
-Wfatal-errors \
-fdata-sections \
-ffunction-sections \
-ffile-prefix-map=$(MAKEFILE_DIR)=

ifeq ($(DEBUG), 1)
CFLAGS += -g -gdwarf-2
Expand Down
4 changes: 4 additions & 0 deletions docs/FlashAndTheBug.md
Original file line number Diff line number Diff line change
Expand Up @@ -174,3 +174,7 @@ To view the output, open up an application like [PuTTY](https://www.chiark.green
- For PuTTY, click Serial and enter your desired COM port. This should show up on your device manager (for Mac or Linux, run `lsusb`). Set the baud rate to what you configured the UART for. Hit the big open button at the bottom.
- For picocom, type in `picocom -b <baud-rate> <tty-name>` and you should be set.

#### Logging Library

A convenient integration we've made for easier debugging is this logging header file. Credit to Clark Poon.

85 changes: 85 additions & 0 deletions driver/Inc/log.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
#include <stdbool.h>
#include "printf.h"

#ifndef LOGGING_ENABLE
#define LOGGING_ENABLE 0
#endif

#ifndef LOGGING_LEVEL
#define LOGGING_LEVEL 6
#endif

#define LOG_MAX_BEFORE_SIZE 64
#define LOG_MAX_LOG_SIZE 256

#define CONVERT_TO_STRING(arg) #arg
#define AS_STRING(arg) CONVERT_TO_STRING(arg)

static char log_buf[LOG_MAX_LOG_SIZE];
static char log_before_buf[LOG_MAX_BEFORE_SIZE];
Comment on lines +19 to +20
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The global static buffers log_buf and log_before_buf are not thread-safe. This project uses FreeRTOS, and if two tasks call the log() macro concurrently, they will write to these buffers at the same time, leading to a race condition and corrupted log output.

To ensure thread safety, all operations within the log macro that use these shared buffers should be atomic. This can be achieved by protecting the critical section with a mutex. You'll likely need to add an initialization function for the logger to create the mutex, which should be called once at startup before any logging occurs.


typedef enum {
L_NONE = 0,
L_FATAL = 1,
L_ERROR = 2,
L_WARN = 3,
L_INFO = 4,
L_DEBUG = 5,
L_TRACE = 6,
} LoggingLevel;

static inline const char *
log_level_name(LoggingLevel level)
{
switch (level) {
case L_TRACE: return "TRACE";
case L_DEBUG: return "DEBUG";
case L_INFO: return "INFO";
case L_WARN: return "WARN";
case L_ERROR: return "ERROR";
case L_FATAL: return "FATAL";
case L_NONE: return "NONE";
}
/* Should be unreachable */
return NULL;
}
Comment on lines +32 to +46
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This function returns NULL if an unknown LoggingLevel is passed. This NULL is then passed to snprintf with a %s format specifier, which results in undefined behavior and could crash the program.

To make the logger more robust, consider adding a default case to handle unknown levels.

static inline const char *
log_level_name(LoggingLevel level)
{
    switch (level) {
    case L_TRACE: return "TRACE";
    case L_DEBUG: return "DEBUG";
    case L_INFO:  return "INFO";
    case L_WARN:  return "WARN";
    case L_ERROR: return "ERROR";
    case L_FATAL: return "FATAL";
    case L_NONE:  return "NONE";
    default:      return "?????";
    }
}


static inline const char *
log_level_color(LoggingLevel level)
{
switch (level) {
case L_TRACE: return "\x1b[00;34m";
case L_DEBUG: return "\x1b[00;35m";
case L_INFO: return "\x1b[00;36m";
case L_WARN: return "\x1b[00;33m";
case L_ERROR: return "\x1b[00;31m";
case L_FATAL: return "\x1b[37;41m";
case L_NONE: return "\x1b[0m";
}
/* Should be unreachable */
return NULL;
}
Comment on lines +48 to +62
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similar to log_level_name, this function returns NULL for an unknown LoggingLevel, which can lead to undefined behavior. Adding a default case will make the function more robust. It should probably return the 'reset color' code to avoid breaking terminal output.

static inline const char *
log_level_color(LoggingLevel level)
{
    switch (level) {
    case L_TRACE: return "\x1b[00;34m";
    case L_DEBUG: return "\x1b[00;35m";
    case L_INFO:  return "\x1b[00;36m";
    case L_WARN:  return "\x1b[00;33m";
    case L_ERROR: return "\x1b[00;31m";
    case L_FATAL: return "\x1b[37;41m";
    case L_NONE:  return "\x1b[0m";
    default:      return "\x1b[0m";
    }
}


static inline void
log_printf(void)
{
printf("%-" AS_STRING(LOG_MAX_BEFORE_SIZE) "s | %s%s\n\r",
(char *)&log_before_buf, (char *)&log_buf, log_level_color(L_NONE));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The casts (char *)&log_before_buf and (char *)&log_buf are unnecessary and potentially confusing. Since log_before_buf and log_buf are arrays of char, they will decay to char* when passed to printf. You can use them directly.

    printf("%-" AS_STRING(LOG_MAX_BEFORE_SIZE) "s | %s%s\n\r",
           log_before_buf, log_buf, log_level_color(L_NONE));

}

#if LOGGING_ENABLE == 0 || LOGGING_LEVEL == 0
#define log(LEVEL, fmt ...)
#else
#define log(LEVEL, fmt ...) \
do { \
LoggingLevel level = LEVEL; \
if (LOGGING_LEVEL >= level) { \
snprintf(log_before_buf, LOG_MAX_BEFORE_SIZE, \
"%s%-5s | %s:%d:%s() ", \
log_level_color(level), log_level_name(level), \
__FILE__, __LINE__, __func__); \
snprintf((char *)&log_buf, LOG_MAX_LOG_SIZE, fmt); \
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The cast (char *)&log_buf is unnecessary. Since log_buf is an array of char, it will decay to a char* when passed to snprintf. You can use log_buf directly.

            snprintf(log_buf, LOG_MAX_LOG_SIZE, fmt);

log_printf(); \
} \
} while (0)
#endif
78 changes: 78 additions & 0 deletions test/tests/log_test.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
// A simple echo application to test input and output over serial
#include "UART.h"
#include "projdefs.h"
#include "stm32xx_hal.h"
#include <stdio.h>
#include "printf.h"

// Enables logging
#define LOGGING_ENABLE 1

// MUST BE DEFINED AS AN INTEGER
// Logging levels:
// L_FATAL: 1
// L_ERROR: 2
// L_WARN: 3
// L_DEBUG: 4
// L_TRACE: 5
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The comments describing the logging levels are incorrect and incomplete. They are missing L_INFO and have the wrong values for L_DEBUG and L_TRACE. This can be misleading for anyone reading the test.

The correct values from log.h are:

  • L_INFO: 4
  • L_DEBUG: 5
  • L_TRACE: 6
// Logging levels:
// L_FATAL: 1
// L_ERROR: 2
// L_WARN: 3
// L_INFO: 4
// L_DEBUG: 5
// L_TRACE: 6

#define LOGGING_LEVEL 4
#include "log.h"

StaticTask_t txTaskBuffer;
StackType_t txTaskStack[configMINIMAL_STACK_SIZE];

void HAL_UART_MspGPIOInit(UART_HandleTypeDef *huart){
GPIO_InitTypeDef init = {0};
__HAL_RCC_GPIOA_CLK_ENABLE();

/* enable port A USART2 gpio
PA2 -> USART2_TX
PA3 -> USART2_RX
*/
init.Pin = GPIO_PIN_2|GPIO_PIN_3;
init.Mode = GPIO_MODE_AF_PP;
init.Pull = GPIO_NOPULL;
init.Speed = GPIO_SPEED_FREQ_VERY_HIGH;
init.Alternate = GPIO_AF7_USART2;
HAL_GPIO_Init(GPIOA, &init);
}

void TxTask(void *argument){
husart2->Init.BaudRate = 115200;
husart2->Init.WordLength = UART_WORDLENGTH_8B;
husart2->Init.StopBits = UART_STOPBITS_1;
husart2->Init.Parity = UART_PARITY_NONE;
husart2->Init.Mode = UART_MODE_TX_RX;
husart2->Init.HwFlowCtl = UART_HWCONTROL_NONE;
husart2->Init.OverSampling = UART_OVERSAMPLING_16;

printf_init(husart2);

while(1){
log(L_FATAL, "Test Fatal");
log(L_ERROR, "Test Error");
log(L_WARN, "Test Warn");
log(L_INFO, "Test Info");
log(L_DEBUG, "Test Debug");
log(L_TRACE, "Test Trace");
vTaskDelay(pdMS_TO_TICKS(100));
}
}

int main(void) {
HAL_Init();
SystemClock_Config();

xTaskCreateStatic(TxTask,
"TX",
configMINIMAL_STACK_SIZE,
NULL,
tskIDLE_PRIORITY + 2,
txTaskStack,
&txTaskBuffer);

vTaskStartScheduler();

while (1) {
}
}
Loading