C-Library const-correctness for STM32F1xx

[Brian H]

I think const-correctness is an important-enough concept to call this a bug report.

In the HAL Driver for STM32F1xx, much of the codebase is not const-correct.  For G4xx, I see that I2C is not.

What is const-correctness in C?

If a function that accepts a pointer to some data doesn't change that data, the function can tell the calling context that fact by declaring the buffer pointer to be const, like so:

void i_do_not_change_the_data(const void * data_ptr);

Then the calling context can be sure that the function will not attempt to write to the buffer, and can take advantage of compiler and language optimizations surrounding const arguments.  One such (poor) example is a string literal, which can serve as a const char * argument, but not a char * argument.

Most (all?) of the HAL functions that take pointers to buffers take non-const pointers, even if those functions don't write to the buffer.  That means that I cannot write my own code to be const-correct without casting away the constness, exposing my code to undefined behaviors.

Unfortunately, this problem runs extremely deeply in the library code, all the way down to HAL struct members.  TX buffers in particular should be declared as const *, but they're not, so const pointers can't be assigned to them and the APIs can't be corrected.

This needs an intense application of effort to correct.  If ST intends its HAL libraries to be suitable for production code, this really ought to be fixed.

All pointer arguments to functions should be const unless they can't be.  Const-ness should be a forethought, not an afterthought.

[Pavel A.]

>  If ST intends its HAL libraries to be suitable for production code

That's a big if.

 

[TDK]

> TX buffers in particular should be declared as const *, but they're not, so const pointers can't be assigned to them and the APIs can't be corrected.

Most buffers are declared this way. For example (F4 SPI as a randomly chosen example):

const uint8_t *pTxBuffPtr; /*!< Pointer to SPI Tx transfer Buffer */

and the calling function takes a pointer to const:

HAL_StatusTypeDef HAL_SPI_Transmit(SPI_HandleTypeDef *hspi, const uint8_t *pData, uint16_t Size, uint32_t Timeout);

 

Since most functions are changing the peripheral state in some way, it makes sense for most of them to take non-const handles. Things that are read-only take a const handle. For example:

HAL_SPI_StateTypeDef HAL_SPI_GetState(const SPI_HandleTypeDef *hspi);

 

 

Are there specific other examples where you think this should be looked at? Certainly the intention is already there to make things const-correct.

[Brian H]

Not for me.  From "stm32f1xx_hal_spi.h" as found in the project I'm working on now:

 

 

HAL_StatusTypeDef HAL_SPI_Transmit(SPI_HandleTypeDef *hspi, uint8_t *pData, uint16_t Size, uint32_t Timeout);

HAL_StatusTypeDef HAL_SPI_TransmitReceive(SPI_HandleTypeDef *hspi, uint8_t *pTxData, uint8_t *pRxData, uint16_t Size, uint32_t Timeout);

 

 

The one I encountered that "triggered" me, so to speak, is from the USB CDC implementation:

 

uint8_t CDC_Transmit_FS(uint8_t* Buf, uint16_t Len);

 

So maybe it varies across platforms.

Edit: I just checked the H5xx project I have, and it's much better.  So clearly it is inconsistent across the processor families.

[TDK]

> Not for me.  From "stm32f1xx_hal_spi.h" as found in the project I'm working on now:

Might want to update your library then. It's const in the latest version. Looks like it may have been updated 7 months ago.

stm32f1xx-hal-driver/Inc/stm32f1xx_hal_spi.h at 9577d269eed0a141a7e339adcaf434c325738b99 · STMicroelectronics/stm32f1xx-hal-driver

 

 

The USB code is part of Middlewares, not HAL, and is generally lower quality/robustness. Should be corrected there, but doing a const-cast doesn't cause problems.

[gbm]

That's an old and well-known problem. Also, init structures' pointers are not const-qualified, so any config structure must be variable although it never gets modified.

Besides const there are few other C keywords and constructs unknown to HAL people, like bool type, enum, structure initializers and compound literals. This might be for the sake of C90 or maybe FORTRAN compatibility. ;)

[Saket_Om]

Hello @Brian H 

Our HAL driver is compliant with the rule 8.13 of MISRA C 2012. 

"Rule 8.13:  A pointer should point to a const-qualified type whenever possible"

A const qualifier has been added to pointer argument of HAL API whenever is possible. 

[Brian H]

---

@Saket_Om wrote:

Hello @Brian H 

Our HAL driver is compliant with the rule 8.13 of MISRA C 2012.  

---

No it isn't.  Not in the project I created some time last year against STM32F1xx.  See my reply to TDK with quotes from the stm32f1xx_hal_spi.h file for example.  In fact, I'll attach the entire SPI driver header to this message and you can observe for yourself how the keyword const does not appear anywhere in the file.

In fact, let's see how many times the const keyword appears in the entire include path.  In project_path/Drivers/STM32F1xx_HAL_Driver, I did this:

$ grep -r "\<const\>" > constness.txt

and

$ grep -rc "\<const\>" > constness_count.txt

Too much output for the body of this post, but take note of how many files have zero uses of the const keyword (in constness_count.txt).

So, no.  HAL Driver for STM32F1xx is not compliant with Rule 8.13.

In case it's a version issue (maybe you guys JUST fixed it), tell me how to check what version of the library is installed and I will (I can't find it).

[Brian H]

Note: Since the const-correctness of HAL Driver clearly varies across supported processor families, I've edited my original post to be more specific to the families where I've directly observed the problem.

I think it would be good for STM to audit the entire set of HAL Driver packages across all supported processor families to ensure none are missed.

To summarize, for the processors I've worked with lately:

STM32H5xx - good const-correctness (as far as I looked, which was not exhaustive)

STM32G4xx - bad const-correctness

STM32F1xx - bad const-correctnes

Clearly the concept is understood, but the implementation is incomplete.