You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/01/11 12:11:17 UTC
[GitHub] [incubator-nuttx] wingunder opened a new issue #79: An idea to
reduce duplicate code in ./boards/.
wingunder opened a new issue #79: An idea to reduce duplicate code in ./boards/.
URL: https://github.com/apache/incubator-nuttx/issues/79
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/`?
```bash
$ 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_%):
```bash
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:
```shell
$ diff -u ./boards/arm/stm32f0l0g0/nucleo-l073rz/src/stm32_autoleds.c
```
```diff
./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 <ra...@railab.me>
@@ -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:
```C
#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:
```sh
$ 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] wingunder commented on issue #79: An idea to
reduce duplicate code in ./boards/.
Posted by GitBox <gi...@apache.org>.
wingunder commented on issue #79: An idea to reduce duplicate code in ./boards/.
URL: https://github.com/apache/incubator-nuttx/issues/79#issuecomment-573343893
Hi Greg,
Thanks for your answer. I appreciate your point of view.
I do understand that making a braking change in any common place will break all code that uses it.
The trade-off is that changing something, that was copied 50 or more times, takes much more time and is just as error prone as fixing it in a common place.
@patacongo wrote:
> The board directories are customization points and MUST NOT BE MERGED INTO COMMON FILES.
>.
>.
>What you are proposing WILL damage the OS.
This was not my intention. I wanted to move common stuff from ./boards/ into a common place inside ./boards/. From my understanding this cannot affect the OS, but it can affect other implementations inside `./boards/`.
I'm sorry if I'm misunderstanding the OS, but am I wrong to argue that ./boards/ is not part of the OS, as ./boards/ has implementations that just use the OS?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] acassis commented on issue #79: An idea to reduce
duplicate code in ./boards/.
Posted by GitBox <gi...@apache.org>.
acassis commented on issue #79: An idea to reduce duplicate code in ./boards/.
URL: https://github.com/apache/incubator-nuttx/issues/79#issuecomment-573317081
Hi Pieter, in fact the idea of moving from the old configs/ directory to the new boards/ was exactly to let boards to share common code. Maybe to make it clear, the directory should be called ./boards/arm/common/drivers/ and so besides drivers we could have other common things (init/, script/, etc).
Also note that inside each board family (i.e. stm32, nrf52, etc) already exist a drivers/ directory. So I think putting this new drivers/ inside common/ will help to avoid users making mistakes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #79: An idea to
reduce duplicate code in ./boards/.
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #79: An idea to reduce duplicate code in ./boards/.
URL: https://github.com/apache/incubator-nuttx/issues/79#issuecomment-573345125
Reducing code duplication is not a priority. Never has been, never will be. Maintaining a strict module, independent architecture where changes in one are does not affect other areas is a high priority. We get there via a modulare, siloed architecture with no sharing between silos.
I am VERY strongly opposed with the urge to combine code for no reason than the other people are obsessed with combining code. It is unhealthy for the OS and will be resisted with every fiber.
There has never been a single case where duplicating code has caused any significant issues. yes, sometimes people have the same problem but that had been corrected in similar code. That happens maybe a couple of times per yeard and is immediately resolved. But combined code has a far worse problem (aside from trashing the architecture): Peoplel change the combined code for their own purposes and break everyone else's platforms. That is no acceptable and will not be allowed to happen (at least any futher that it already has).
This is a bad topic and is not going get you any support. I suggest doing something better with your time. This is unacceptable.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #79: An idea
to reduce duplicate code in ./boards/.
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #79: An idea to reduce duplicate code in ./boards/.
URL: https://github.com/apache/incubator-nuttx/issues/79#issuecomment-573319328
I am opposed to these kinds of changes. The board directories are customization points and MUST NOT BE MERGED INTO COMMON FILES. That corrupts the modulare architecture of the OS and cannot be permitted.
There are things that are common not particularly customization points (like some USB Host logic). But I would resist strong any willy nilly combination of code just because it looks the same today (it might look different tomorrow, that is the nature of customization points).
I would refer you to the top-level INVIOLABLES.txt under "The Enemies."
109 Sometimes Code Duplication is OK
110 --------------------------------
111
112 o Sometimes is better to duplicate some logic than to introduce coupling
Any rampant combination of logic for now reason than the OCD compulsion to due so must be resisted. What you are proposing WILL damage the OS.
Autoleds, in particular, must not be combined!
Anyone who supports this proposal has no sense or understanding of the NuttX architecture or modularity goals. This would an absolute disaster for the OS!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #79: An idea to
reduce duplicate code in ./boards/.
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #79: An idea to reduce duplicate code in ./boards/.
URL: https://github.com/apache/incubator-nuttx/issues/79#issuecomment-573319328
I am opposed to these kinds of changes. The board directories are customization points and MUST NOT BE MERGED INTO COMMON FILES. That corrupts the modulare architecture of the OS and cannot be permitted.
There are things that are common not particularly customization points (like some USB Host logic). But I would resist strong any willy nilly combination of code just because it looks the same today (it might look different tomorrow, that is the nature of customization points).
I would refer you to the top-level INVIOLABLES.txt under "The Enemies."
109 Sometimes Code Duplication is OK
110 --------------------------------
111
112 o Sometimes is better to duplicate some logic than to introduce coupling
Any rampant combination of logic for now reason than the OCD compulsion to due so must be resisted. What you are proposing WILL damage the OS.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] acassis edited a comment on issue #79: An idea to
reduce duplicate code in ./boards/.
Posted by GitBox <gi...@apache.org>.
acassis edited a comment on issue #79: An idea to reduce duplicate code in ./boards/.
URL: https://github.com/apache/incubator-nuttx/issues/79#issuecomment-573317081
Hi Pieter, in fact the idea of moving from the old configs/ directory to the new boards/ was exactly to let boards to share common code. Maybe to make it clear, the directory should be called ./boards/arm/common/drivers/ and so besides drivers we could have other common things (init/, script/, etc).
Also note that inside each board family (i.e. stm32, nrf52, etc) already exist a drivers/ directory. So I think putting this new drivers/ inside common/ will help to avoid users making mistakes.
Since @jerpelea is was to responsible to this original change, we need to wait for this feedback.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] wingunder commented on issue #79: An idea to
reduce duplicate code in ./boards/.
Posted by GitBox <gi...@apache.org>.
wingunder commented on issue #79: An idea to reduce duplicate code in ./boards/.
URL: https://github.com/apache/incubator-nuttx/issues/79#issuecomment-573342989
Hi Alan,
Thanks for your answer.
@acassis wrote:
> Maybe to make it clear, the directory should be called ./boards/arm/common/drivers/ and so besides drivers we could have other common things (init/, script/, etc).
I take it you mean `./boards/arm/common/drivers/ `, `./boards/arm/common/init/`, `./boards/arm/common/init/`, etc?
@acassis wrote:
> Also note that inside each board family (i.e. stm32, nrf52, etc) already exist a drivers/ directory. So I think putting this new drivers/ inside common/ will help to avoid users making mistakes.
Do you mean that for example a new driver for `./boards/arm/stm32/<newboard>/src/<newdriver>.*` will have to live in `./boards/arm/common/drivers/`, so that `./boards/arm/<stm32variants>/<board>/` can re-use it in a modular way?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #79: An idea to
reduce duplicate code in ./boards/.
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #79: An idea to reduce duplicate code in ./boards/.
URL: https://github.com/apache/incubator-nuttx/issues/79#issuecomment-573319565
Very bad idea. Violates the basic principles or a modular operating system. We cannot consider this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo closed issue #79: An idea to reduce
duplicate code in ./boards/.
Posted by GitBox <gi...@apache.org>.
patacongo closed issue #79: An idea to reduce duplicate code in ./boards/.
URL: https://github.com/apache/incubator-nuttx/issues/79
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services