Requires #882

Is there any way we can re-run Travis now that #882 is merged?

Yes, anyway, I will not merge this one as it is.
As I explained in #876, the default config for Generic boards should be HSI.
#876 (comment)

This is no different than the other generic pinouts. First, it tries HSE. If no crystal/clock is detected, it will switch to HSI. This must be the most compatible option?

I will also change for other generic.
The main issue is that the config is linked to an HSE value.
As I point in the other issue, the clock config for the HSE is based on a 8 MHz value, while per default the HSE_VALUE for F4 is 25 MHz. So this is not correct.

So would you accept the generic pinouts to use an HSE based clock/crystal of 25 MHz and automatically switch to HSI if not present?

And wouldn't it be better to merge my pending PRs like they are at the moment, and fix all generic F4 variants in the same commit/PR afterwards? After all their clock config is pretty much identical.

So would you accept the generic pinouts to use an HSE based clock/crystal of 25 MHz and automatically switch to HSI if not present?

No it is too much support. Goal is to provide an unique working clock config for every case.

And wouldn't it be better to merge my pending PRs like they are at the moment, and fix all generic F4 variants in the same commit/PR afterwards? After all their clock config is pretty much identical.

Why adding codes we know they will be removed.

No it is too much support. Goal is to provide an unique working clock config for every case.

So for all users that would like to use HSE they will have to redefine SystemClock_Config()?
How about making it so that the SetSysClock_PLL_HSE() function could have it's external clock speed specified as a parameter? something like this:

void SystemClock_Config(void)
{
  SetSysClock_PLL_HSE(1 /* no bypass */, 25 /* MHz */);
}

This would actually be quite neat!

So for all users that would like to use HSE they will have to redefine SystemClock_Config()?

Yes, at least value and config will be aligned as they will have also to define the proper HSE_VALUE if different from the default one.

How about making it so that the SetSysClock_PLL_HSE() function could have it's external clock speed specified as a parameter?

The main issue is that the HSE_VALUE is a define so used at build time not execution.

The main issue is that the HSE_VALUE is a define so used at build time not execution.

Yes, but HSE_VALUE is just a constant that isn't directly used by SetSysClock_PLL_HSE(uint8_t bypass). Couldn't the default value be loaded like so?

SetSysClock_PLL_HSE(0 /* no bypass*/, HSE_VALUE /* default val */);

@MCUdude
I didn't talk about the SystemClock_Config(), but all HAL RCC and more.

OK. So basically it isn't that easy to just pick a random crystal and expect it to work by only tweaking the clock config function.

Hear me out here:

• It would be nice to be able to use an external clock/crystal without having to re-define the clock config function. This can easily be achieved if we standardize on one frequency, I suggest 25 MHz since this is default anyways.
  • A crystal/clock is automatically detected, This works well!
• If the user wants to use a clock/crystal with a different frequency the HSE_VALUE can be defined in the build process (i.e in platformio.ini). This will not mess up HAL RCC since HSE_VALUE can be defined "externally":
  

  Arduino_Core_STM32/system/STM32F4xx/system_stm32f4xx.c

  Lines 65 to 69 in 07bb4f0

  	#include "stm32f4xx.h"
  	#if !defined (HSE_VALUE)
  	#define HSE_VALUE ((uint32_t)25000000) /*!< Default value of the External oscillator in Hz */
  	#endif /* HSE_VALUE */

• Clock configuration would be transparent for the average user, but "pro users" will still be able to change HSE_VALUE.

If you want, I can submit a PR for this.

Wait, how is this not a problem on the F407 DISCO board (which I do own)? This board has an external 8 MHz crystal, but the HSE_VALUE is not redefined anywhere.

In that case the clock config is correct but I guess if you use some dedicated feature, this would not work as expected, example RTC using HSE.

In that case the clock config is correct but I guess if you use some dedicated feature, this would not work as expected, example RTC using HSE.

Got it. So like mentioned in my previous post, why not "standardize" on 25 MHz for all generic F4 targets, and let the user redefine HSE_VALUE if needed?

Even if you redefine HSE_VALUE, it is need to redefine the SystemClock_Config()

Even if you redefine HSE_VALUE, it is need to redefine the SystemClock_Config()

Yes, but this is very trivial:

RCC_OscInitStruct.PLL.PLLM  = HSE_VALUE/1000000L;

Even if you redefine HSE_VALUE, it is need to redefine the SystemClock_Config()

Yes, but this is very trivial:

RCC_OscInitStruct.PLL.PLLM  = HSE_VALUE/1000000L;

Yes in that case, this work for the requested SYSCLK but for some other freq no.

I've just done a pretty deep dive into how the clocks are initialized to get timing correctly. I've used CubeMX's graphical window to figure out what all the clock config values might represent.

Working on my Nucleo F410RB, I was only able to get accurate timing (delay() in this case) when using an 8 MHz external clock. The PLL settings didn't even matter, the LED was blinking way too fast when using a 25 MHz clock even though the PLL settings should result in a 100 MHz SYSCLK.

It turned out that the HSE_VALUE isn't 25000000 by default (For F401, F410 and F411 at least), it is 8000000!

I believe it gets its definition here:

And not from here:

So then, 8 MHz is actually the default HSE_VALUE for the F4 series. Is this a bug or a feature?

But this was at least a great learning experience for me 😀. I now feel I understand more about how the whole clock configuration works. In the process, I found out that the clock settings for some of the other generic variants I've created aren't really ideal. When we figure out what the default HSE_VALUE is, then I can submit a PR where I clean up some of this.

I suggest we stick with either 8 MHz (default value for practically all other STM32 families) or 25 MHz, and then pass -DHSE_VALUE=25000000UL as a build option for targets/boards that require a special HSE_VALUE, such as the F401 blackpill board.

Right, header is used before the source, so yes it is 8MHz. Forgot this.

The default is the one in the stm32f4xx_hal_conf_default.h.
You always can do a PR but I can't tell if it will be merged.
As said the simpliest way it to provide a well know config using the HSI or other internal clock for generic.
Then a board can use the generic variant and adding extra config under ARDUINO_<BOARDNAME> definition to customize it properly (pinmaping, clock config, hal conf,...)

As said the simpliest way is to provide a well know config using the HSI or other internal clock for generic.

In my opinion, it is even better to first try to use an external clock/oscillator and fall back to HSI if not present. This will cover most cases. For known boards that use another crystal, it's just a matter of adding -DHSE_VALUE={someval} in boards.txt.

If you refuse this, another option is to rewrite SystemClock_Config into this:

WEAK void SystemClock_Config(void)
{
  if (SetSysClock_PLL_HSI() == 0) {
      Error_Handler();
  }
}

... and let the user override it in the sketch, like so, if he/she wants to use an external clock/crystal:

void SystemClock_Config(void)
{
  if (SetSysClock_PLL_HSE(0) == 0) {
      Error_Handler();
  }
}