Soliciting suggestions on improving a way to use the HAL libraries in a moderately object oriented C++ app

[magene]

I've got a lot of help from people on this forum getting up to speed with STM32 controllers and the STM32 HAL libraries. Coming from the C# world, I've been kind of stubborn about trying to maintain a moderately object oriented architecture. That's been a really painful learning experience and I've got a long way to go but I've finally gotten to the point where I have something working well enough to move forward with. As an example, here's how I've combined my UARTs class with the HAL libraries and I'd value any input from this forum on how to improve it.

For background, there are 2 devices right now that need a UART, GPS1 and CTD1. Eventually, I know there will be 8 UART devices and probably more in the future. I'm trying to implement a non-blocking architecture so I use the HAL_UART_Receive_IT, ...Transmit_IT, ...Receive_DMA and ...Transmit_DMA functions. There's a class for each of the devices. All that happens in those classes is they call their respective initGPS1() or initCTD1() functions and then when they need to transmit something and receive the response, they call their ctd1RX... or GPSRX.. function. Right now, I'm erring on the side of keeping things specific at the expense of having several functions that look a lot like each other. If that becomes a problem, I can think about generalizing later.

Thanks in advance - Gene

 //UARTs.hpp
#pragma once
#include <stm32h7xx_hal.h>
#include <stm32h7xx_hal_tim.h>
 
#include <iostream>
using namespace std;
 
#include "main.hpp"
#include "FIFO.hpp"
 
extern "C" void HAL_UART_RxCpltCallback(UART_HandleTypeDef *huart);
 
static uint8_t gps1MsgBuffer[128];
static uint8_t gps1MsgBufferIndex = 0;
static uint8_t gps1RXBuffer[32];
 
static uint8_t ctd1MsgBuffer[32];
static uint8_t ctd1MsgBufferIndex = 0;
static uint8_t ctd1RXBuffer[32];
 
static MainFIFORecordIDs gps1RecordID = undefined;
static MainFIFORecordIDs ctd1RecordID = undefined;
 
class UARTs
{
public:	
	UARTs()
	{
	}
 
	void initGPS1_UART();
	void initCTD1_UART();
	void gps1RXitTXDMA(uint8_t cmd[], size_t size, MainFIFORecordIDs gpsRecordID);
	void ctd1RXitTXit(uint8_t cmd[], size_t size, MainFIFORecordIDs ctdRecordID);
 
};
 
//UARTs.cpp
#include "UARTs.hpp"
 
static UART_HandleTypeDef gps1UART = UART_HandleTypeDef();
static DMA_HandleTypeDef gps1HDMA_tx;
static DMA_HandleTypeDef gps1HDMA_rx;
 
static UART_HandleTypeDef ctd1UART = UART_HandleTypeDef();
static DMA_HandleTypeDef ctd1HDMA_tx;
static DMA_HandleTypeDef ctd1HDMA_rx;
 
extern "C" void USART1_IRQHandler()
{
	HAL_UART_IRQHandler(&gps1UART);
}
 
extern "C" void USART2_IRQHandler()
{
	HAL_UART_IRQHandler(&ctd1UART);
}
 
extern "C" void DMA1_Stream1_IRQHandler()
{
	HAL_DMA_IRQHandler(&gps1HDMA_tx);
}
 
void HAL_UART_TxCpltCallback(UART_HandleTypeDef *huart)
{
	HAL_GPIO_WritePin(GPIOE, GPIO_PIN_1, GPIO_PIN_RESET);
}
 
extern "C" void DMA1_Stream0_IRQHandler()
{
	HAL_DMA_IRQHandler(&gps1HDMA_rx);
}
 
void HAL_UART_RxCpltCallback(UART_HandleTypeDef *huart)
{
	if (huart == &gps1UART)
	{
		//cout << "GPS1 buffer index: " << gps1MsgBufferIndex << "   GPS1 Msg Buffer: " << gps1MsgBuffer << endl;
		HAL_UART_Receive_IT(&gps1UART, gps1RXBuffer, 1);
		gps1MsgBuffer[gps1MsgBufferIndex] = gps1RXBuffer[0];
		gps1MsgBufferIndex++;
	
		if (gps1RXBuffer[0] == 10)
		{
			gps1MsgBufferIndex = 0;
			enqueueMainFIFO(getSysTickCounter(), gps1RecordID, gps1MsgBuffer);		
		}
	}
	
	if (huart == &ctd1UART)
	{
		//cout << "CTD1 buffer index: " << ctd1MsgBufferIndex << "   CTD1 Msg Buffer: " << ctd1MsgBuffer << endl;
		HAL_UART_Receive_IT(&ctd1UART, ctd1RXBuffer, 1);
		ctd1MsgBuffer[ctd1MsgBufferIndex] = ctd1RXBuffer[0];
		ctd1MsgBufferIndex++;
	
		//TODO maybe deal with unexpected messages here by making rxtxInProgress visible?
		if(ctd1RXBuffer[0] == 10)
		{
			ctd1MsgBufferIndex = 0;
			enqueueMainFIFO(getSysTickCounter(), ctd1RecordID, ctd1MsgBuffer);		
		}
	}
}
 
void UARTs::gps1RXitTXDMA(uint8_t cmd[], size_t size, MainFIFORecordIDs id)
{
	gps1RecordID = id;
	HAL_UART_Receive_IT(&gps1UART, gps1RXBuffer, 1);
	HAL_UART_Transmit_DMA(&gps1UART, (uint8_t*)cmd, size);  
}
 
void UARTs::ctd1RXitTXit(uint8_t cmd[], size_t size, MainFIFORecordIDs id)
{
	ctd1RecordID = id;
	HAL_UART_Receive_IT(&ctd1UART, ctd1RXBuffer, 1);
	HAL_UART_Transmit_IT(&ctd1UART, (uint8_t*)cmd, size); 
}
 
void UARTs::initGPS1_UART()
{
	__USART1_CLK_ENABLE();
	__GPIOB_CLK_ENABLE();
	
	GPIO_InitTypeDef uart1GPIO_InitStructure;
 
	uart1GPIO_InitStructure.Pin = GPIO_PIN_6;
	uart1GPIO_InitStructure.Mode = GPIO_MODE_AF_PP;
	uart1GPIO_InitStructure.Alternate = GPIO_AF7_USART1;
	uart1GPIO_InitStructure.Speed = GPIO_SPEED_HIGH;
	uart1GPIO_InitStructure.Pull = GPIO_NOPULL;
	HAL_GPIO_Init(GPIOB, &uart1GPIO_InitStructure);
	
	uart1GPIO_InitStructure.Pin = GPIO_PIN_7;
	uart1GPIO_InitStructure.Mode = GPIO_MODE_AF_OD;
	HAL_GPIO_Init(GPIOB, &uart1GPIO_InitStructure);
 
	gps1UART.Instance = USART1;
	gps1UART.Init.BaudRate = 9600;
	gps1UART.Init.WordLength = UART_WORDLENGTH_8B;
	gps1UART.Init.StopBits = UART_STOPBITS_1;
	gps1UART.Init.Parity = UART_PARITY_NONE;
	gps1UART.Init.HwFlowCtl = UART_HWCONTROL_NONE;
	gps1UART.Init.Mode = UART_MODE_TX_RX;
 
	if (HAL_UART_Init(&gps1UART) != HAL_OK)
		asm("bkpt 255");
	
	NVIC_EnableIRQ(USART1_IRQn);
		
	__HAL_RCC_DMA1_CLK_ENABLE();		
		
	gps1HDMA_tx.Instance                 = DMA1_Stream1;
	gps1HDMA_tx.Init.Direction           = DMA_MEMORY_TO_PERIPH;
	gps1HDMA_tx.Init.PeriphInc           = DMA_PINC_DISABLE;
	gps1HDMA_tx.Init.MemInc              = DMA_MINC_ENABLE;
	gps1HDMA_tx.Init.PeriphDataAlignment = DMA_PDATAALIGN_BYTE;
	gps1HDMA_tx.Init.MemDataAlignment    = DMA_MDATAALIGN_BYTE;
	gps1HDMA_tx.Init.Mode                = DMA_NORMAL;
	gps1HDMA_tx.Init.Priority            = DMA_PRIORITY_LOW;
	gps1HDMA_tx.Init.FIFOMode            = DMA_FIFOMODE_DISABLE;
	gps1HDMA_tx.Init.FIFOThreshold       = DMA_FIFO_THRESHOLD_FULL;
	gps1HDMA_tx.Init.MemBurst            = DMA_MBURST_INC4;
	gps1HDMA_tx.Init.PeriphBurst         = DMA_PBURST_INC4;
	gps1HDMA_tx.Init.Request             = DMA_REQUEST_USART1_TX;
 
	HAL_DMA_Init(&gps1HDMA_tx);
	__HAL_LINKDMA(&gps1UART, hdmatx, gps1HDMA_tx);
		
	/* Configure the DMA handler for reception process */
	gps1HDMA_rx.Instance                 = DMA1_Stream0;
	gps1HDMA_rx.Init.Direction           = DMA_PERIPH_TO_MEMORY;
	gps1HDMA_rx.Init.PeriphInc           = DMA_PINC_DISABLE;
	gps1HDMA_rx.Init.MemInc              = DMA_MINC_ENABLE;
	gps1HDMA_rx.Init.PeriphDataAlignment = DMA_PDATAALIGN_BYTE;
	gps1HDMA_rx.Init.MemDataAlignment    = DMA_MDATAALIGN_BYTE;
	gps1HDMA_rx.Init.Mode                = DMA_NORMAL;
	gps1HDMA_rx.Init.Priority            = DMA_PRIORITY_HIGH;
	gps1HDMA_rx.Init.FIFOMode            = DMA_FIFOMODE_DISABLE;
	gps1HDMA_rx.Init.FIFOThreshold       = DMA_FIFO_THRESHOLD_FULL;
	gps1HDMA_rx.Init.MemBurst            = DMA_MBURST_INC4;
	gps1HDMA_rx.Init.PeriphBurst         = DMA_PBURST_INC4;
	gps1HDMA_rx.Init.Request             = DMA_REQUEST_USART1_RX;
 
	HAL_DMA_Init(&gps1HDMA_rx);
	__HAL_LINKDMA(&gps1UART, hdmarx, gps1HDMA_rx);
		
	/* NVIC configuration for DMA transfer complete interrupt (USART1_RX) */
	HAL_NVIC_SetPriority(DMA1_Stream0_IRQn, 0, 1);
	HAL_NVIC_EnableIRQ(DMA1_Stream0_IRQn);
		
	/* NVIC configuration for DMA transfer complete interrupt (USART1_TX) */
	HAL_NVIC_SetPriority(DMA1_Stream1_IRQn, 0, 0);
	HAL_NVIC_EnableIRQ(DMA1_Stream1_IRQn);
}
 
void UARTs::initCTD1_UART()
{
	__USART2_CLK_ENABLE();
	__GPIOD_CLK_ENABLE();
	
	GPIO_InitTypeDef usart2GPIO_InitStructure;
 
	usart2GPIO_InitStructure.Pin = GPIO_PIN_5;
	usart2GPIO_InitStructure.Mode = GPIO_MODE_AF_PP;
	usart2GPIO_InitStructure.Alternate = GPIO_AF7_USART2;
	usart2GPIO_InitStructure.Speed = GPIO_SPEED_HIGH;
	usart2GPIO_InitStructure.Pull = GPIO_NOPULL;
	HAL_GPIO_Init(GPIOD, &usart2GPIO_InitStructure);
	
	usart2GPIO_InitStructure.Pin = GPIO_PIN_6;
	usart2GPIO_InitStructure.Mode = GPIO_MODE_AF_OD;
	HAL_GPIO_Init(GPIOD, &usart2GPIO_InitStructure);
 
	ctd1UART.Instance = USART2;
	ctd1UART.Init.BaudRate = 9600;
	ctd1UART.Init.WordLength = UART_WORDLENGTH_8B;
	ctd1UART.Init.StopBits = UART_STOPBITS_1;
	ctd1UART.Init.Parity = UART_PARITY_NONE;
	ctd1UART.Init.HwFlowCtl = UART_HWCONTROL_NONE;
	ctd1UART.Init.Mode = UART_MODE_TX_RX;
 
	if (HAL_UART_Init(&ctd1UART) != HAL_OK)
		asm("bkpt 255");
	
	NVIC_EnableIRQ(USART2_IRQn);
}

[Javier1]

So youre just wrapping C written HAL libraries in C++ classes?

>I'm trying to implement a non-blocking architecture

Have you tried out any RTOS? maybe the functionality your looking for could be implemented with dynamically created RTOS tasks.

>There's a class for each of the device

Only if you have plenty of SRAM to spare... i wouldnt like to be you when SRAM sectors start to override themselves and behave funky.

[Paul1 · Accepted solution]

I'd go further and set some hard rules/guidelines for Constructors/destructors, as this is embedded with way more restrictions and physical limits than PC coding, especially as products often have to run "forever" without any operator intervention.

* We prefer to keep constructors/destructors blank.

This is because the constructors may be called either before main() or after, depending on how you declare your variables (inside/outside of functions, etc.).

Instead we add "init" functions to every class so we can visibly control where/when those are called.

i.e. If you get this wrong and your constructor has an alloc you may run out of memory over time, horrible for embedded systems.

** Actually I hate using alloc in embedded systems. I prefer to compile time define the memory using arrays/structs. If needed then provide pointers to that predefined memory, or allocate indexes into memory structs. This removes memory leaks.

• Sure proper code shouldn't have memory leaks, but we are talking real life and real people.

Paul

[Piranha]

If classes are designed only for a single instance objects, then it's not an object oriented code at all. Ironically your C++ code is not object oriented, while HAL's C code is...

[magene]

@Javier Muñoz​ I have looked at RTOSs but to be honest, the non-blocking state machine pattern with asynchronous FIFOs for device to state machine communication is providing all the functionality I need and looks like it will be extensible to cover my foreseeable future needs. Memory is one of my concerns. The uController board I'm designing has a fair amount of external RAM and FLASH but I'll be paying attention to memory as I flesh out the app. I need to do some tests to see how much of a resource penalty I'm paying with an object oriented approach.

I am just wrapping the HAL IRQ calls into my UARTs class. That's the only way I could make it work with my current state of knowledge. If there's a better way to do it I'd be overjoyed to hear about it.

@Community member​ Excellent advice. Most of my classes do have an init function but not all and I'm not rigorous about leaving the constructors/destructors blank. I'll add that to my list of coding rules. And so far, I haven't had to do any explicit alloc/malloc or similar. And I try to restrict all my "new" calls to app startup. And I"m trying to figure out how to use the embedded template library, ETL https://www.etlcpp.com/ , so there isn't any, or at least minimal, dynamic memory allocation under the hood.

Thanks to both of you for your advice.

[Javier1]

>Memory is one of my concerns

It should be.

>I need to do some tests to see how much of a resource penalty I'm paying with an object oriented approach.

That sounds like a very sensible thing to do, it looks like you dont have a very tight timeline .

@magene​ i confess im just an engineer learning how to code, the only time ive used C++ for embedded has been using arduino. (with stm32duino and teensys)

The rest of times i use c++ is strictly within PC realms (Qt).

In my experience Debugging Object oriented C++ in embedded is exponentially hellish than C, maybe is lack of experience.... time will tell.

[Paul1]

C++: We've gotten into using cpp files even for our C code so as to take advantage of the stronger type checking of C++. It helps detect some bugs/errors at the compile stage.

[magene]

This is an interesting and helpful discussion. A few more points. I have one big embedded project under my belt and that was with C#. I know that's weird but it made sense at the time and worked out really well. We used a GHI Electronics G400S SOM with 128 MB of RAM and 4 MB of Flash. The extra memory was a huge help in allowing me to make progress quickly and I'm putting a fair amount of external memory on our new STM32H7 SOM. Once I figured out how to take advantage of a few basic object oriented techniques in C#, I became a believer. Classes that represent things (motor controllers, instruments, GPS receivers, states in my state machine, etc), inheritance, encapsulation, and the few other OO techniques I use made really intuitive sense to me once I figured out the C# semantics.

While it has been an effort to learn the C++ semantics, it still seems well worth the struggle to me. What I'm amazed at is the size, complexity and what I would consider poor architecture of the STM HAL library. I don't think that's just my opinion, that seems to be the majority opinion of the STM community. The fact that the HAL libraries can't easily be incorporated in an OO app strikes me as pretty archaic. A simple example is dealing with a UART. Seems like a competent C++ programmer at ST Micro could write an example app with an effective HAL based UART class that could be instantiated in a C++ app every time the app needed one of the UARTs. An example program like this could hopefully be used by a not-so-competent C++ programmer like me to build on for other peripherals.

Thanks again for all the help.

[Harvey White]

I have yet to see a concise explanation of why the HAL libraries are done the way they are. However, they do provide most of the functions needed. I do C++ code which then calls the HAL drivers. I do use FreeRTOS, though, which you may not be doing.

That allows me to worry less about whether or not the function is blocking.

However, I do not trust the lockout code in the HAL libraries, so I have isolated the HAL code by using semaphores. They run at two levels. One level isolates the HAL driver (useful for I2C, which may be called by different chip drivers). If a driver is used only by one chip driver, then this semaphore is not needed. The second semaphore protects the chip driver, so it can only respond to one task at a time. This allows FreeRTOS tasks to access the resource in an orderly manner.

Since C++ can run C code without a problem, all of the C code (where possible) is accessed that way.

I do not recommend using the constructor and destructor methods in the C++ classes, since the code may be called before any part of main.c is run. If main.c sets up (say) QSPI memory and validates it, and the classes already initialized by constructor code use that QSPI memory, you have a problem.