Kconfig support for stm32 HAL/LL file selection

[lowlander]

These commits add the possibility to select parts of the STM32 CUBE HAL/LL by simply adding a USE_STM32_xxXX_HAL_* entry to a Kconfig file. That way driver writers do not have to change the Kconfig/Kbuild files under ext/hal/. Also users can use the HAL without needing to change Zephyr code.

[erwango]

Understand what you're trying to do, but I have 3 concerns:
-moving build logic in drivers Kconfig files
-creation of huge number of Kconfig flags (compile duration impact?)
-This is making huge files difficult to review even if there should be much less impact on these files afterwards.

Is there a way to split these files and distribute it in ext/hal/st/stm32cube/stm32yyxx/ folders?
Then only dedicated part of the flags/files would be visible when compiling.

One last point is that LL headers could be used without any flags (as done for serial driver case for instance). Maybe some comment somewhere should make this clear (USE_LL flags are only valid for LL.c files). This is not new, but creation of USE_LL flags might be misleading in that regard.

[lowlander]

@erwango

-moving build logic in drivers Kconfig files

Is that really worse than having driver build logic in a completely different part of the Zephyr source tree like it is now? With this change the logic is all local to the driver. Also it will cause less commit "collisions" cause at the moment all HAL/LL users will need to edit the etx/hal/st/ kbuild files.

Also when more than one driver (for example PWM and Counter) need the same hal files (tim.c) the current kbuild file will get more and more cluttered. If there are than also user applications, demo's or tests that need HAL/LL sources it will get even worse.

Another thing is now the hal kbuid file has things like;

obj-$(CONFIG_SERIAL_HAS_DRIVER) += stm32f0xx/drivers/src/stm32f0xx_hal_uart.o

That doesn't give any information which serial driver actually needs the HAL. And if there is a serial driver that doesn't need the HAL it can not disable compiling of the HAL code. Same for CONFIG_PWM and CONFIG_I2C.

-creation of huge number of Kconfig flags (compile duration impact?)

The "if SOC_SERIES_STM32F4X" already reduces that it seems. Splitting it up in one file per SOC-type will make the file more readable.

-This is making huge files difficult to review even if there should be much less impact on these files afterwards.

As long as no files are added or removed from the Cube HAL the kconfig/kbuild file should never be touched anymore.

Maybe one could use some python script to auto generate the kbuild and kconfig files ? They are pretty much one to one with the *.c files. If it is hard to do that at build time, the cube maintainer could use the script to generate the files at "commit" time?

One last point is that LL headers could be used without any flags (as done for serial driver case for instance). Maybe some comment somewhere should make this clear (USE_LL flags are only valid for LL.c files). This is not new, but creation of USE_LL flags might be misleading in that regard.

Maybe a different verb than USE? Something like LINK_TO__STM32F1XX_LL_I2C or NEED_STM32F1XX_LL_I2C _OBJ ?

[erwango]

Is that really worse than having driver build logic in a completely different part of the Zephyr source tree like it is now? With this change the logic is all local to the driver. Also it will cause less commit "collisions" cause at the moment all HAL/LL users will need to edit the etx/hal/st/ kbuild files.

Also when more than one driver (for example PWM and Counter) need the same hal files (tim.c) the current kbuild file will get more and more cluttered. If there are than also user applications, demo's or tests that need HAL/LL sources it will get even worse.

Ok, this is a fair point.

-creation of huge number of Kconfig flags (compile duration impact?)

The "if SOC_SERIES_STM32F4X" already reduces that it seems. Splitting it up in one file per SOC-type will make the file more readable.

Splitting up files would be nice

-This is making huge files difficult to review even if there should be much less impact on these files afterwards.

As long as no files are added or removed from the Cube HAL the kconfig/kbuild file should never be touched anymore.

Maybe one could use some python script to auto generate the kbuild and kconfig files ? They are pretty much one to one with the *.c files. If it is hard to do that at build time, the cube maintainer could use the script to generate the files at "commit" time?

Doing it at build time would require pretty good documentation, to make it understandable for user.
Let keep it static for now. Splitting it up would allow easier generation at 'commit' time.

One last point is that LL headers could be used without any flags (as done for serial driver case for instance). Maybe some comment somewhere should make this clear (USE_LL flags are only valid for LL.c files). This is not new, but creation of USE_LL flags might be misleading in that regard.

Maybe a different verb than USE? Something like LINK_TO__STM32F1XX_LL_I2C or NEED_STM32F1XX_LL_I2C _OBJ ?

USE_STM32F1XX_LL_I2C _OBJ ? Not quite sure here..

[lowlander]

@erwango I split the Kconfig and Kbuild files up and put them in the cpu dirs, that really cleans up the code.

To the name LINK_TO_STM32F1XX_LL_I2C, NEED_STM32F1XX_LL_I2C_OBJ, USE_STM32F1XX_LL_I2C, etc. I really don't know. Maybe someone else can comment on this ?
The most difficult part of software engineering, what should be the name of my class, function, variable :-)

[lowlander]

@galak @erwango any ideas what to do with the Kconfig menu entry names ? Should I change it to USE_STM32F1XX_LL_I2C_OBJ ? And anything else I can do to get this patch accepted.

@dbkinder there are README's in the st hal directories. But those don't end up in the official documentation, do they ? The maintainer notes will probably be of no use to normal users, but the how to use the HAL is of use to them. What would be the way to get that in the "official" documentation? (assuming this patch is accepted of course)

[dbkinder]

The documentation gathering process only looks for RestructuredText-formatted .rst files, so no, the .txt files are ignored when creating our public documentation. If you separate the maintainer notes (in the .txt file) from the public/user notes (in a .rst file) we can get them included.

[lowlander]

@dbkinder thanks for the info, if I have a go from the code maintainers I will split up the documentation and make a rst file out of it.

[erwango]

@lowlander : Let's keep USE_STM32YYXX_LL_FOO and document the fact that using LL headers don't require any flags activation.

As a next step, would be good to get feedback from other frequent and active STM32 contributors like @mbolivar, @ydamigos , @superna9999

[superna9999]

This header seems inaccurate !

[lowlander]

Probably just added headers to all open files in my editor that didn't have a header asuming they were new files.

Remove or change ?

[superna9999]

Remove please

[lowlander]

will do.

[superna9999]

I'm ok with @erwango about the fact we can include headers without necessarily building the *.c files

Except the unnecessary header on ext/hal/Kbuild I'm ok with this.

[ydamigos]

I am fine with the changes. Just a comment which probably is not relevant with this PR.

[ydamigos]

Does the STM32F0 family use the HAL serial driver? I thought that all families use the LL serial api.

[superna9999]

F0 should use the same as all STM32 families

[ydamigos]

Could we remove this line or should we do it in a separate PR?

[erwango]

Should be a conversion of an oversight from F0 PR merged following merge of the PR that converted serial to full LL (Not quite clear, but you got my point).
To do it in a clean way, I propose an initial commit that remove culprit line in ext/hal/st/stm32cube/Kbuild.

[superna9999]

Acked

[lowlander]

I modeled the driver-kconfig files after the hal-Kconfig file that had;
obj-$(CONFIG_SERIAL_HAS_DRIVER) += stm32f0xx/drivers/src/stm32f0xx_hal_uart.o

[erwango]

That was my point this line was an oversight following of F0 series introduction.
Migration from HAL to LL in serial driver has been merged during process of F0 review. It should have been detected at that point (my bad).
It would be nice if you could do a preliminary commit to your PR where you remove this line.

[erwango]

@lowlander : Btw, just observed that initial Kbuild has been fixed with #4493.
So you could remove it safely after rebase

[mbolivar]

As a next step, would be good to get feedback from other frequent and active STM32 contributors like @mbolivar, @ydamigos , @superna9999

I think the overall approach is clever and have no problem with the changes.

It'd be nice to have CMakeFiles.txt equivalents in cmake_migration 😉

[erwango]

ok seems that change has community approval!
What about @galak? Are you ok with this proposal too?

I'm thinking that @MaureenHelm, you could be interested too, it is a solution that could be interesting for other HALs in general.

[lowlander]

@erwango yes I noticed yesterday evening. I wanted to update the docu (readme.txt) before pushing an update to this PR.

[lowlander]

@superna9999 F0 I2C Will be in the next push. @galak do you have anything else like the I2C from @superna9999 or were you just revering to the "merge conflict" ?

[lowlander]

@galak , @erwango any chance of getting this in master before Xmas ?

[erwango]

@galak : Before requesting a rebase to @lowlander, do you have any comments beforehand on this change?

[galak]

I haven't read the full thread, so sorry if this got discussed. Instead of having:

• select USE_STM32F0XX_LL_UTILS if SOC_SERIES_STM32F0X
• select USE_STM32F1XX_LL_UTILS if SOC_SERIES_STM32F1X
• select USE_STM32F3XX_LL_UTILS if SOC_SERIES_STM32F3X
• select USE_STM32F4XX_LL_UTILS if SOC_SERIES_STM32F4X
• select USE_STM32L4XX_LL_UTILS if SOC_SERIES_STM32L4X
  help

Can we just do:

select USE_STM32_LL_UTILS

(similar comment for config vars)

[lowlander]

@galak to be honest I didn't think about that, and it seems nobody else here :-) I changed it so they all have the same name for all cpu types. It makes things for the driver writer easier and cleaner.

[codecov-io]

Codecov Report

Merging #4646 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4646      +/-   ##
==========================================
+ Coverage   51.29%   51.29%   +<.01%     
==========================================
  Files         441      441              
  Lines       42275    42275              
  Branches     8066     8066              
==========================================
+ Hits        21685    21687       +2     
+ Misses      20069    20067       -2     
  Partials      521      521

Impacted Files	Coverage Δ
arch/posix/core/posix_core.c	87.06% <0%> (+1.72%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 834ab2c...43fcd95. Read the comment docs.

[erwango]

@lowlander : You need to add a comment to commit "ext: hal: stm32cube: update README". This is a new rule coming from TSC since last week.

Then, last question: Following change proposed by @galak , is there something that prevent to factorize Kconfig from to stm32cube/stm32yyxx/Kconfig to stm32cube/Kconfig ?
I know we did the opposite move initially, but given recent change, I think this could simplify a step further.

[lowlander]

@erwango yeah I noticed that commits with just a title aren't enough anymore, I am sure others already have discussed the usefulness of that rule.

To moving the Kconfig files to stm32cube, I think that should be possible. Some CPU's don't have all options, but I don't think that should be a problem.

I'll try to cleanup the patch in the next couple of days.

[galak]

@lowlander any update on this patch series?

[galak]

recheck

[lowlander]

@galak I'll fix the patch tomorow, it is to late now.

[lowlander]

@galak @erwango now things should be OK I think. There is only one (big) Kconfig for all CPU's and a CMakeLists.txt per CPU. There are a few files that are always compiled when the HAL is selected, for the rest it can be done with "select" statements.