An idea to reduce duplicate code in ./boards/. #79
Comments
|
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. |
|
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 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! |
|
Very bad idea. Violates the basic principles or a modular operating system. We cannot consider this. |
|
Hi Alan, Thanks for your answer. @acassis wrote:
I take it you mean @acassis wrote:
Do you mean that for example a new driver for |
|
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. @patacongo wrote:
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 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? |
|
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. |
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 49The 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.shfrom PR #78 lists the following (file, common_%, file, common_%):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:
Actually, if you look closer, only the following 2 lines differ:
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.cfile.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/stm32f7This 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
The text was updated successfully, but these errors were encountered: