STMicroelectronics / STM32CubeL4

STM32Cube MCU Full Package for the STM32L4 series - (HAL + LL Drivers, CMSIS Core, CMSIS Device, MW libraries plus a set of Projects running on all boards provided by ST (Nucleo, Evaluation and Discovery Kits))
Other
262 stars 153 forks source link

const as much as possible #17

Closed CanastraRF closed 3 years ago

CanastraRF commented 3 years ago

const should us as much as possible.

example 1: HAL_StatusTypeDef IRDA_SetConfig(IRDA_HandleTypeDef *hirda) could be modified to HAL_StatusTypeDef IRDA_SetConfig(IRDA_HandleTypeDef * const hirda)

this protect hirda to be modified inside the function.

example 2: uint32_t HAL_COMP_GetOutputLevel(COMP_HandleTypeDef *hcomp) could be modified to uint32_t HAL_COMP_GetOutputLevel(COMP_HandleTypeDef const * const hcomp)

this protect hcomp and the destination of hcomp to be modified inside the function.

example 3: HAL_StatusTypeDef HAL_DAC_SetValue(DAC_HandleTypeDef *hdac, uint32_t Channel, uint32_t Alignment, uint32_t Data); could be modified to HAL_StatusTypeDef HAL_DAC_SetValue(DAC_HandleTypeDef * const hdac, uint32_t const Channel, uint32_t const Alignment, uint32_t const Data);

please improve the library

atsju commented 3 years ago

@CanastraRF @ALABSTM Starting with example 3 which is the most strange. I've never seen a prototype likevoid function(const uint32_t variable);. This is because it is very heavy to add no information. If variable is modified inside the function then the compiler will not modify the variable externally. So we do not add const in this case.

For exact same reason it seems example 1 adds no information. So I don't think const should be added.

Exception for example 2 where the first const has a meaning. (see #15 and #17)

CanastraRF commented 3 years ago

@atsju We are working with Misra Rules and check our code with pclint. pclint reports when a function argument is not modified and not const. So we use const as much as possible. This protect the developer to modify the wrong variable. example C++

void (int & i1, int const & i2)
{
  i1 = i2;  //correct
  i2 = i1;  //error
}

this technic can find bugs earlier.

atsju commented 3 years ago

@CanastraRF I fully agree with the example in the last comment. This is also subject of tickets #15 and #17.

I am concerned about what is the interest of void function(int const i1) and void function(int * const p1) ? Maybe I just do not know the reason but it seams heavy notation with no additional information.

Hish15 commented 3 years ago

@CanastraRF , I agree 100% with you on pointers and references, but in one of your examples you put a const before a uint32_t parameter!

Is that the rule you are speaking about?

RKOUSTM commented 3 years ago

Hi all of you,

Thank you for your report @CanastraRF. This issue has also been previously reported by other users (link). This is one of the main points in the tasks queue of our development teams. They said they have planned a global update of the HAL and LL drivers since a while.

However, this is likely to require some time to be deployed. We cannot share a date regarding its deployment and publication. We count on your patience and comprehension.

Thank you again for this report.

With regards,

RKOUSTM commented 3 years ago

Hi @CanastraRF,

First, allow me to thank you for your contribution. The final decision of our technical committee is no plan to add a const inside the function. The reported point is already highlighted during the MISRA-C 2012 compliance rework (Rule-8.13). It has been decided, from the quality point of view, to derogate this rule as fixing it will break the compatibility with almost all HAL APIs and then update all examples and applications within the Cube deliverable and also on user application side.

Please, allow me to close this issue. Thank you for your comprehension.

With regards,

CanastraRF commented 3 years ago

Dear Support

Const not always break the API. An argument can always convert to const but not back. void foo(int i); and void foo(const int i); result in the same code. This is also true for void foo(handle * h) and void foo(handle * const h) But the improvement with const is that h cannot modify by mistake.

void foo(handle * const h)
{
    h++;    //Compiler report an error
    (*h)++; // is possible
}

So, most of pointer should not modified at all. There is a difference between void foo(handle * const h) and void foo(handle const * const h) A get function should not modify the object. With (handle const const h) the compiler can protect this. A set function should modify the object. With (handle const h) the compiler can check this. The improvement of (handle const const h) is also that a literal can be assigned. Example `HAL_StatusTypeDef HAL_UART_Transmit(UART_HandleTypeDef huart, uint8_t pData, uint16_t Size, uint32_t Timeout); Here an Object from flash cannot transmit directly. It must first copy to Ram. HAL_StatusTypeDef HAL_UART_Transmit(UART_HandleTypeDef const huart, uint8_t const pData, uint16_t Size, uint32_t Timeout); huart is no protected from modify by mistake. pData can now be in Flash. Most of time the arguments are copied to internal data in huart. So, the API can change to HAL_StatusTypeDef HAL_UART_Transmit(UART_HandleTypeDef const huart, uint8_t const * const pData, uint16_t const Size, uint32_t const Timeout);` Without any API break. So please add const to all pointer that should not modified at all. Also add const to object in transmit function to allow transmit object from flash. Thanks Reto Felix

atsju commented 3 years ago

Hi @Hish15 I think @CanastraRF refers to MISRA C++ Rule 7-1-1. From older thread I have seen it seems he requires to respect some safety standards and runs PC-lint. Having the code "good" from start requires less deviation documentation.

Hi @CanastraRF First let me say all agree of the the need for int16_t const * var. Second I suppose ST will follow same idea as CMSIS issue. This means respecting MISRA-C for C code and not writing any int16_t const var. You are referring to 2 different MISRA rules (without pointing them out) so this could be treated as 2 different issues.

Regards, atsju

ALABSTM commented 1 year ago

Duplicate of #15