Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add generic F410Rx pinout #880

Merged
merged 3 commits into from Jan 31, 2020
Merged

Add generic F410Rx pinout #880

merged 3 commits into from Jan 31, 2020

Conversation

MCUdude
Copy link
Contributor

@MCUdude MCUdude commented Jan 21, 2020

This pinout is formatted the same way as the Generic F401Rx, F411Rx and 446Rx pinouts, and is used with the STM32F410R8 and STM32F410RB chips

Note that currently, this pinout does not compile due to a bug somewhere in the core. More info in #878.

This pinout is formatted the same way as the Generic F401Rx, F411Rx and 446Rx pinouts, and is used with the STM32F410R8 and STM32F410RB chips
@fpistm
Copy link
Member

fpistm commented Jan 21, 2020

Requires #882

@MCUdude
Copy link
Contributor Author

MCUdude commented Jan 22, 2020

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

@fpistm
Copy link
Member

fpistm commented Jan 22, 2020

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)

@MCUdude
Copy link
Contributor Author

MCUdude commented Jan 22, 2020

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?

@fpistm
Copy link
Member

fpistm commented Jan 22, 2020

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.

@MCUdude
Copy link
Contributor Author

MCUdude commented Jan 22, 2020

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.

@fpistm
Copy link
Member

fpistm commented Jan 22, 2020

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.

@MCUdude
Copy link
Contributor Author

MCUdude commented Jan 22, 2020

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!

@fpistm
Copy link
Member

fpistm commented Jan 22, 2020

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.

@MCUdude
Copy link
Contributor Author

MCUdude commented Jan 22, 2020

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 */);

@fpistm
Copy link
Member

fpistm commented Jan 22, 2020

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

@MCUdude
Copy link
Contributor Author

MCUdude commented Jan 22, 2020

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":
    #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.

@MCUdude
Copy link
Contributor Author

MCUdude commented Jan 22, 2020

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.

@fpistm
Copy link
Member

fpistm commented Jan 22, 2020

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.

@MCUdude
Copy link
Contributor Author

MCUdude commented Jan 22, 2020

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?

@fpistm
Copy link
Member

fpistm commented Jan 22, 2020

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

@MCUdude
Copy link
Contributor Author

MCUdude commented Jan 22, 2020

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;

@fpistm
Copy link
Member

fpistm commented Jan 23, 2020

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.

@MCUdude
Copy link
Contributor Author

MCUdude commented Jan 23, 2020

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:

#if !defined (HSE_VALUE)
#define HSE_VALUE 8000000U /*!< Value of the External oscillator in Hz */
#endif /* HSE_VALUE */

And not from here:

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

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

@MCUdude
Copy link
Contributor Author

MCUdude commented Jan 23, 2020

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.

@fpistm
Copy link
Member

fpistm commented Jan 23, 2020

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,...)

@MCUdude
Copy link
Contributor Author

MCUdude commented Jan 23, 2020

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();
  }
}

 Now the external clock/crystal doesn't have to be 8 MHz as long as HSE_VALUE is defined at compile time. Clock frequency is also adjusted from 84 to 100 MHz
Copy link
Member

@fpistm fpistm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the clock config let's try like this.
Anyway all other should be updated according to this.

variants/Generic_F410Rx/PeripheralPins.c Outdated Show resolved Hide resolved
FMP TWI is not supported by this core anyways
@fpistm fpistm merged commit 72dca3c into stm32duino:master Jan 31, 2020
@fpistm fpistm added this to In progress in STM32 core based on ST HAL via automation Jan 31, 2020
@fpistm fpistm added this to the 1.9.0 milestone Jan 31, 2020
@fpistm fpistm moved this from In progress to Done in STM32 core based on ST HAL Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants