BSP UART Draft #72
BSP UART Draft #72
Conversation
IshDeshpa
assigned Lakshay983, NathanielDelgado, connorl309, ppatra126 and fangjim and unassigned Lakshay983, NathanielDelgado, connorl309 and ppatra126
|
Make sure you know that the RX queue exists in the user code so that the user can allocate according to their needs and size of buffer. The RX queue will not be in the BSP layer itself (I know you may not have meant that by your diagram, just want to reiterate). I'd also make sure you know the specific UART interrupt handlers you will need to use. Otherwise block diagram looks good. |
BSP/Inc/BSP_UART.h
Outdated
| #include "stm32f4xx.h" | ||
| #include "stm32f4xx_hal.h" | ||
| #include "stm32f4xx_hal_def.h" | ||
| #include "stm32f4xx_hal_rcc.h" | ||
| #include "stm32f4xx_hal_gpio.h" | ||
| #include "stm32f4xx_hal_uart.h" | ||
| #include "stm32f4xx_hal_usart.h" |
There was a problem hiding this comment.
the headers here should be system agnostic to support L4 and F4; i'd include <stm32xx_hal.h> (see how this is done in Tests/Test/blinky.c)
There was a problem hiding this comment.
@IshDeshpa are you going for like
#ifdef STM32F4 <include F4 files> #else <include L4 files> #endif
I don't remember how your Makefile stuff was changing
There was a problem hiding this comment.
That's what the stm32xx_hal.h does internally lol
BSP/Inc/BSP_UART.h
Outdated
|
|
||
|
|
||
| #define TX_SIZE 60 | ||
| #define RX_SIZE 60 |
There was a problem hiding this comment.
RX size will be defined by the user since the RX queue will exist outside of the BSP
BSP/Src/BSP_UART.c
Outdated
|
|
||
| // Define static queues for TX and RX | ||
| static QueueHandle_t txQueue; | ||
| static QueueHandle_t rxQueue; |
There was a problem hiding this comment.
RX queue will be defined by the user.
Additionally, we need to use StaticQueue_t in order to avoid dynamically allocating the memory. See https://www.freertos.org/Documentation/02-Kernel/04-API-references/06-Queues/02-xQueueCreateStatic
|
can you check to see if the queue is full (for adding) or empty (for popping) before you do any queue accesses. |
There was a problem hiding this comment.
I'll leave a larger review after @NathanielDelgado but I've started thinking about our structure for these a bit more and honestly I'm a bit confused on it too.
There was a problem hiding this comment.
If UART.c is the one we're using now you can delete this file.
BSP/Src/UART.c
Outdated
| uart_status_t uart_init(UART_HandleTypeDef* handle, QueueHandle_t* rxQueue) { | ||
| if (handle->Instance != UART4) { | ||
| return UART_ERR; | ||
| } | ||
|
|
||
| // Create TX queue | ||
| tx_queue = xQueueCreateStatic(QUEUE_LENGTH, | ||
| QUEUE_ITEM_SIZE, | ||
| tx_queue_storage, | ||
| &tx_queue_buffer); | ||
|
|
||
| rx_queue = rxQueue; | ||
|
|
There was a problem hiding this comment.
Perhaps we can just allocate the rx queues locally. See Nathaniel's CAN driver for reference, but he has the little macro that allows the user to define what addresses and size for each address that they require, and then it creates the rx queues in the preprocessor step. Not sure about it though, please provide your thoughts or questions.
BSP/Inc/UART.h
Outdated
| #ifndef INC_UART_H_ | ||
| #define INC_UART_H_ |
There was a problem hiding this comment.
nitpick. Just have it as UART_H
BSP/Src/UART.c
Outdated
| #include "UART.h" | ||
|
|
||
|
|
||
| #define QUEUE_LENGTH 128 |
There was a problem hiding this comment.
Allow this to be overwritten, like in the CAN driver
BSP/Src/UART.c
Outdated
| static UART_HandleTypeDef huart4_ = {.Instance = UART4}; | ||
| UART_HandleTypeDef* huart4 = &huart4_; | ||
|
|
||
| // Single TX queue | ||
| static StaticQueue_t tx_queue_buffer; | ||
| static uint8_t tx_queue_storage[QUEUE_LENGTH * QUEUE_ITEM_SIZE]; | ||
| static QueueHandle_t tx_queue; | ||
|
|
||
| // RX queue (provided by user) | ||
| static QueueHandle_t* rx_queue; | ||
|
|
||
|
|
||
| static bool initialized = false; |
There was a problem hiding this comment.
Will need these for each UART interface initialized
BSP/Src/UART.c
Outdated
| // Setup interrupts | ||
| HAL_NVIC_SetPriority(UART4_IRQn, 5, 0); | ||
| HAL_NVIC_EnableIRQ(UART4_IRQn); |
There was a problem hiding this comment.
Needs to be in the MSP function
|
Can you clean this up by:
|
Key considerations:
Data in UART should be tied with a Bus since UART doesn't use 'id's like other peripherals do.
Would also need to include error handling, e.g. dropped frames for UART RX