RIOT-OS / RIOT

RIOT - The friendly OS for IoT
https://riot-os.org
GNU Lesser General Public License v2.1
4.91k stars 1.98k forks source link

Discussion: pseudo-boards / board variants with different configurations #14681

Open miri64 opened 4 years ago

miri64 commented 4 years ago

Currently there are a few boards defined as pseudo-modules to include differing board configurations, e.g.

https://github.com/RIOT-OS/RIOT/blob/6adf07caf04e9439087b4cd5ebcdc961f6df0b7f/boards/esp32-ttgo-t-beam/Makefile.include#L1

https://github.com/RIOT-OS/RIOT/blob/6adf07caf04e9439087b4cd5ebcdc961f6df0b7f/boards/feather-m0/Makefile.include#L1

This can in my opinion easily lead to a deep well of untested code since those board variants are effectively invisible to the CI and the build system. We have other examples, like the different revisions of remote-reva and remote-revb, the SiLabs 6000b (see boards/slwstk6000b-*), and especially the STM Nucleo variants where this isn't the case. The question is: Do we somehow want to group those variants or to go the route of the past and divide them up into each of their separate directories?

See also discussion in https://github.com/RIOT-OS/Release-Specs/pull/185#issuecomment-667898724 ff

maribu commented 4 years ago

@cladmi has strongly argued towards separate directories when this discussion last popped up in the mailing list. Nobody opposed this, so I think this can be considered a consensus.

I would just stick with that consensus, until we gain support in the CI to cover board variants other than creating distinct board folders for each configuration tuple.

fjmolinas commented 4 years ago

As so far all integration in RIOT expects one BOARD for directory, it should be kept that way. If the approach want's to be changed (to use PSEUDOMODULES or whatever) then all the handling around it should be adapted as well. I think this issue is the right place to have this discussion, but for now the "offending" BOARDs should be reverted to having a directory IMO.

So what are the cons of having one BOARD per directory? I personally don't mind it and it seems often that it ends up being the best way to model things as is also evidenced by the discussion in https://github.com/RIOT-OS/RIOT/issues/11674 for CPU_MODEL.

gschorcht commented 4 years ago

My biggest concerns were:

  1. The number of board directories could further explode and the compilation time in CI will further increase.
  2. A lot of tests are compiled for each board version even if the test is not affected by the differences.
  3. Multiple copies of the same documentation reside in different directories and are hard to maintain.
  4. Full copies of configurations in different directories that are 95% or even more identical.

An example for problem 2 are tests that use only features which are provided by the CPU and not by the board. Even if board features are used, the board revision might not affect the test as long as they are don't used in HIL tests, e.g., the periph_gpio test.

Examples for problems 3 and 4 are feather-m0 and feather-m0-wifi. The only difference is the WiFi interface on board. Other examples are remote-reva and remote-revb. The only difference is the LED pin configuration and the micro-SD card pin definition. However, exactly the same documentation is used in both board directories. I had to learn this when I fixed broken URLs some months ago.

I know, if multiple boards use equal or similar configurations, a common board definition might be used. But quite often the differences between different board versions, especially between different board revisions, are so small that it is not worth having a new common board definition.

IMHO, the approach that @benpicco used for openlabs-kw41z-mini-256kib could solve problems 3 and 4 for the moment. According to this approach, feather-m0-wifi would have its own directory that only adds the WiFi module, but would simply include everything else from feather-m0. Having a common board definition would be an overkill in this case, in my opinion.

gschorcht commented 4 years ago

Additional comment. We should at least try to group the boards in the documentation a bit more. The flat board list in the documentation is quite long and hard to read. There are some groupings such as ESP8266, ESP32 and STM32 Nucleo-*, but there could be more.

maribu commented 4 years ago
  1. The number of board directories could further explode and the compilation time in CI will further increase.

I agree that a huge number of board directories is not helpful for human beings keeping track of supported boards. But if we indeed want CI coverage of every board configuration, we have to pay the price for it.

  1. A lot of tests are compiled for each board version even if the test is not affected by the differences.

This is true. But this also applies to boards in general: I bet that we would be able to catch at least 3 out of 4 compilation issue by just compiling for only a single board of each CPU family. But still, catching the remaining 25% of issues might be worth the effort.

If there would be a way to automatically detect which board & application tuples are indeed affected by a PR so to only build them, this would be nice. But not that this solutions would need to a) be easy to maintain and b) be actually faster (remember that ccache is doing quite a good job at compiling mostly identical stuff again). So far, ccache was the best solution to my

  1. Multiple copies of the same documentation reside in different directories and are hard to maintain.

Only if the doc as actually copy-pasted. E.g. for the Bluepill and Blackpill (which are identical except for pin layout and status LED), the documentation is done in boards/common/blxxxpill/ and the individual boards refer to it. A common board directory should be created for mostly identical boards anyway to reduce code duplication, so it can also be used to reduce documentation duplication.

  1. Full copies of configurations in different directories that are 95% or even more identical.

Again, common board folders are a solution.

But I fully agree that this separate board folder approach has scalability issue. Let's say a single board comes with four extension slots and we have four distinct extension cards that could be added to each slot. That single board would require 5^4 board directories if we would want to have every configuration combination being build by Murdock. Adding 625 board directories in such a case is clearly not an option.

cladmi commented 4 years ago

I know, if multiple boards use equal or similar configurations, a common board definition might be used. But quite often the differences between different board versions, especially between different board revisions, are so small that it is not worth having a new common board definition.

One technical fix that would help for variants, is allow a board common to be a board. (same for cpu) This would allow easily extending a board when you add a shield, or an external peripheral.

The technical limitation, last time I checked, was that a board implements the board module, board_init(), periph, instead of having it namespaced. If a board defines the board_BOARDNAME module, the board_init_BOARD_NAME function, and maybe playing with headers too, it is just a matter of referring the right one in the generic board_init, board.h, and properly call/include the common one when you depend on one.

It requires cleanup as I remember some nucleo boards defining board_init in the common one (or periph_init maybe).

The correct includes was already achieved in makefiles IIRC.

stale[bot] commented 3 years ago

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.

miri64 commented 3 years ago

I think this should still be discussed.

stale[bot] commented 2 years ago

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.