Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
14 changes: 13 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ $(wildcard $(FREERTOS_PATH)/*.c) \
$(FREERTOS_PATH)/portable/GCC/ARM_CM4F/port.c \
$(wildcard common/Src/*.c) \
$(wildcard driver/Src/*.c) \
$(wildcard task/Src/*.c) \
$(filter-out $(addprefix bsp/Src/,$(addsuffix .c,$(BSP_DISABLE))),$(wildcard bsp/Src/*.c))

# ASM sources
Expand Down Expand Up @@ -180,14 +181,25 @@ $(FREERTOS_PATH)/include \
$(FREERTOS_PATH)/portable/GCC/ARM_CM4F \
common/Inc \
driver/Inc \
task/Inc \
bsp/Inc

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
59 changes: 32 additions & 27 deletions bsp/Src/UART.c
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
#include "UART.h"
#include <string.h>

static int a = 0;
static int b = 0;

// Define the size of the data to be transmitted
// Currently not used, as we send uint8_t directly
// may need to be configured for support for packets less more than 8 bits
Expand Down Expand Up @@ -227,7 +230,7 @@ __weak void HAL_UART_MspGPIOInit(UART_HandleTypeDef *huart){

init.Pin = GPIO_PIN_2;

HAL_GPIO_Init(GPIOD, &init);
HAL_GPIO_Init(GPIOD, &init);
}
#endif /* UART5 */

Expand Down Expand Up @@ -502,7 +505,7 @@ uart_status_t uart_init(UART_HandleTypeDef* handle) {
if (HAL_UART_Init(handle) != HAL_OK ||
!IS_UART_INSTANCE(handle->Instance) ||
!IS_USART_INSTANCE(handle->Instance)){
return UART_ERR;
return UART_ERR;
}

// Start reception
Expand Down Expand Up @@ -531,36 +534,36 @@ uart_status_t uart_deinit(UART_HandleTypeDef* handle) {
// Deinitialize Handler
#ifdef UART4
if (handle->Instance == UART4) {
uart4_tx_queue = NULL;
uart4_rx_queue = NULL;
uart4_tx_queue = NULL;
uart4_rx_queue = NULL;
}
#endif /* UART4 */

#ifdef UART5
if (handle->Instance == UART5) {
uart5_tx_queue = NULL;
uart5_rx_queue = NULL;
uart5_tx_queue = NULL;
uart5_rx_queue = NULL;
}
#endif /* UART5 */

#ifdef USART1
if (handle->Instance == USART1) {
usart1_tx_queue = NULL;
usart1_rx_queue = NULL;
usart1_tx_queue = NULL;
usart1_rx_queue = NULL;
}
#endif /* USART1 */

#ifdef USART2
if (handle->Instance == USART2) {
usart2_tx_queue = NULL;
usart2_rx_queue = NULL;
usart2_tx_queue = NULL;
usart2_rx_queue = NULL;
}
#endif /* USART2 */

#ifdef USART3
if (handle->Instance == USART3) {
usart3_tx_queue = NULL;
usart3_rx_queue = NULL;
usart3_tx_queue = NULL;
usart3_rx_queue = NULL;
}
#endif /* USART3 */

Expand Down Expand Up @@ -650,6 +653,7 @@ uart_status_t uart_send(UART_HandleTypeDef* handle, const uint8_t* data, uint8_t
portENTER_CRITICAL();
if ((HAL_UART_GetState(handle) & HAL_UART_STATE_BUSY_TX) != HAL_UART_STATE_BUSY_TX &&
tx_queue != NULL && uxQueueMessagesWaiting (*tx_queue) == 0 ) { // check if UART is ready and queue is empty
a++;
// Copy data to static buffer
memcpy(tx_buffer, data, length);
if (HAL_UART_Transmit_IT(handle, tx_buffer, length) != HAL_OK) {
Expand All @@ -660,26 +664,27 @@ uart_status_t uart_send(UART_HandleTypeDef* handle, const uint8_t* data, uint8_t
}
portEXIT_CRITICAL();

b++;
// Send data in chunks based on DATA_SIZE
for (uint8_t i = 0; i < length; i+=DATA_SIZE) {
tx_payload_t payload;
for (size_t i = 0; i < length; i+=DATA_SIZE) {
tx_payload_t payload;

// Ensure we only copy DATA_SIZE bytes at a time
uint8_t chunk_size = (length - i < DATA_SIZE) ? (length - i) : DATA_SIZE;
// EX: i=4, length=6, DataSize=4, then chunk_size = 2, instead of usual 4 since we've reached end of length
// Ensure we only copy DATA_SIZE bytes at a time
size_t chunk_size = (length - i < DATA_SIZE) ? (length - i) : DATA_SIZE;
// EX: i=4, length=6, DataSize=4, then chunk_size = 2, instead of usual 4 since we've reached end of length

// Copy the appropriate number of bytes to the payload data
memcpy(payload.data, &data[i], chunk_size); // Usually chunk_size = DATA_SIZE until end of data length
// Copy the appropriate number of bytes to the payload data
memcpy(payload.data, &data[i], chunk_size); // Usually chunk_size = DATA_SIZE until end of data length

// If data size is smaller than DATA_SIZE, fill the rest of the payload
if (chunk_size < DATA_SIZE) {
memset(&payload.data[chunk_size], 0, DATA_SIZE - chunk_size); // Fill the rest with 0 (or other padding if needed)
}
// If data size is smaller than DATA_SIZE, fill the rest of the payload
if (chunk_size < DATA_SIZE) {
memset(&payload.data[chunk_size], 0, DATA_SIZE - chunk_size); // Fill the rest with 0 (or other padding if needed)
}

// Enqueue the payload to be transmitted
if (xQueueSend(*tx_queue, &payload, delay_ticks) != pdTRUE) {
return UART_ERR;
} //delay_ticks: 0 = no wait, portMAX_DELAY = wait until space is available
// Enqueue the payload to be transmitted
if (xQueueSend(*tx_queue, &payload, delay_ticks) != pdTRUE) {
return UART_ERR;
} //delay_ticks: 0 = no wait, portMAX_DELAY = wait until space is available
}

exit:
Expand Down
18 changes: 2 additions & 16 deletions common/Src/syscalls.c
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,7 @@
#include <sys/times.h>
#include <time.h>

#include "UART.h"
#include "portmacro.h"

extern int __io_putchar(int ch) __attribute__((weak));
extern int __io_getchar(void) __attribute__((weak));
#include "stm32xx_hal.h"

char *__env[1] = {0};
char **environ = __env;
Expand All @@ -56,22 +52,12 @@ void _exit(int status) {

__weak int _read(int file, char *ptr, int len) {
(void)file;
int DataIdx;

for (DataIdx = 0; DataIdx < len; DataIdx++) {
*ptr++ = __io_getchar();
}

(void)ptr;
return len;
}

__weak int _write(int file, char *ptr, int len) {
(void)file;
int DataIdx;

for (DataIdx = 0; DataIdx < len; DataIdx++) {
__io_putchar(*ptr++);
}
return len;
}

Expand Down
17 changes: 17 additions & 0 deletions docs/FlashAndTheBug.md
Original file line number Diff line number Diff line change
Expand Up @@ -174,3 +174,20 @@ 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.

To use the logging library, `#define LOGGING_ENABLE 1` and `#define LOGGING_LEVEL x`, where x is a number between 1 and 5. Then, `#include "log.h"`.

Now, use `log(LEVEL, ...)` to log to the console. It's the exact same variadic arguments as `printf`, but with a LEVEL argument beforehand. The way this works is that any LOGGING_LEVEL equal to or below x will be what is recorded to the console.

The logging levels are as follows:
- L_FATAL: 1
- L_ERROR: 2
- L_WARN: 3
- L_INFO: 4
- L_DEBUG: 5
- L_TRACE: 6

So, for example, if logging level 5 is selected, any logs of L_DEBUG or below will be printed (L_INFO, L_WARN, L_ERROR, L_FATAL).
86 changes: 86 additions & 0 deletions driver/Inc/log.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
#include <stdbool.h>
#include <stdio.h>
#include "stm32xx_hal.h"

#ifndef LOGGING_ENABLE
#define LOGGING_ENABLE 0
#endif

#ifndef LOGGING_LEVEL
#define LOGGING_LEVEL 6
#endif

#define LOG_MAX_BEFORE_SIZE 32
#define LOG_MAX_LOG_SIZE 128

#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\r",
(char *)&log_before_buf, (char *)&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() ", \
log_level_color(level), log_level_name(level), \
pcTaskGetTaskName(NULL)); \
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
35 changes: 0 additions & 35 deletions driver/Src/printf.c

This file was deleted.

3 changes: 3 additions & 0 deletions middleware/FreeRTOS-Kernel/queue.c
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
#include <stdlib.h>
#include <string.h>

static int number_of_yields = 0;

/* Defining MPU_WRAPPERS_INCLUDED_FROM_API_FILE prevents task.h from redefining
* all the API functions to use the MPU wrappers. That should only be done when
* task.h is included from an application file. */
Expand Down Expand Up @@ -1129,6 +1131,7 @@ BaseType_t xQueueGenericSend( QueueHandle_t xQueue,
* is also a higher priority task in the pending ready list. */
if( xTaskResumeAll() == pdFALSE )
{
number_of_yields++;
taskYIELD_WITHIN_API();
}
}
Expand Down
5 changes: 3 additions & 2 deletions driver/Inc/printf.h → task/Inc/task_print.h
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
#pragma once

#include <stdbool.h>
#include <stdio.h>
#include "UART.h"

extern int printf ( const char * format, ... );
extern int scanf ( const char * format, ... );

bool printf_init(UART_HandleTypeDef *huart);
void task_print_init(UART_HandleTypeDef *huart);
void task_print(void *huart);
Loading