search

hathach / tinyusb

An open source cross-platform USB stack for embedded system
https://www.tinyusb.org
MIT License
5.08k stars 1.06k forks source link

DHSE_VALUE=8000000 override in STM32 Makefiles #204

Closed hierophect closed 4 years ago

hierophect commented 5 years ago

Here is my question Is there a particular reason why this override of the xxx_hal_conf.h file exists in the Makefile Cflags of all the BSP examples? If a user isn't aware of its use, or assumes it is overridden by the HAL instead of the other way around, it will quietly cause the frequency to be wrong across all registers and HAL functions.

I personally skimmed over it in my initial installation of tusb and then forgot about it, inviting problems down the line. It doesn't seem to serve any purpose since this value is an essential component of hal_conf. Should it be removed?

hathach commented 5 years ago

I have no particular reason, we could use the value in _hal_conf.h if it is how STM32 user typically put that. @cr1901 @pigrew you guys know stm32 much better than me, what do you think ?

cr1901 commented 5 years ago

I think it went like this...

IIRC, HSE_VALUE is required so SystemCoreClockUpdate or similar works on STM32, regardless of whether one is using the HAL or not. Therefore, when I did the first STM32 port, I didn't use the HAL, but I defined HSE_VALUE anyway just so the define was there for CMSIS. At some point @hathach switched the STM32 examples to use parts of the HAL- fair enough! And I did the same w/ subsequent board ports.

In tinyusb right now, the _hal_conf.h headers provided for each board are not board specific. Rather, the _hal_conf.h headers are the generic defaults you get from a STM32CubeMX project for a given chip family. I copied the chip-generic _hal_conf.h for each board instead of a board-specific one for consistency with the existing tinyusb examples.

Turns out, HSE_VALUE defaults for a given family in STM32CubeMX don't always match a given board (Nucleo H7 uses a 8MHz crystal) :).

Anyways, I'm not overly attached to using generic _hal_conf.h; we could go to board-specific ones if necessary (as generated by STM32CubeMX), I just wanted to be consistent with how each HAL header was generated when porting examples; board-specific header or a family-specific header w/ defaults?

hathach commented 5 years ago

Yeah, I am fine with either one as well. There is little difference, @hierophect If you want to change the example to use _hal_conf.h would you mind submit an PR for it. You probably need to do that for all the stm32 board though to stay consistent.

hathach commented 5 years ago

hmm, I changed my mind, I think I prefer the current way to have -DHSE_VALUE in the board.mk since we will modify it when doing a board portting anyway. I rarely open the hal_conf.h to change a config, usually copy it from other board of same family. This is just board & example, it is not part of the tinyusb. Application project e.g CPy are free to do it in either way, we better spend energy for other pending issues (which are quite a lot now :( )

dhalbert commented 5 years ago

Then maybe the current setting just needs to be documented in the code so if anyone copies it they know the consequences.

hathach commented 5 years ago

@dhalbert that is good idea, though there is no documentation now for any specific bsp rather than the code and board.mk itself. Board and example are merely the way for user to run test out tinyusb API, I have no intentions to make it as template or standard for other projects.

Though I switch my mind again, I may change it to use hal_conf.h for the reason that other IDE than makefile doesn’t need to define the macros, making using other IDE easier with existing files

pigrew commented 4 years ago

@cr1901, @hathach : While trying add HAL UART calls to the f407vg discovery board (which I'm not sure we should include in the BSP by default), I discovered that the data rate was wrong, I believe due to a CMSIS call, which is overriding a system clock speed global variable (that is shared between CMSIS and HAL): https://github.com/hathach/tinyusb/blob/435485b524f7d5299f489b34555a0c9d5f616159/hw/bsp/stm32f407disco/stm32f407disco.c#L113

This call is looking for a #define of HSE_VALUE which doesn't exist, so it uses the CMSIS default of 16 MHz., instead of the stm32f4xx_hal_conf.h value. -DHSE_VALUE=8000000 needs to be added to board.mk.

Another option would be to remove the calls to SystemCoreClockUpdate().

Also, it looks like we should include stm32_f4xx_hal.h and not: https://github.com/hathach/tinyusb/blob/435485b524f7d5299f489b34555a0c9d5f616159/hw/bsp/stm32f407disco/stm32f407disco.c#L30

This _hal.h file does include the _conf file.

So, to summarize, I propose the following changes:

1a. Add the appropriate -DHSE_VALUE=8000000 to all STM32 board.mk OR 1b. Remove all calls of SystemCoreClockUpdate() in all STM32 BSP source files.

  1. Change all STM32 BSP to include stm32_f4xx_hal.h instead of the _conf.h file.
  2. (maybe) Add UART to the BSP, even on boards without a prewired connection. (I'm only 90% confident in the above, let me know if I'm wrong...)
hathach commented 4 years ago

thanks @pigrew for the discovering this, it is indeed annoying. ST should make the clokcupdate take the HSE instead of assuming the value in 2 places which seems to be asking for troubles. Unfortunately we will need SystemCoreClockUpdate() to get the correct value of SystemCoreClock for systick. I guess ST intend to have people to put that in the makefile, so let's do the 1a option.

for 2) : I am ok with stm32_f4xx_hal as well, just took a look, it has the handy HAL_GetUID that we can use for the serial number later on

3) Yeah sure, if the pin is physically accessible via pin header. UART log is adding recently, so several bsp isn't updated to make use of that. I actually only enable bsp that I need to test/debug with :D.

pigrew commented 4 years ago

@hathach I just created a branch for this (well, only for the F407disco). we can apply to the other BSP based on your feedback.

Both SystemClock_Config() and SystemCoreClockUpdate() update the same global variable SystemCoreClock (and that's all they do). There is no need for both. Let's remove the CMSIS one?

ADDED: Both of them use HSE_VALUE. However, the CMSIS one only reads from the CFLAGS, while the HAL one uses the value specified in hal_config, unless CFLAGS HSE_VALUE is set.

Please preview changes at:

https://github.com/pigrew/tinyusb/commit/2e63d8aee680c9342b2c67bfa36cafe7d1a9d654

We can remove the ENABLE_UART? Or good idea? Yes, the STM32F4 target pin A2 is on a pin header. I had to use a flywire because the USB CDC pin of the stlink isn't, and I wanted to use the st link UART (more convenient).

Thoughts?

hathach commented 4 years ago

@hathach I just created a branch for this (well, only for the F407disco). we can apply to the other BSP based on your feedback.

Both SystemClock_Config() and SystemCoreClockUpdate() update the same global variable SystemCoreClock (and that's all they do). There is no need for both. Let's remove the CMSIS one?

ADDED: Both of them use HSE_VALUE. However, the CMSIS one only reads from the CFLAGS, while the HAL one uses the value specified in hal_config, unless CFLAGS HSE_VALUE is set.

Oh, I didn't know that, Let's remove CMSIS one, one less thing to worry about :D

Please preview changes at:

pigrew@2e63d8a

We can remove the ENABLE_UART? Or good idea? Yes, the STM32F4 target pin A2 is on a pin header. I had to use a flywire because the USB CDC pin of the stlink isn't, and I wanted to use the st link UART (more convenient).

Thoughts?

for stm32f407 disco1 (newer version), the newt stlink has builtin cdc <-> uart, we should use those pins which is very convenient. The older version doesn't have that, user can wire hook with FTDI for the output ( I often do that )

pigrew commented 4 years ago

Agreed.

And I should be saying HAL_RCC_ClockConfig() (not SystemClock_Config).

I'll finish up the patch, maybe tomorrow. It's getting late here.

It looks like SysTick_Config(SystemCoreClock / 1000); might need to be moved a few lines later in a few of the BSP, since it's using SystemCoreClock BEFORE it's setup.

hathach commented 4 years ago

thanks for the change, let's just enable the UART.

hathach commented 4 years ago

Agreed.

And I should be saying HAL_RCC_ClockConfig() (not SystemClock_Config).

I'll finish up the patch, maybe tomorrow. It's getting late here.

It looks like SysTick_Config(SystemCoreClock / 1000); might need to be moved a few lines later in a few of the BSP, since it's using SystemCoreClock BEFORE it's setup.

No problem at all, move it anywhere that makes sense to you. No hurry at all. Please take the rest :)