boards/nrf52: introduce shared dependencies in common/nrf52 and update boards

[aabadie]

Contribution description

When reviewing #11769, I found that softdevice package and nrf802154 dependencies could be moved in common files under boards/common/nrf52 using the Makefile.nrf52<family>.dep pattern. This is a bit similar to the Makefile.nrf52832.features added in #11769.

So this PR is proposing this change and applies it to nrf52840 based boards as well as some nrf52832 based boards. Note that for some nrf52832 boards, the softdevice is now added (nrf52832-mdk, acd52832).

Maybe the naming the new files could be improved (use softdevice and nrf802154 instead of nrf52832 and nrf52840 instead ? This would also allow to add an nrfmin dep file for example).
Suggestions are welcome.

Testing procedure

Build and flash gnrc_networking on related boards and verify the expected dependencies are pulled in (softdevice on nrf52832, nrf802154 on nrf52840)

Issues/PRs references

None

[cladmi]

Are these CPU_MODEL or common board specific ?

[aabadie]

Are these CPU_MODEL or common board specific ?

Not exactly CPU_MODEL which is defined as nrf52840xxaa or nrf52832xxaa. Also some nrf52832 boards doesn't use softdevice but nrfmin by default (thingy52, ruuvitag), this is why I even suggested another naming for these files.

[cladmi]

If all Makefile files are concerned, maybe even a dedicated README at some point, why not another common directory ?
This pattern with a Makefile.name.dep is somehow the same as common/name/Makefile.dep when there is only one name.

There may be a reason to not do it with a common directory, I am just asking before introducing this pattern that should be supported.

One reason I could see is if it could prevent having 500 directories for each variations.
There is a similar pattern in cpu as many cpu models are implemented in one cpu directory. These multiple implementation currently rely on either ifeq, wildcard/grep (in efm32 to include the right definition), or makefile algorithmic like in kinetis, and these ones could benefit from a similar change.

[cladmi]

which is defined as nrf52840xxaa or nrf52832xxaa

They are too long for me to see a difference when reading :D

[aabadie]

@haukepetersen, maybe this PR could be of interest for you (just saw 2b34b45 which is also done by PR).

[haukepetersen]

Yes, this is interesting to me, I just missed this PR when doing #12114 and #12113, sorry...

As stated before, I would not mind to go with this one and close my PRs. However, there are some fixes from #12114 still missing here, so would you mind to sync them into this PR? Than I can just close both of mine...

[aabadie]

so would you mind to sync them into this PR?

I can adapt. After having a look, I see that the nimble_netif module was made the default (instead of softdevice) in #11578 (at least for nrf52dk).
Initially this PR was generalizing the softdevice for nrf52832 based boards.

[aabadie]

on the contrary of #12114, ruuvitag and thingy52 are not using the shared nrf52832 dependency file in this PR. This is because nrfmin won't be built by default on those boards by the CI.
We could just keep one of them with nrfmin if needed.

Another approach could be to play with features and have test applications requiring nrfmin/softdevice/nimble/nrf802154 features explicitly. The dependency resolution would then be managed in another place than in boards.

EDIT: the paragraph above doesn't make sense regarding auto_init stuff. But there is still the question of the default stack used with auto_init_netdev_default. And we should still have at least one test application per stack: there's already one for softdevice but not for nrfmin for example.

[haukepetersen]

LGTM, just need to run some tests, will do so ASAP...

[haukepetersen]

Tested different radio options for the effected boards, everything compiles as expected and no more 'dual radio configs' are to be seen -> ACK

[haukepetersen]

I am all good with this PR.

@cladmi IMHO introducing even more common boards folders here for only 1 or 2 trivial files seems to be overkill to me. I would rather go the opposite direction in the future and see if there is some way we can slim down this common/nrf5x mess in the boards again. So are you ok with merging this PR for now?

[haukepetersen]

@cladmi ping :-)

[cladmi]

For me they are not "trivial files", there are a part of a board definition. There was already "features", now "dep" and next step will require putting an "include". Boards could be implemented in a single C file but they are not, even if they are just "trivial files".

Keeping one directory allows consistent behavior and handling in the build system. I am still with the goal and doing cleaning so that for a board or cpu, specifying a list of directories could be enough.
It also allows adding a doc.txt or README. Currently nothing explains what nrf52832 is.

Even if the directory is a subdirectory of common/nrf52 like common/nrf52/nrf52832, I would find it better.

[haukepetersen]

I see your point. However, I think we need to be practical here! In this context the meaning of nrf52832 or nrf52840 is obvious. IMO a set of specific Makfiles, from which a board implementation can compose its configuration is a nice way to get a lean, generic code path without any unnecessary code duplication.

One problem I see with the 'shared-configuration-tree' for the Nordic boards is that we don't have a strict tree (at the moment) but rather some overlapping groups: e.g. common/nrf52xxxdk contains boards of both variants. So this can not so easily be covered by pure folder based dependencies...

I guess I will see if I can come up with something else then.

[aabadie]

@haukepetersen, if you agree, I think we can merge this PR as it is now.

[aabadie]

@haukepetersen, how should we proceed here ?

[haukepetersen]

I would not mind merging this, but @cladmi's concerns are not addressed. So I think we still owe him to propose something more in line with this arguments?!

[haukepetersen]

I also just added a separate test app for the nrfmin driver, so we can simplify this PR by taking out the 'special' handling for those few boards that use this as default right now -> #12314

[haukepetersen]

Ok, just looked into this a little bit more, and I want to propose the following:
we merge this PR as is, and I will then add another PR on top, adding the additional cleanup possible through #12314, and also some more refactoring to take care of the concerns raised by @cladmi

@cladmi: would that work for you?

[aabadie]

Rebased on top of master and fixed conflicts. @cladmi @haukepetersen how shoud we proceed ? I'm fine with @haukepetersen's proposal in #11792 (comment)

[cladmi]

It's on my list to look at.

[cladmi]

Done automatically as CPU is defined.

[cladmi]

RIOT/Makefile.dep

Lines 8 to 13 in a7cb0a4

	# Transitional conditional include until all boards define 'CPU' in
	# Makefile.features
	ifneq (,$(CPU))
	# include cpu dependencies
	-include $(RIOTCPU)/$(CPU)/Makefile.dep
	endif

[cladmi]

On my side I would still rather namespace with a directory than with the file names. And a directory in common/nrf52 would give the same verbosity

-include $(RIOTBOARD)/common/nrf52/Makefile.nrf52832.dep
+include $(RIOTBOARD)/common/nrf52/nrf52832/Makefile.dep

[aabadie]

And a directory in common/nrf52 would give the same verbosity

I'll follow your suggestion then and adapt this PR.

[aabadie]

@haukepetersen, @cladmi, I've updated the PR following @cladmi's suggestion. The dependencies (and features) are now included conditionally from the nrf52 common directory.

@haukepetersen, now that there's a nrfmin test application, I can drop the nrfmin automatic dependeny for ruuvitag and thingy52 boards. This way we are consistent across all nrf52(832) boards.

[haukepetersen]

now that there's a nrfmin test application, I can drop the nrfmin automatic dependeny for ruuvitag and thingy52 boards. This way we are consistent across all nrf52(832) boards.

nice! Will look through the code now once more, just to be safe

[haukepetersen]

Just this minor simplification, else everything looks nice to me!

Also tested all possible radio configurations for nimble_netif, nrfmin, nordic_softdevice_ble, nrf802154 for both nrf52832 and nrf52840 based boards, everything builds as expected.

[haukepetersen]

now look at that included file, we might simply want to drop nrf52832/Makefile.features and decleare the softdevice feature in the if block directly. If ever (and that is to be quite unexpected) there is additional feature(s), we can always re-introduce that file...

[aabadie]

Done, squashed and rebased (there was a conflict introduced by #12290)

[haukepetersen]

Wonderful -> ACK

[haukepetersen]

All green -> lets go.