ST's (STM32Cube) software ecosystem is terrible - how can we fix it?

[Sal Ammoniac]

When we wrote drivers, we only wrote code that addressed the specific features we needed on the specific MCU we were using. Our code was cleaner, neater, and considerably more bug-free than any vendor library code I've ever seen.

How many man months did it take? How much did it cost? How do you know for certain it has less bugs than the vendor code if you not even used it in the first place?
I am not saying it was not a good decision for your example since i do not know the facts but i have been a SW engineer for over 20 years and I have met lots and lots of colleagues that are suffering from the so called "not written by me or us" syndrome that costs companies an enormous amount of money and in the end does not always deliver a better end result.

It probably cost less than using a buggy vendor library would have. I'm willing to bet that our code had fewer bugs than typical vendor code. We did thorough code reviews with people familiar with our hardware and our application--I doubt you can prove to me that generic open source code perused by Larry, Daryl, and Daryl out on the Internet is in any way better.

[dannyf]

" ome on, if you do not use a certain peripheral you don,t include it in your project, it costs nothing"

I think this fear of "excessive" code come from people who are stuck in the stone ages. Modern compilers have the ability to trim unused code from the executable, and such ability has existed for decades.

A typical hobby project for me includes my application specific code, my own library, vendor library and my own substitutes, and cmsis for for the arm chips. The libraries covers all peripherals whether I use them or not - I don't want to spend my time trimming the code when the computer can do it for me.

A work project would be quite similar.

[dannyf]

" It's often been speculated here that vendor libraries are written by summer interns"

It is probably not terribly smart to make decisions on speculation shared with you by unknown strangers on the internet.

[westfw]

Modern compilers have the ability to trim unused code from the executable

One of the specific complains against the vendor libraries is that they've been written in a way that makes this sort of code-trimming nearly impossible.
Fancy macros?  Nope.  Static inline functions that rely/count on constant parameters cause most of the code to "drop out"?  Nope.  Separate functions for wildly separate modes?  Hmm - maybe (it looks like Cube/HAL could be a bit better at that than some other libraries I've seen (looking at the UART/USART code.  The RCC (clock control) code is still really sucky, with a single OscConfig() function that handles ALL possible clock setups...)

People aren't looking at the source and saying "OMG this is so huge!", they're looking at their binaries, scratching their heads "Why is this so big?", and THEN looking at the source code and gagging.   The lack of some of the "tricks" mentioned above is also one of the clues leading to the "written by interns" suggestions - too many school-like "mannerisms" and not enough signs that the author has any experience writing code for smallish systems.


(there was an earlier thread where I cut down code size significantly just by #including the C code instead of (in addition to) the .h files (allowing the compiler to "see" more optimization possibilities.)  IIRC, that was with the older ST "Standard peripheral library", though.) (Umm...  https://www.eevblog.com/forum/microcontrollers/stm32-ghetto-style/msg522553/#msg522553 )

[westfw]

BTW, it IS really nice to have the source code of the various vendor libraries to look at, even if I don't like it.  It's probably even better to have the several different versions to look at...)

[Kjelt]

I don't need, I'm talking about drivers for peripherals that support every single feature that peripheral has, including ones that I don't use. It's hard to exclude that code because a lot of the vendor libraries I've seen have large functions that handle every single option a peripheral device has, often in a huge switch statement.

So to put it sarcastically: actually you are complaining that a peripheral has too many functionality that you don't need/use but then blame the vendor for supporting all these functions?
Either you have less functionality (different uC, different peripherals) or you support what is available from a vendor POV.

Yes sure you can write your own peripheral or just delete the part of the "spaghetti" you don't use but as Danny said the compiler can do this for you.
The risk you take is that you have a non-feature complete peripheral driver.and that a few years from now you do need some of that peripheral functionality you have not supported and someone in the company again has to dive in the datasheets to see how this is done. Probably the person who skipped it in the first place and at that time does not have room in his agenda to again spent time on this
A good practice if you want company (vendor independent) HAL is to write a company defined glue layer on top of the vendors HAL, but that will cost additional codesize.

It's often been speculated here that vendor libraries are written by summer interns. Based on most of this code I've seen, I believe it. While there may be exceptions that I haven't seen, what I have seen I would classify as utter crap.

Sure and risk the companies net worth on a couple of interns? That sounds what a billion$ company would do 
No, what I could believe is that some of the early libraries from ST were build by HW engineers that have learned to write software instead of software engineers. Probaly the same team as responsible for part of the HW itself. The good thing is that at least they know how their peripheral should be configured to operate as expected so you can use at least this code as example.
BTW I have also seen HALs from experienced software engineers where most HW engineers/embedded SW engineers could not work with. To give an example we had this new uC prototype from the vendor (so nothing of any kind of SW available)  and it had tiny ROM so each byte counted.
What a 20 yr experienced EE educated embedded SW engineer did was write the entire peripheral/clock/interrupt configuration in a very elaborate X-macro.
Actually this was mis using the X-macro principle to its fullest but hey the result was tiny!
Now this worked great if you did exactly how it was supposed to work.
Then came these SW educated SW engineers and all they did was complain because they could not understand the concept of the X-macro, the SCA tool could not understand it and if you made a tiny mistake (a ,  or ; missed) the whole x-macro would explode but did not show exactly where the problem occurred in that macro. So brilliant, but a nightmare to work with unless you knew exactly what you were doing.

IMO SW is a field of work where you can bitch about much more than HW.
In SW it is very easy to change things, get used to a specific style of coding you personally prefer and if you put 5 SW engineers in a room and tell them to write a simple program they all do it their way and if you ask them to choose the best one there will be a catfight. There are lots of "best practices" some contradicting eachother, newer insights and on and on. If you are used of working in a 30 man SW team you are tired of the coffee corner discussions about this. 

[Kjelt]

It probably cost less than using a buggy vendor library would have. I'm willing to bet that our code had fewer bugs than typical vendor code.

As said all code has bugs, the statistical chance that a bug will be found, reported and fixed in an open source software project is many times larger than in own closed source code unless the company has a strict code analysis and review strategy in place.

We did thorough code reviews with people familiar with our hardware and our application--I doubt you can prove to me that generic open source code perused at by Larry, Daryl, and Daryl out on the Internet is in any way better.

Neither can you prove that yours is better since there is no absolute way of comparing.
I can ask you some SW quality questions that give me a good gutfeeling about your code quality, for instance do you Lint the code and run additional SCA tools on your code like HP Fortify or Coverity?

Anyway we are living in a century where a single company is not able to produce all its software itself neither is it necessary. We don't write our own email and wordprocessor software now do we? Why because some other company does that for us. So why would you for instance still write your own TCP/IP stack or TLS stack. You buy it or take an opensource version like OpenSSL. The most important thing left and forgotten by a lot of companies becomes SW lifecycle management, you should keep track of found bugs, updates on parts of the code and update your product.

[richardman]

For an example of ST's HAL code, I give you this ... well, take a look:

#define __HAL_DMA_GET_TC_FLAG_INDEX(__HANDLE__) \
(((uint32_t)((__HANDLE__)->Instance) == ((uint32_t)DMA1_Stream0))? DMA_FLAG_TCIF0_4 :\
((uint32_t)((__HANDLE__)->Instance) == ((uint32_t)DMA2_Stream0))? DMA_FLAG_TCIF0_4 :\
((uint32_t)((__HANDLE__)->Instance) == ((uint32_t)DMA1_Stream4))? DMA_FLAG_TCIF0_4 :\
((uint32_t)((__HANDLE__)->Instance) == ((uint32_t)DMA2_Stream4))? DMA_FLAG_TCIF0_4 :\
((uint32_t)((__HANDLE__)->Instance) == ((uint32_t)DMA1_Stream1))? DMA_FLAG_TCIF1_5 :\
((uint32_t)((__HANDLE__)->Instance) == ((uint32_t)DMA2_Stream1))? DMA_FLAG_TCIF1_5 :\
((uint32_t)((__HANDLE__)->Instance) == ((uint32_t)DMA1_Stream5))? DMA_FLAG_TCIF1_5 :\
((uint32_t)((__HANDLE__)->Instance) == ((uint32_t)DMA2_Stream5))? DMA_FLAG_TCIF1_5 :\
((uint32_t)((__HANDLE__)->Instance) == ((uint32_t)DMA1_Stream2))? DMA_FLAG_TCIF2_6 :\
((uint32_t)((__HANDLE__)->Instance) == ((uint32_t)DMA2_Stream2))? DMA_FLAG_TCIF2_6 :\
((uint32_t)((__HANDLE__)->Instance) == ((uint32_t)DMA1_Stream6))? DMA_FLAG_TCIF2_6 :\
((uint32_t)((__HANDLE__)->Instance) == ((uint32_t)DMA2_Stream6))? DMA_FLAG_TCIF2_6 :\

Could they have written it as a switch? Probably. A jump table? May be. A mapping table? For sure! A 12 nested conditional that depends on all sort of optimizations to make it into efficient code?!! Sure, why not, if you are ST.

[Kjelt]

But what is the problem if it works? You just use the macro with the correct handle and it solves it for you. I have seen worse VHDL code but then it is ok? 
It looks more neat than if you wrote it in a function with al if()'s explicitly but then I am used to macros and a lot of people hate them so that is a personal preference.

[Kjelt]

An alternative would be something like this which you need three parameters and other stuff but if you only want to know the single thing as in your code I could live better with that solution.

Code: [Select]

/**
  * @brief  Checks whether the specified DMAy Streamx flag is set or not.
  * @param  DMAy_Streamx: where y can be 1 or 2 to select the DMA and x can be 0
  *          to 7 to select the DMA Stream.
  * @param  DMA_FLAG: specifies the flag to check.
  *          This parameter can be one of the following values:
  *            @arg DMA_FLAG_TCIFx:  Streamx transfer complete flag
  *            @arg DMA_FLAG_HTIFx:  Streamx half transfer complete flag
  *            @arg DMA_FLAG_TEIFx:  Streamx transfer error flag
  *            @arg DMA_FLAG_DMEIFx: Streamx direct mode error flag
  *            @arg DMA_FLAG_FEIFx:  Streamx FIFO error flag
  *         Where x can be 0 to 7 to select the DMA Stream.
  * @retval The new state of DMA_FLAG (SET or RESET).
  */
FlagStatus DMA_GetFlagStatus(DMA_Stream_TypeDef* DMAy_Streamx, uint32_t DMA_FLAG)
{
  FlagStatus bitstatus = RESET;
  DMA_TypeDef* DMAy;
  uint32_t tmpreg = 0;

  /* Check the parameters */
  assert_param(IS_DMA_ALL_PERIPH(DMAy_Streamx));
  assert_param(IS_DMA_GET_FLAG(DMA_FLAG));

  /* Determine the DMA to which belongs the stream */
  if (DMAy_Streamx < DMA2_Stream0)
  {
    /* DMAy_Streamx belongs to DMA1 */
    DMAy = DMA1;
  }
  else
  {
    /* DMAy_Streamx belongs to DMA2 */
    DMAy = DMA2;
  }

  /* Check if the flag is in HISR or LISR */
  if ((DMA_FLAG & HIGH_ISR_MASK) != (uint32_t)RESET)
  {
    /* Get DMAy HISR register value */
    tmpreg = DMAy->HISR;
  }
  else
  {
    /* Get DMAy LISR register value */
    tmpreg = DMAy->LISR;
  }   
 
  /* Mask the reserved bits */
  tmpreg &= (uint32_t)RESERVED_MASK;

  /* Check the status of the specified DMA flag */
  if ((tmpreg & DMA_FLAG) != (uint32_t)RESET)
  {
    /* DMA_FLAG is set */
    bitstatus = SET;
  }
  else
  {
    /* DMA_FLAG is reset */
    bitstatus = RESET;
  }

  /* Return the DMA_FLAG status */
  return  bitstatus;
}


[dannyf]

" Could they have written it as a switch? "

Sure. But there are many roads one can take. Had it written as a switch, or any other implementation, I'm sure someone somewhere would ask if it could be implemented in their favorite way.

End of the day, we just need to realize that any implementation decision we make is a compromise. Unless you can articulate why the said implementation is bad, it is best to just learn to use it.

In this particular case, the implementation is quite clear to me.

[Kjelt]

Exactly and what you did not show is that there is also comment, which is of the utmost importance and everyone that ever coded knows what it does.

Code: [Select]

/**
66   * @brief  Returns the current DMA Channel transfer complete flag.
67   * @param  __HANDLE__: DMA handle
68   * @retval The specified transfer complete flag index.
69   */


[Sal Ammoniac]

I don't need, I'm talking about drivers for peripherals that support every single feature that peripheral has, including ones that I don't use. It's hard to exclude that code because a lot of the vendor libraries I've seen have large functions that handle every single option a peripheral device has, often in a huge switch statement.

So to put it sarcastically: actually you are complaining that a peripheral has too many functionality that you don't need/use but then blame the vendor for supporting all these functions?

No, I blame the vendor for implementing these libraries as collections of huge, monolithic functions instead of modular code that handles peripheral functionality in small, modular pieces.

Yes sure you can write your own peripheral or just delete the part of the "spaghetti" you don't use but as Danny said the compiler can do this for you.

Sure, the linker can exclude functions in a library that you don't call, but how does it exclude code in a huge monolithic function that implements support for every possible option a peripheral has? How does the compiler know what code will execute at runtime? Answer: it doesn't and it can't. The linker pulls this entire huge function into your binary even if you use just a tiny piece of it. Clock initialization code is a good example. This is typically implemented as a huge function that handles every possible clock configuration option, which on a modern ARM MCU, for example, can run to dozens of different combinations.

The risk you take is that you have a non-feature complete peripheral driver.and that a few years from now you do need some of that peripheral functionality you have not supported and someone in the company again has to dive in the datasheets to see how this is done. Probably the person who skipped it in the first place and at that time does not have room in his agenda to again spent time on this

This is one of the most ridiculous arguements I've ever seen and I'm not going to spend another second addressing it...

Probaly the same team as responsible for part of the HW itself. The good thing is that at least they know how their peripheral should be configured to operate as expected so you can use at least this code as example.

This is another bone I have to pick with MCU vendors. Sometimes you are forced to look at their library source code to figure out how a peripheral works because it's not properly documented in the datasheet, user guide, and/or errata. This drives me nuts and makes me want to immediately look for a better part. Trouble is, it's such a common practice in the industry that it's hard to avoid. 

[Kjelt]

This is one of the most ridiculous arguements I've ever seen and I'm not going to spend another second addressing it...

You will think back of it (and spent time) when someone after a long period asks a new feature or update where you will need a certain configuration of one of the peripherals that was not implemented the first time, been there 
And then you will probably again go through:

Sometimes you are forced to look at their library source code to figure out how a peripheral works because it's not properly documented in the datasheet, user guide, and/or errata.

[westfw]

#define __HAL_DMA_GET_TC_FLAG_INDEX(__HANDLE__) \
(((uint32_t)((__HANDLE__)->Instance) == ((uint32_t)DMA1_Stream0))? DMA_FLAG_TCIF0_4 :\
((uint32_t)((__HANDLE__)->Instance) == ((uint32_t)DMA2_Stream0))? DMA_FLAG_TCIF0_4 :\
((uint32_t)((__HANDLE__)->Instance) == ((uint32_t)DMA1_Stream4))? DMA_FLAG_TCIF0_4 :\
((uint32_t)((__HANDLE__)->Instance) == ((uint32_t)DMA2_Stream4))? DMA_FLAG_TCIF0_4 :\

That would be acceptable, or even desirable, if it's used in a context where __HANDLE__ and __HANDLE__->Instance are constants and the whole expression resolves to a constant at compile time...

But here: I'll throw up the clock initialization function, which I think is really hard to justify...

Code: [Select]

/**
  * @brief  Initializes the RCC Oscillators according to the specified parameters in the
  *         RCC_OscInitTypeDef.
  * @param  RCC_OscInitStruct pointer to an RCC_OscInitTypeDef structure that
  *         contains the configuration information for the RCC Oscillators.
  * @note   The PLL is not disabled when used as system clock.
  * @note   The PLL is not disabled when USB OTG FS clock is enabled (specific to devices with USB FS)
  * @retval HAL status
  */
HAL_StatusTypeDef HAL_RCC_OscConfig(RCC_OscInitTypeDef  *RCC_OscInitStruct)
{
   uint32_t tickstart = 0;
 
  /* Check the parameters */
  assert_param(RCC_OscInitStruct != NULL);
  assert_param(IS_RCC_OSCILLATORTYPE(RCC_OscInitStruct->OscillatorType));
 
  /*------------------------------- HSE Configuration ------------------------*/
  if(((RCC_OscInitStruct->OscillatorType) & RCC_OSCILLATORTYPE_HSE) == RCC_OSCILLATORTYPE_HSE)
  {
    /* Check the parameters */
    assert_param(IS_RCC_HSE(RCC_OscInitStruct->HSEState));
       
    /* When the HSE is used as system clock or clock source for PLL in these cases it is not allowed to be disabled */
    if((__HAL_RCC_GET_SYSCLK_SOURCE() == RCC_SYSCLKSOURCE_STATUS_HSE)
       || ((__HAL_RCC_GET_SYSCLK_SOURCE() == RCC_SYSCLKSOURCE_STATUS_PLLCLK) && (__HAL_RCC_GET_PLL_OSCSOURCE() == RCC_PLLSOURCE_HSE)))
    {
      if((__HAL_RCC_GET_FLAG(RCC_FLAG_HSERDY) != RESET) && (RCC_OscInitStruct->HSEState == RCC_HSE_OFF))
      {
        return HAL_ERROR;
      }
    }
    else
    {
      /* Reset HSEON and HSEBYP bits before configuring the HSE --------------*/
      __HAL_RCC_HSE_CONFIG(RCC_HSE_OFF);
     
      /* Get Start Tick */
      tickstart = HAL_GetTick();
     
      /* Wait till HSE is disabled */ 
      while(__HAL_RCC_GET_FLAG(RCC_FLAG_HSERDY) != RESET)
      {
        if((HAL_GetTick() - tickstart ) > HSE_TIMEOUT_VALUE)
        {
          return HAL_TIMEOUT;
        }
      }
     
      /* Set the new HSE configuration ---------------------------------------*/
      __HAL_RCC_HSE_CONFIG(RCC_OscInitStruct->HSEState);
     

       /* Check the HSE State */
      if(RCC_OscInitStruct->HSEState != RCC_HSE_OFF)
      {
        /* Get Start Tick */
        tickstart = HAL_GetTick();
       
        /* Wait till HSE is ready */
        while(__HAL_RCC_GET_FLAG(RCC_FLAG_HSERDY) == RESET)
        {
          if((HAL_GetTick() - tickstart ) > HSE_TIMEOUT_VALUE)
          {
            return HAL_TIMEOUT;
          }
        }
      }
      else
      {
        /* Get Start Tick */
        tickstart = HAL_GetTick();
       
        /* Wait till HSE is bypassed or disabled */
        while(__HAL_RCC_GET_FLAG(RCC_FLAG_HSERDY) != RESET)
        {
           if((HAL_GetTick() - tickstart ) > HSE_TIMEOUT_VALUE)
          {
            return HAL_TIMEOUT;
          }
        }
      }
    }
  }
  /*----------------------------- HSI Configuration --------------------------*/
  if(((RCC_OscInitStruct->OscillatorType) & RCC_OSCILLATORTYPE_HSI) == RCC_OSCILLATORTYPE_HSI)
  {
    /* Check the parameters */
    assert_param(IS_RCC_HSI(RCC_OscInitStruct->HSIState));
    assert_param(IS_RCC_CALIBRATION_VALUE(RCC_OscInitStruct->HSICalibrationValue));
   
    /* Check if HSI is used as system clock or as PLL source when PLL is selected as system clock */
    if((__HAL_RCC_GET_SYSCLK_SOURCE() == RCC_SYSCLKSOURCE_STATUS_HSI)
       || ((__HAL_RCC_GET_SYSCLK_SOURCE() == RCC_SYSCLKSOURCE_STATUS_PLLCLK) && (__HAL_RCC_GET_PLL_OSCSOURCE() == RCC_PLLSOURCE_HSI_DIV2)))
    {
      /* When HSI is used as system clock it will not disabled */
      if((__HAL_RCC_GET_FLAG(RCC_FLAG_HSIRDY) != RESET) && (RCC_OscInitStruct->HSIState != RCC_HSI_ON))
      {
        return HAL_ERROR;
      }
      /* Otherwise, just the calibration is allowed */
      else
      {
        /* Adjusts the Internal High Speed oscillator (HSI) calibration value.*/
        __HAL_RCC_HSI_CALIBRATIONVALUE_ADJUST(RCC_OscInitStruct->HSICalibrationValue);
      }
    }
    else
    {
      /* Check the HSI State */
      if(RCC_OscInitStruct->HSIState != RCC_HSI_OFF)
      {
       /* Enable the Internal High Speed oscillator (HSI). */
        __HAL_RCC_HSI_ENABLE();
       
        /* Get Start Tick */
        tickstart = HAL_GetTick();
       
        /* Wait till HSI is ready */
        while(__HAL_RCC_GET_FLAG(RCC_FLAG_HSIRDY) == RESET)
        {
          if((HAL_GetTick() - tickstart ) > HSI_TIMEOUT_VALUE)
          {
            return HAL_TIMEOUT;
          }
        }
               
        /* Adjusts the Internal High Speed oscillator (HSI) calibration value.*/
        __HAL_RCC_HSI_CALIBRATIONVALUE_ADJUST(RCC_OscInitStruct->HSICalibrationValue);
      }
      else
      {
        /* Disable the Internal High Speed oscillator (HSI). */
        __HAL_RCC_HSI_DISABLE();
       
        /* Get Start Tick */
        tickstart = HAL_GetTick();
       
        /* Wait till HSI is disabled */
        while(__HAL_RCC_GET_FLAG(RCC_FLAG_HSIRDY) != RESET)
        {
          if((HAL_GetTick() - tickstart ) > HSI_TIMEOUT_VALUE)
          {
            return HAL_TIMEOUT;
          }
        }
      }
    }
  }
  /*------------------------------ LSI Configuration -------------------------*/
  if(((RCC_OscInitStruct->OscillatorType) & RCC_OSCILLATORTYPE_LSI) == RCC_OSCILLATORTYPE_LSI)
  {
    /* Check the parameters */
    assert_param(IS_RCC_LSI(RCC_OscInitStruct->LSIState));
   
    /* Check the LSI State */
    if(RCC_OscInitStruct->LSIState != RCC_LSI_OFF)
    {
      /* Enable the Internal Low Speed oscillator (LSI). */
      __HAL_RCC_LSI_ENABLE();
     
      /* Get Start Tick */
      tickstart = HAL_GetTick();
     
      /* Wait till LSI is ready */ 
      while(__HAL_RCC_GET_FLAG(RCC_FLAG_LSIRDY) == RESET)
      {
        if((HAL_GetTick() - tickstart ) > LSI_TIMEOUT_VALUE)
        {
          return HAL_TIMEOUT;
        }
      }
      /*  To have a fully stabilized clock in the specified range, a software temporization of 1ms
          should be added.*/
      HAL_Delay(1);
    }
    else
    {
      /* Disable the Internal Low Speed oscillator (LSI). */
      __HAL_RCC_LSI_DISABLE();
     
      /* Get Start Tick */
      tickstart = HAL_GetTick();
     
      /* Wait till LSI is disabled */ 
      while(__HAL_RCC_GET_FLAG(RCC_FLAG_LSIRDY) != RESET)
      {
        if((HAL_GetTick() - tickstart ) > LSI_TIMEOUT_VALUE)
        {
          return HAL_TIMEOUT;
        }
      }
    }
  }
  /*------------------------------ LSE Configuration -------------------------*/
  if(((RCC_OscInitStruct->OscillatorType) & RCC_OSCILLATORTYPE_LSE) == RCC_OSCILLATORTYPE_LSE)
  {
    /* Check the parameters */
    assert_param(IS_RCC_LSE(RCC_OscInitStruct->LSEState));
   
    /* Enable Power Clock*/
    __HAL_RCC_PWR_CLK_ENABLE();
   
    /* Enable write access to Backup domain */
    SET_BIT(PWR->CR, PWR_CR_DBP);

    /* Wait for Backup domain Write protection disable */
    tickstart = HAL_GetTick();
   
    while((PWR->CR & PWR_CR_DBP) == RESET)
    {
      if((HAL_GetTick() - tickstart ) > RCC_DBP_TIMEOUT_VALUE)
      {
        return HAL_TIMEOUT;
      }     
    }
   
    /* Reset LSEON and LSEBYP bits before configuring the LSE ----------------*/
    __HAL_RCC_LSE_CONFIG(RCC_LSE_OFF);
   
    /* Get Start Tick */
    tickstart = HAL_GetTick();
   
    /* Wait till LSE is disabled */ 
    while(__HAL_RCC_GET_FLAG(RCC_FLAG_LSERDY) != RESET)
    {
      if((HAL_GetTick() - tickstart ) > RCC_LSE_TIMEOUT_VALUE)
      {
        return HAL_TIMEOUT;
      }
    }
   
    /* Set the new LSE configuration -----------------------------------------*/
    __HAL_RCC_LSE_CONFIG(RCC_OscInitStruct->LSEState);
    /* Check the LSE State */
    if(RCC_OscInitStruct->LSEState != RCC_LSE_OFF)
    {
      /* Get Start Tick */
      tickstart = HAL_GetTick();
     
      /* Wait till LSE is ready */ 
      while(__HAL_RCC_GET_FLAG(RCC_FLAG_LSERDY) == RESET)
      {
        if((HAL_GetTick() - tickstart ) > RCC_LSE_TIMEOUT_VALUE)
        {
          return HAL_TIMEOUT;
        }
      }
    }
    else
    {
      /* Get Start Tick */
      tickstart = HAL_GetTick();
     
      /* Wait till LSE is disabled */ 
      while(__HAL_RCC_GET_FLAG(RCC_FLAG_LSERDY) != RESET)
      {
        if((HAL_GetTick() - tickstart ) > RCC_LSE_TIMEOUT_VALUE)
        {
          return HAL_TIMEOUT;
        }
      }
    }
  }

#if defined(RCC_CR_PLL2ON)
  /*-------------------------------- PLL2 Configuration -----------------------*/
  /* Check the parameters */
  assert_param(IS_RCC_PLL2(RCC_OscInitStruct->PLL2.PLL2State));
  if ((RCC_OscInitStruct->PLL2.PLL2State) != RCC_PLL2_NONE)
  {
    /* This bit can not be cleared if the PLL2 clock is used indirectly as system
      clock (i.e. it is used as PLL clock entry that is used as system clock). */
    if((__HAL_RCC_GET_PLL_OSCSOURCE() == RCC_PLLSOURCE_HSE) && \
        (__HAL_RCC_GET_SYSCLK_SOURCE() == RCC_SYSCLKSOURCE_STATUS_PLLCLK) && \
        ((READ_BIT(RCC->CFGR2,RCC_CFGR2_PREDIV1SRC)) == RCC_CFGR2_PREDIV1SRC_PLL2))
    {
      return HAL_ERROR;
    }
    else
    {
      if((RCC_OscInitStruct->PLL2.PLL2State) == RCC_PLL2_ON)
      {
        /* Check the parameters */
        assert_param(IS_RCC_PLL2_MUL(RCC_OscInitStruct->PLL2.PLL2MUL));
        assert_param(IS_RCC_HSE_PREDIV2(RCC_OscInitStruct->PLL2.HSEPrediv2Value));

        /* Prediv2 can be written only when the PLLI2S is disabled. */
        /* Return an error only if new value is different from the programmed value */
        if (HAL_IS_BIT_SET(RCC->CR,RCC_CR_PLL3ON) && \
          (__HAL_RCC_HSE_GET_PREDIV2() != RCC_OscInitStruct->PLL2.HSEPrediv2Value))
        {
          return HAL_ERROR;
        }
       
        /* Disable the main PLL2. */
        __HAL_RCC_PLL2_DISABLE();
       
        /* Get Start Tick */
        tickstart = HAL_GetTick();
       
        /* Wait till PLL2 is disabled */
        while(__HAL_RCC_GET_FLAG(RCC_FLAG_PLL2RDY) != RESET)
        {
          if((HAL_GetTick() - tickstart ) > PLL2_TIMEOUT_VALUE)
          {
            return HAL_TIMEOUT;
          }
        }
       
        /* Configure the HSE prediv2 factor --------------------------------*/
        __HAL_RCC_HSE_PREDIV2_CONFIG(RCC_OscInitStruct->PLL2.HSEPrediv2Value);

        /* Configure the main PLL2 multiplication factors. */
        __HAL_RCC_PLL2_CONFIG(RCC_OscInitStruct->PLL2.PLL2MUL);
       
        /* Enable the main PLL2. */
        __HAL_RCC_PLL2_ENABLE();
       
        /* Get Start Tick */
        tickstart = HAL_GetTick();
       
        /* Wait till PLL2 is ready */
        while(__HAL_RCC_GET_FLAG(RCC_FLAG_PLL2RDY)  == RESET)
        {
          if((HAL_GetTick() - tickstart ) > PLL2_TIMEOUT_VALUE)
          {
            return HAL_TIMEOUT;
          }
        }
      }
      else
      {
       /* Set PREDIV1 source to HSE */
        CLEAR_BIT(RCC->CFGR2, RCC_CFGR2_PREDIV1SRC);

        /* Disable the main PLL2. */
        __HAL_RCC_PLL2_DISABLE();
 
        /* Get Start Tick */
        tickstart = HAL_GetTick();
       
        /* Wait till PLL2 is disabled */ 
        while(__HAL_RCC_GET_FLAG(RCC_FLAG_PLL2RDY)  != RESET)
        {
          if((HAL_GetTick() - tickstart ) > PLL2_TIMEOUT_VALUE)
          {
            return HAL_TIMEOUT;
          }
        }
      }
    }
  }

#endif /* RCC_CR_PLL2ON */
  /*-------------------------------- PLL Configuration -----------------------*/
  /* Check the parameters */
  assert_param(IS_RCC_PLL(RCC_OscInitStruct->PLL.PLLState));
  if ((RCC_OscInitStruct->PLL.PLLState) != RCC_PLL_NONE)
  {
    /* Check if the PLL is used as system clock or not */
    if(__HAL_RCC_GET_SYSCLK_SOURCE() != RCC_SYSCLKSOURCE_STATUS_PLLCLK)
    {
      if((RCC_OscInitStruct->PLL.PLLState) == RCC_PLL_ON)
      {
        /* Check the parameters */
        assert_param(IS_RCC_PLLSOURCE(RCC_OscInitStruct->PLL.PLLSource));
        assert_param(IS_RCC_PLL_MUL(RCC_OscInitStruct->PLL.PLLMUL));
 
        /* Disable the main PLL. */
        __HAL_RCC_PLL_DISABLE();
       
        /* Get Start Tick */
        tickstart = HAL_GetTick();
       
        /* Wait till PLL is disabled */
        while(__HAL_RCC_GET_FLAG(RCC_FLAG_PLLRDY)  != RESET)
        {
          if((HAL_GetTick() - tickstart ) > PLL_TIMEOUT_VALUE)
          {
            return HAL_TIMEOUT;
          }
        }

        /* Configure the HSE prediv factor --------------------------------*/
        /* It can be written only when the PLL is disabled. Not used in PLL source is different than HSE */
        if(RCC_OscInitStruct->PLL.PLLSource == RCC_PLLSOURCE_HSE)
        {
          /* Check the parameter */
          assert_param(IS_RCC_HSE_PREDIV(RCC_OscInitStruct->HSEPredivValue));
#if defined(RCC_CFGR2_PREDIV1SRC)
          assert_param(IS_RCC_PREDIV1_SOURCE(RCC_OscInitStruct->Prediv1Source));
         
          /* Set PREDIV1 source */
          SET_BIT(RCC->CFGR2, RCC_OscInitStruct->Prediv1Source);
#endif /* RCC_CFGR2_PREDIV1SRC */

          /* Set PREDIV1 Value */
          __HAL_RCC_HSE_PREDIV_CONFIG(RCC_OscInitStruct->HSEPredivValue);
        }

        /* Configure the main PLL clock source and multiplication factors. */
        __HAL_RCC_PLL_CONFIG(RCC_OscInitStruct->PLL.PLLSource,
                             RCC_OscInitStruct->PLL.PLLMUL);
        /* Enable the main PLL. */
        __HAL_RCC_PLL_ENABLE();
       
        /* Get Start Tick */
        tickstart = HAL_GetTick();
       
        /* Wait till PLL is ready */
        while(__HAL_RCC_GET_FLAG(RCC_FLAG_PLLRDY)  == RESET)
        {
          if((HAL_GetTick() - tickstart ) > PLL_TIMEOUT_VALUE)
          {
            return HAL_TIMEOUT;
          }
        }
      }
      else
      {
        /* Disable the main PLL. */
        __HAL_RCC_PLL_DISABLE();
 
        /* Get Start Tick */
        tickstart = HAL_GetTick();
       
        /* Wait till PLL is disabled */ 
        while(__HAL_RCC_GET_FLAG(RCC_FLAG_PLLRDY)  != RESET)
        {
          if((HAL_GetTick() - tickstart ) > PLL_TIMEOUT_VALUE)
          {
            return HAL_TIMEOUT;
          }
        }
      }
    }
    else
    {
      return HAL_ERROR;
    }
  }
 
  return HAL_OK;
}

[Sal Ammoniac]

But here: I'll throw up the clock initialization function, which I think is really hard to justify...

Barf!    My code, which does essentially the same thing, is 25 lines of code.

[jnz]

Barf!    My code, which does essentially the same thing, is 25 lines of code.

While I largely agree with you... My issue with statements like this is no one ever posts up their code. No one puts their money where their mouth is.

It's not like peripheral libs are some dangerous proprietary code...

[Koen]

Yes, ChibiOS and libopencm3 would surely welcome patches from such incredibly talented programmers who write improved libraries from scratch !

[richardman]

Ha ha, OK, now we know you guys have not looked at assembly code much or efficient coding in C ;-)

Yes, in theory, a compiler should "do what I mean" but...

[Kjelt]

But here: I'll throw up the clock initialization function, which I think is really hard to justify.

Been awhile since i worked with those but what i remember is that if you want to switch between the clocks external / internal /   high speed / low speed you can,t just flip a bit and be done or things go terribly wrong. You have to wait for certain conditions with the pll to arise etc and then switch.
When you only want to use one clock source like the HSE then you can delete all the rest or better add some #ifdefs  to loose it through a user config file BUT again if some requirements change like say you need to power down in low power mode till some interrupt occurs or some requirement like that, you again need that code.
So the code is there to be tweaked but at least it is complete. I would be much more upset if they would have given the HSE as example and left the rest for the user to figure out.
So yeah you can tweak it if you like to save 50 bytes of code or so, then I do wonder with a 128kB ROM to even 1MB of ROM who cares that much about that. Did you ever take a look at OpenSSL or CYASSL(WolfSSL) how much that costs?
And please do give us a better rewrite of that code but mind it should still work and be complete.

[richardman]

Most M0 only has 32K...

[Sal Ammoniac]

All software has bugs also if you write the custom code. The difference is that the vendor libraries are open source and youre not the first and only customer. If there are major bugs you will read about it and also if there is a solution, if not you can always comment out the buggy stuf and write that part on your own.

OpenSSL is open source. How long was the heartbleed vulnerability in that code before someone exploited it? Why didn't all of those hoards and hoards of eyes looking at it day and night find and fix it before it became a major headache?

[dannyf]

" When you only want to use one clock source like the HSE then you can delete all the rest or"

Precisely. I have a set of routines that would switch me to hsi, HSE, hsipll and hsepll, among others. Then I write a routine that switches me to any of those based on the clock configuration. With that, the code is tight if you wish to a known target clock, or easy to read if you wish to retain the flexibility if run time user configurable clock.

St simply messed up the layering of their clock modules and tried to write an all encompassing module in one function. Total incompetency if you ask me.

[westfw]

if you want to switch between the clocks external / internal /   high speed / low speed

Because doing that is SO common?  I don't think so!  (that's one of the big objections.)  Average use case is that you set up the clock at startup and it stays at that rate, and/or stops completely in low-power sleep modes.

at least it is complete. I would be much more upset if they would have given the HSE as example and left the rest for the user to figure out.

I can agree with that.  And it's pretty readable, too.  It might be an interesting exercise to "fix" the HAL code to the point where it's someone more acceptable, without descending into "maximally efficient by incomprehensible" status.   Separate init routines for each clock type and a "static inline" wrapper would help a lot.

It also bothers me that the clock function is NOT easy to use.  It's pretty much "fill in a data structure with a bunch of obscure parameters that you'll have to study the datasheet to understand", and then we'll copy those into the appropriate registers."  All the cost of abstraction with none of the benefits. :-(

So yeah you can tweak it if you like to save 50 bytes of code or so

It's not a "50 byte" savings.   More like 1000 bytes.  For code that SHOULD be about 50 bytes.  My most recent experience is with Atmel ARM chips and their ASF library (which is very similar.)  The clock initialization code was about 1kbytes.  This was for a SAMD10 that only had 16k, so that was particularly depressing, but it's a pretty significant hit even on a 128k part.  (I checked: The SAMD10 "Blink an LED" example program has 1072 bytes of system clock functions.)

no one ever posts up their code.

I did.  https://www.eevblog.com/forum/microcontrollers/stm32-ghetto-style/msg551553/#msg551553
(Implements: static inline void SimpleClockInit(long cpuspeed, long xtalspeed);  Use xtalspeed=0 for the internal oscillator, doesn't currently handle low speed oscillators (but could.)  It does rely on the compiler being pretty intelligent about compile-time evaluation of constant expressions to get full 'efficiency')

There's also an assembler macro here: https://github.com/WestfW/Minimal-ARM/blob/master/inc/WestfwMacros.S

Mind you, I don't have the evangelistic energy to re-write that for a dozen different chip variations, do the requisite testing for all the possible cases, and push it into various OSSW projects or at vendors...  And vendors/ OSSW administrators have a (justifiable) resistance to importing new code that is "merely faster and smaller" if it's less general (witness the failure of Arduino to adopt any of the "fast" versions of digitalWrite())

[richardman]

I can see why people would want to switch between HSE/HSI etc. For example, one might want to use HSE, but then if the crystal fails somehow, it should drop back down to HSI.

re: system clock setup function
On fear that someone will accuse me on "spamming" again, it does not have to be hard to define a simple to use function API. I am on the road, but our clock function is as simple as

jsapi_clock.setSystemClock(unsigned hsi_mhz, unsigned hse_mhz, unsigned pll_mhz, int wait_states);

if hsi_mhz is non-zero, it is forced to whatever HSI is for that series, e.g. 8 for F0xx and 16 for F4xx etc. Otherwise, hse_mhz should be non-zero. If pll_mhz is non-zero, the function computes the PLLx values and set them up for you.

It is not rocket science to write simple API that works for 90% of the people 90% of the time (or whatever the percentages are). I call it the Steve Jobs' principles.