doc: document how to handle multiple cpu_models in a directory #12407

cladmi · 2019-10-09T14:14:01Z

Contribution description

This introduces ways of handling multiple cpu models defined in a
directory without relying on shell sed magic and algorithms.

This documentation, introduces different alternative ways to handle multiple cpu models in a cpu. Currently these handling are done through transforming the CPU_MODEL with shell | sed handling and then performing extractions and magic algorithms conversion to declare the specific CPU_MODEL variables.

This is currently the case for example in:

boards/slwstk6000b-*
cpu/efm32
cpu/kinetis
cpu/stm32f*

I am currently not capable in a few seconds to tell you how much memory a kinetis board has, and I would guess you would have a hard time knowing if an efm32 board provides the periph_hwrng feature without executing the build system.

On top of not being clear, these also require calling a $(shell) with some sed parsing to allow doing there matching and then perform other operations.
Which is way slower that pure make resolution.

So execute a slow algorithmic resolution at every make invocation (even clean) for something that is a fixed mapping.

It has been bothering me for some time, and now, that we are trying to move the required definitions to Makefile.features, these resolutions should be split and refactored anyway so found it would be the good time to also implement it in a developer friendly way. For reading and execution time.

This is currently written toward CPU and CPU_MODEL but also apply to some boards.
It was started this way and prefer getting feedback now than searching again for a name that will not be perfect anyway.

Testing procedure

Read the document and evaluate the content.
Also if you have feedback on the format, it is not a documentation anymore but maybe guidelines/ideas for maintainers.

The handling that would be replaced would be these ones (if I do not miss any):

Issues/PRs references

This is part of #11477

Tracking: move CPU/CPU_MODEL to Makefile.features Tracking: move CPU/CPU_MODEL to Makefile.features #11477 (for the slwstk600b-*
Build dependencies - processing order issues Build dependencies - processing order issues #9913
Tracking: remove harmful use of export in make and immediate evaluation Tracking: remove harmful use of export in make and immediate evaluation #10850 for the call to the shell even when not required

cladmi · 2019-10-09T14:14:59Z

@fjmolinas @aabadie @leandrolanzieri you may be interested.

cladmi · 2019-10-09T14:18:47Z

@vincent-d you may also be interested as I complained about stm32_line.mk to you at the summit. This would be my alternative implementation for this.

fjmolinas

General comments, that IMO makes the different proposals easier to understand.

fjmolinas · 2019-10-09T15:06:37Z

doc/doxygen/src/build-system-cpu-conventions.md

+Also, as currently dependencies are not declarative, it is with the previous
+solution one of the only alternative as the following declarative only would
+not work.


As currently dependencies are not declarative. Only solutions 1 and 2 would not be able to declare specific CPU_MODEL dependencies (as solutions 3 and 4 are declarative only.)

Also move this before the solutions descriptions.

With the solutions numbering it is way better.

I fixed "only solutions 1 and 2 would ~~not~~ be able".

I rephrased and moved to the first paragraph. I had to change the other description already talking about the different solutions.
It is in its own squash commit so you can see in details what I changed.

fjmolinas · 2019-10-09T15:07:07Z

doc/doxygen/src/build-system-cpu-conventions.md

+files.
+
+
+### `ifneq` based handling ###


Please add numbers to reference all solutions in a easy way.

Sure I will, I did not found how to have all automatically numbered but will compensate manually for now.
Also add them to the TOC for easier access.

fjmolinas · 2019-10-09T15:11:59Z

doc/doxygen/src/build-system-cpu-conventions.md

+In the case of only declarative values of specific variables, handling different
+`CPU_MODEL` is simply defining one per `CPU_MODEL`.
+When not relying on an `if` evaluation as before, one could define dictionaries
+which are implemented in `make` when using only one file with "namespaced"
+variables names including the `key` and so the `CPU_MODEL` value in the name.


Rewording suggestion:

Here every value is declared for each CPU_MODEL , handling different
CPU_MODEL is simply defining one per CPU_MODEL...

Same as the section 4, this changes the meaning, as it does not explain when it applies anymore which was my goal.

The "in the case of only declarative values of specific variables" is indeed hard to understand.

Maybe a "In the case of only declaring variables" ?

fjmolinas · 2019-10-09T15:13:50Z

doc/doxygen/src/build-system-cpu-conventions.md

+variables that have a list as value as the following solution does not apply.
+This is true for example for `FEATURES_PROVIDED`.


or for variables whose value is a list of values (this is case where solution 4 does not apply). This is true ...

doc/doxygen/src/build-system-cpu-conventions.md

fjmolinas · 2019-10-09T15:16:41Z

doc/doxygen/src/build-system-cpu-conventions.md

+Again in the case only delcarative values of specific variables, it can happen
+that many variables are somehow highly correlated.


In this case variables that are somehow correlated are declared for each CPU_MODEL.

That changes the style as I wanted to say "in THE case of declarative values that are correlated". In the same way as the section 3 introduction. The again was refering to the section 3.
I may not consistent with the style in all solutions now that I re-read them :/

I have your version in a similar way as the last sentence of the paragraph.

"somehow highly" is weird indeed :D and I used to much "somehow"

I will fix the typo in delcarative as the same time as the rest to not discard your review for nothing.

I reworded this, please @fjmolinas take a look

fjmolinas · 2019-10-14T09:06:46Z

@jia200x @leandrolanzieri I think it would be good to have your input on this since you will be working on configurations.

leandrolanzieri

I find that having these approaches documented clarifies things a LOT, including the fact of distinguishing when they take a declarative syntax (which is the way we should aim to). It's really good to have this type of documentation.

doc/doxygen/src/build-system-cpu-conventions.md

basilfx · 2019-10-17T11:49:53Z

I created the EFM32-based solution as a readable alternative to how the STM32 did it.

I'll be honest that I don't have a strong opinion on this topic, nor the knowledge what's the best for the build system. That's why I removed myself from the reviewers.

doc/doxygen/src/build-system-cpu-conventions.md

fjmolinas · 2019-10-18T07:31:06Z

IMO we should go with solution 1 when dependencies need to be declared and 4 otherwise. Unless configuraiton wise solution 1 would be better for configuration guys @leandrolanzieri @jia200x?

I think not all the options should be used, 2 should be avoided and and choose one between 3 and 4 as a less file cluttered alternative. Keeping one or two options at the end.

cladmi · 2019-10-30T12:03:52Z

The number 2 with ifneq/ifeq is used currently for exactly FEATURES_SUPPORTED += periph_hwrng with a list of boards. (it could at least be replaced with CPU_MODEL now that it is available in Makefile.features, and possibly with a whitelist and not blacklist.

It is currently in place and cannot be ignored. So should be tried to be migrated to the other alternatives before saying version 2 must not be used.
It is also done with the KINETIS_SERIES variable

https://github.com/RIOT-OS/RIOT/blob/e806d4bcbda2bd0735d2f60ba871b68db28ba118/cpu/kinetis/Makefile.features
https://github.com/RIOT-OS/RIOT/blob/aed12b48532d1414242ed4ca518c003bc77d4bfb/cpu/stm32_common/Makefile.features
https://github.com/RIOT-OS/RIOT/blob/aed628f08b1cf9ab450821653aeb6862fc53be80/cpu/saml21/Makefile.features

fjmolinas · 2019-11-12T10:14:29Z

ping @leandrolanzieri @jia200x

fjmolinas · 2019-11-12T10:15:51Z

It is currently in place and cannot be ignored. So should be tried to be migrated to the other alternatives before saying version 2 must not be used.
It is also done with the KINETIS_SERIES variable

Of course, but is different to say should not and must not, we can start with the first-most and migrate later. I would say agreeing on how to do it is the right first step. We can add an issue for the conflicts and eventually a static check.

fjmolinas · 2019-11-25T08:24:07Z

@leandrolanzieri how about re-phrasing "#2" as something that is being used and that we are trying to move away from towards "#1".

Regarding "#3" and "#4" do you have a preference? I would like to start moving towards implementing these. Maybe 3 is better although more verbose.

fjmolinas · 2019-11-25T08:40:37Z

Also maybe for the kinetis case #1 works largely better for you regarding kconfig ore devicetree? I seem to remember you a folder for each in some of your demos.

jia200x · 2019-11-26T18:49:07Z

Also maybe for the kinetis case #1 works largely better for you regarding kconfig ore devicetree? I seem to remember you a folder for each in some of your demos.

Yes! We are opting for that folder structure. It defines a clear interface with every cpu model, and in case we want to add external cpus (e.g privatives) then we can add them withouth touching RIOT files (only specifying the right RIOTCPU env).

leandrolanzieri · 2019-11-27T08:05:37Z

@fjmolinas Sorry for the delay!

IMHO we should definitely go with Approach 1. Although I can see that it might seem a bit of an overhead to add a folder per model, it gives us flexibility and order. This, as @jia200x pointed out, provides us with a clear interface to CPU models.

@leandrolanzieri how about re-phrasing "# 2" as something that is being used and that we are trying to move away from towards "# 1".

I agree. We should still mention it as a 'legacy' approach, that is migrating towards # 1.

Regarding "#3" and "#4" do you have a preference? I would like to start moving towards implementing these. Maybe 3 is better although more verbose.

I find Approach 3 to be clearer and more scalable, as it could include model-specific lists.

Also maybe for the kinetis case # 1 works largely better for you regarding kconfig ore devicetree? I seem to remember you a folder for each in some of your demos.

True, we took an approach similar to # 1 with the Devicetree folder structure, to place the bindings and the model-specific .dts. I would say # 1 is better, regardless of that though.

leandrolanzieri · 2019-11-27T11:55:46Z

doc/doxygen/src/build-system-cpu-conventions.md

+$(RIOTCPU)/$(CPU)/models/$(CPU_MODEL)/doc.txt
+~~~~~~~~~~~~~~~~~~~
+
+And then each main `$(CPU)` file will include the `$(CPU_MODEL)` specific one.


I believe I misunderstood this part when I first read it. I think this should be the other way around. A board should specify which CPU_MODEL it's using, then the model is in charge of including (or not) any common files like the CPU (which may then include other common, and thus more generic, files).

Files should be included from more specific to less specific. CPU_MODEL depends on BOARD, and CPU depends on CPU_MODEL, not the other way around.

yes I agree.

fjmolinas · 2019-11-29T13:54:02Z

True, we took an approach similar to # 1 with the Devicetree folder structure, to place the bindings and the model-specific .dts. I would say # 1 is better, regardless of that though.

Ok if we don't mind folder cluter, I say we move on with only solution number 1 as accepted, although we list 2 as something that exists and that we wan't to get rid off. We can the open an issue with all the cases that do not respect that. Some will need some work to get into shape.

leandrolanzieri · 2020-03-09T14:52:59Z

@fjmolinas I addressed some pending comments.

Ok if we don't mind folder cluter, I say we move on with only solution number 1 as accepted, although we list 2 as something that exists and that we wan't to get rid off.

So we should only keep those alternatives in the document?

miri64 · 2020-06-25T12:51:34Z

@aabadie @fjmolinas care to review?

This introduces ways of handling multiple cpu models defined in a directory without relying on `shell sed` magic and algorithms.

Add numbering for the different solutions.

Co-Authored-By: Francisco <femolina@uc.cl>

Move the description that solution 3 and 4 cannot declare dependencies to the introduction. I also reworded the "if only one should be used" to use the numbering, explain 1 over 2 and match more the style with the other one.

Co-Authored-By: Francisco <femolina@uc.cl>

Co-Authored-By: Leandro Lanzieri <leandro.lanzieri@haw-hamburg.de>

aabadie · 2020-06-25T17:02:29Z

Let me give my opinion on the different approaches (with a big STM32 bias):

option 1 (one folder/per cpu model): I wouldn't go this way. Because in the STM32 case, with dozens of models, it will be impossible to be exhaustive without supporting one board per cpu model. The directory structure will become huge and even a minor change might have an impact in several directories, so hard to review.
option 2 (ifneq based handling): this is how it's done with stm32 (see stm32_line.mk for example). Even if it allows for good code factorization, this is hard to maintain. And to support all STM32 models, this can become a hell of ifneq, very error prone. I'm not in favor of this one.
option 3 (per model definition): I prefer this approach over options 1 and 2, it's explicit and easy to maintain.
option 4 (grouped model definition): this is the one I like the most and is the direction I'd like to take for STM32 cpus. In the end, it will become the database of existing STM32 cpu models available with all required informations: rom/ram len, number of irqs, number of flash page, etc. We could even group per cpu line, etc.

Looking at the current situation in RIOT, I think option 4 is the best option.
Concerning the device tree discussion, I can't tell since I have no idea about what @jia200x and @leandrolanzieri have in mind about that.

Regarding what should be in this documentation page, I think we should drop options 1 and 2 from this doc, even if they are still used by existing CPUs supported by RIOT. Just adding a sentence at the beginning of the document saying that the approach is in progress and give good examples (efm32 is one of them).

Finally, I don't think this documentation should appear at the top level in the documentation outline. We should group build system related pages under a global build system entry. This page could be added there. In general, the top level documentation needs a bit of reorganization IMHO.

stale · 2020-12-28T04:58:55Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

fjmolinas requested review from fjmolinas, basilfx and leandrolanzieri October 9, 2019 14:22

fjmolinas reviewed Oct 9, 2019

View reviewed changes

This was referenced Oct 9, 2019

Build dependencies - processing order issues #9913

Open

Tracking: move CPU/CPU_MODEL to Makefile.features #11477

Closed

fjmolinas mentioned this pull request Oct 9, 2019

boards/slwstk6000b-*: move CPU_MODEL definition to Makefile.features #12410

Merged

fjmolinas requested a review from jia200x October 14, 2019 09:06

jia200x self-assigned this Oct 14, 2019

leandrolanzieri reviewed Oct 17, 2019

View reviewed changes

basilfx removed their request for review October 17, 2019 11:46

fjmolinas reviewed Oct 18, 2019

View reviewed changes

doc/doxygen/src/build-system-cpu-conventions.md Outdated Show resolved Hide resolved

fjmolinas requested a review from aabadie November 25, 2019 08:17

leandrolanzieri reviewed Nov 27, 2019

View reviewed changes

This was referenced Jan 21, 2020

cpu/efm32: cleanup #13174

Merged

boards/slwstk6000b-*: cleanup #13180

Merged

leandrolanzieri force-pushed the pr/doc/multiple_models_in_one_directory branch from ac8b1d2 to 2f7ff20 Compare March 9, 2020 14:49

cladmi and others added 15 commits June 25, 2020 18:41

doc: document how to handle multiple cpu_models in a directory

f4faaf1

This introduces ways of handling multiple cpu models defined in a directory without relying on `shell sed` magic and algorithms.

squash! doc: document how to handle multiple cpu_models in a directory

3716c4c

Add numbering for the different solutions.

fixup! doc: document how to handle multiple cpu_models in a directory

501daac

Co-Authored-By: Francisco <femolina@uc.cl>

fixup! doc: document how to handle multiple cpu_models in a directory

b1c6598

fixup! doc: document how to handle multiple cpu_models in a directory

52b2759

fixup! doc: document how to handle multiple cpu_models in a directory

6393c4f

Update doc/doxygen/src/build-system-cpu-conventions.md

cd0335f

Co-Authored-By: Francisco <femolina@uc.cl>

Update doc/doxygen/src/build-system-cpu-conventions.md

5f6c4e8

Co-Authored-By: Leandro Lanzieri <leandro.lanzieri@haw-hamburg.de>

Update doc/doxygen/src/build-system-cpu-conventions.md

46a3d6d

Co-Authored-By: Leandro Lanzieri <leandro.lanzieri@haw-hamburg.de>

Update doc/doxygen/src/build-system-cpu-conventions.md

d6c6261

Co-Authored-By: Leandro Lanzieri <leandro.lanzieri@haw-hamburg.de>

Update doc/doxygen/src/build-system-cpu-conventions.md

d6e4b88

Co-Authored-By: Leandro Lanzieri <leandro.lanzieri@haw-hamburg.de>

Update doc/doxygen/src/build-system-cpu-conventions.md

5b0abb3

Co-Authored-By: Leandro Lanzieri <leandro.lanzieri@haw-hamburg.de>

Update doc/doxygen/src/build-system-cpu-conventions.md

35e1676

Co-Authored-By: Leandro Lanzieri <leandro.lanzieri@haw-hamburg.de>

fixup! doc: document how to handle multiple cpu_models in a directory

ae04db7

aabadie force-pushed the pr/doc/multiple_models_in_one_directory branch from 2f7ff20 to ae04db7 Compare June 25, 2020 16:42

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Dec 28, 2020

fjmolinas added the State: don't stale State: Tell state-bot to ignore this issue label Jan 14, 2021

stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Jan 14, 2021

MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021

MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: document how to handle multiple cpu_models in a directory #12407

doc: document how to handle multiple cpu_models in a directory #12407

cladmi commented Oct 9, 2019

cladmi commented Oct 9, 2019

cladmi commented Oct 9, 2019

fjmolinas left a comment

fjmolinas Oct 9, 2019

fjmolinas Oct 9, 2019

cladmi Oct 9, 2019

fjmolinas Oct 9, 2019

cladmi Oct 9, 2019

fjmolinas Oct 9, 2019

cladmi Oct 9, 2019

fjmolinas Oct 9, 2019

cladmi Oct 9, 2019

fjmolinas Oct 9, 2019

cladmi Oct 9, 2019

leandrolanzieri Mar 9, 2020

fjmolinas commented Oct 14, 2019

leandrolanzieri left a comment

basilfx commented Oct 17, 2019

fjmolinas commented Oct 18, 2019

cladmi commented Oct 30, 2019

fjmolinas commented Nov 12, 2019

fjmolinas commented Nov 12, 2019

fjmolinas commented Nov 25, 2019

fjmolinas commented Nov 25, 2019

jia200x commented Nov 26, 2019

leandrolanzieri commented Nov 27, 2019

leandrolanzieri Nov 27, 2019

fjmolinas Nov 29, 2019

fjmolinas commented Nov 29, 2019

leandrolanzieri commented Mar 9, 2020

miri64 commented Jun 25, 2020

aabadie commented Jun 25, 2020 •

edited

Loading

stale bot commented Dec 28, 2020

		variables that have a list as value as the following solution does not apply.
		This is true for example for `FEATURES_PROVIDED`.

		Again in the case only delcarative values of specific variables, it can happen
		that many variables are somehow highly correlated.

doc: document how to handle multiple cpu_models in a directory #12407

Are you sure you want to change the base?

doc: document how to handle multiple cpu_models in a directory #12407

Conversation

cladmi commented Oct 9, 2019

Contribution description

Testing procedure

Issues/PRs references

cladmi commented Oct 9, 2019

cladmi commented Oct 9, 2019

fjmolinas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjmolinas commented Oct 14, 2019

leandrolanzieri left a comment

Choose a reason for hiding this comment

basilfx commented Oct 17, 2019

fjmolinas commented Oct 18, 2019

cladmi commented Oct 30, 2019

fjmolinas commented Nov 12, 2019

fjmolinas commented Nov 12, 2019

fjmolinas commented Nov 25, 2019

fjmolinas commented Nov 25, 2019

jia200x commented Nov 26, 2019

leandrolanzieri commented Nov 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjmolinas commented Nov 29, 2019

leandrolanzieri commented Mar 9, 2020

miri64 commented Jun 25, 2020

aabadie commented Jun 25, 2020 • edited Loading

stale bot commented Dec 28, 2020

aabadie commented Jun 25, 2020 •

edited

Loading