Hi,
I noticed that there are a lot of file duplication in the ./boards/
directory. This is probably a very late point in time to try and fix this, but I am prepared to give it a go. In my PR #78, I added a tool for exposing this.
Before I start, I'd like to make it absolutely clear that I am not trying to say that this is bad or anything in those lines. I simply don't believe that duplicate code is good. It's hard to maintain and hard to stop from getting duplicated even more.
Let's take the following example:
How many stm32_autoleds.c files do we have in ./boards/
?
$ find ./boards/ -type f -name stm32_autoleds.c |wc -l
49
The answer is 49 files. This is not bad, just a fact.
Now the interesting part starts. We'll list the 20 most common combinations of files that are the most common. The ./tools/listcommonpercentage.sh
from PR #78 lists the following (file, common_%, file, common_%):
find ./boards/ -type f -name stm32_autoleds.c |grep -v nero |PERCENT_SIGN="" DELIM=" " xargs ./tools/listcommonpercentage.sh |sort -nk2 -nk4 |tail -20
./boards/arm/stm32/photon/src/stm32_autoleds.c 99 ./boards/arm/stm32l4/b-l475e-iot01a/src/stm32_autoleds.c 99
./boards/arm/stm32/stm3220g-eval/src/stm32_autoleds.c 99 ./boards/arm/stm32/stm3240g-eval/src/stm32_autoleds.c 99
./boards/arm/stm32/stm32f334-disco/src/stm32_autoleds.c 99 ./boards/arm/stm32/nucleo-f103rb/src/stm32_autoleds.c 99
./boards/arm/stm32/stm32f334-disco/src/stm32_autoleds.c 99 ./boards/arm/stm32/nucleo-f302r8/src/stm32_autoleds.c 99
./boards/arm/stm32/stm32f334-disco/src/stm32_autoleds.c 99 ./boards/arm/stm32/nucleo-f334r8/src/stm32_autoleds.c 99
./boards/arm/stm32/stm32f334-disco/src/stm32_autoleds.c 99 ./boards/arm/stm32/nucleo-l152re/src/stm32_autoleds.c 99
./boards/arm/stm32/stm32f334-disco/src/stm32_autoleds.c 99 ./boards/arm/stm32f0l0g0/b-l072z-lrwan1/src/stm32_autoleds.c 99
./boards/arm/stm32/stm32f334-disco/src/stm32_autoleds.c 99 ./boards/arm/stm32f0l0g0/nucleo-g071rb/src/stm32_autoleds.c 99
./boards/arm/stm32/stm32f334-disco/src/stm32_autoleds.c 99 ./boards/arm/stm32f0l0g0/nucleo-l073rz/src/stm32_autoleds.c 99
./boards/arm/stm32f0l0g0/nucleo-f091rc/src/stm32_autoleds.c 99 ./boards/arm/stm32f0l0g0/nucleo-f072rb/src/stm32_autoleds.c 99
./boards/arm/stm32f0l0g0/nucleo-g071rb/src/stm32_autoleds.c 99 ./boards/arm/stm32f0l0g0/b-l072z-lrwan1/src/stm32_autoleds.c 99
./boards/arm/stm32f0l0g0/nucleo-l073rz/src/stm32_autoleds.c 99 ./boards/arm/stm32f0l0g0/b-l072z-lrwan1/src/stm32_autoleds.c 99
./boards/arm/stm32f0l0g0/nucleo-l073rz/src/stm32_autoleds.c 99 ./boards/arm/stm32f0l0g0/nucleo-g071rb/src/stm32_autoleds.c 99
./boards/arm/stm32f7/stm32f746g-disco/src/stm32_autoleds.c 99 ./boards/arm/stm32f7/stm32f769i-disco/src/stm32_autoleds.c 99
./boards/arm/stm32h7/nucleo-h743zi/src/stm32_autoleds.c 99 ./boards/arm/stm32f7/nucleo-144/src/stm32_autoleds.c 99
./boards/arm/stm32h7/nucleo-h743zi/src/stm32_autoleds.c 99 ./boards/arm/stm32h7/stm32h747i-disco/src/stm32_autoleds.c 99
./boards/arm/stm32l4/nucleo-l496zg/src/stm32_autoleds.c 99 ./boards/arm/stm32f7/nucleo-144/src/stm32_autoleds.c 99
./boards/arm/stm32l4/stm32l4r9ai-disco/src/stm32_autoleds.c 99 ./boards/arm/stm32l4/stm32l476vg-disco/src/stm32_autoleds.c 99
./boards/arm/stm32/nucleo-f207zg/src/stm32_autoleds.c 100 ./boards/arm/stm32/nucleo-f303ze/src/stm32_autoleds.c 100
./boards/arm/stm32f0l0g0/stm32f051-discovery/src/stm32_autoleds.c 100 ./boards/arm/stm32f0l0g0/stm32f072-discovery/src/stm32_autoleds.c 100
So, the 20 most common file combinations are >= 99% copies of each other. Does it hurt? Well, only if these files have a bit of code in them. For sure they have code in them, 58 lines of it. This means that if we could get rid of the duplication, we'll save 58 * .99 * (20/2 - 1) = 516 lines of code.
https://github.com/apache/incubator-nuttx/blob/97bead5496614029e612b553daaab5856592df49/boards/arm/stm32f0l0g0/nucleo-l073rz/src/stm32_autoleds.c#L36-L94
Let's look at a diff of a random pair:
$ diff -u ./boards/arm/stm32f0l0g0/nucleo-l073rz/src/stm32_autoleds.c
./boards/arm/stm32f0l0g0/b-l072z-lrwan1/src/stm32_autoleds.c
--- ./boards/arm/stm32f0l0g0/nucleo-l073rz/src/stm32_autoleds.c 2019-12-13 17:09:50.704153380 +0000
+++ ./boards/arm/stm32f0l0g0/b-l072z-lrwan1/src/stm32_autoleds.c 2019-12-13 17:09:50.704153380 +0000
@@ -1,5 +1,5 @@
/****************************************************************************
- * boards/arm/stm32f0l0g0/nucleo-l073rz/src/stm32_autoleds.c
+ * boards/arm/stm32f0l0g0/b-l072z-lrwan1/src/stm32_autoleds.c
*
* Copyright (C) 2018 Gregory Nutt. All rights reserved.
* Authors: Mateusz Szafoni <[email protected]>
@@ -44,11 +44,10 @@
#include <debug.h>
#include <nuttx/board.h>
+#include <arch/board/board.h>
#include "stm32_gpio.h"
-#include "nucleo-l073rz.h"
-
-#include <arch/board/board.h>
+#include "b-l072z-lrwan1.h"
#ifdef CONFIG_ARCH_LEDS
Actually, if you look closer, only the following 2 lines differ:
#include "nucleo-l073rz.h"
#include "b-l072z-lrwan1.h"
So if we could include a file with a common name here, which should be easy, the files will be 100% copies.
The final step will be to find a place for the (common) stm32_autoleds.c
file.
I would suggest it has to live in ./boards/<somewhere>/drivers/
, but we have at least 5 for this case:
$ find ./boards/arm/ -maxdepth 1 -type d -name stm32\*
./boards/arm/stm32
./boards/arm/stm32h7
./boards/arm/stm32f0l0g0
./boards/arm/stm32l4
./boards/arm/stm32f7
This makes me think that ./boards/arm/drivers/
, which does not exist yet, would be the right place for it. If this is the right place, I'll have to add this directory and bind it into the whole make process, which should also not be such a problem.
I'll appreciate any feedback on this idea and I would like to add that I am prepared to continue with the implementation of this or any related idea, in order to reduce the code duplication.
Thanks & regards