RTOS: Rework directory structure

[bulislaw]

Move RTOS implementation/internal files to rtos/src directory to avoid confusion and make
navigation easier.

Status

READY

@0xc0170 @c1728p9

[bulislaw]

@c1728p9 I've updated the PR so that RTX is only compiled for ARM cores.

[c1728p9]

My preference would be to keep header files in the same directory as source files when they are common to all targets. It make things easier to find and reduces the clutter with less directories. That is just my preference though. Aside from that this PR looks good to me.

@sg-, @geky, @0xc0170 Do you have any preferences or keeping .h and .cpp files in the same directory, or having a dedicated source directory?

[geky]

What's the benefit of src/ and include/ directories? From the linux point of view they are definitely an added pain.

[bulislaw]

I find it confusing and not readable when the API is mixed with implementation of system internals. I don't have strong feelings about keeping the RTOS C++ API cpp files together or separate with h, but I don't want the API headers mixed with files like mbed_boot.c etc. And I would vote to apply the same split for all directories (drivers, platform, hal). It makes the sources more readable, clearer and easier to navigate, understand and find things. Yes I know we have online docs, but lots of people don't bother looking at generated doxygen and will just go for the headers. I think keeping API and internal implementation of the system (internal mechanism) separate makes things fit more logically into bigger picture.

[0xc0170]

I think keeping API and internal implementation of the system (internal mechanism) separate makes things fit more logically into bigger picture

Also me, but this needs probably to be set to a bigger picture - how to apply this to other modules, and still be build-able. One point to consider - the paths (the problem that still exists) - this increases paths length. If we follow it for modules, in many cases would be +include and +src or sources.
I find it confusing now here as inside rtos/src folder there are still many headers, I can see only the top directory was changed (rtos and rtos/src).

@sg-, @geky, @0xc0170 Do you have any preferences or keeping .h and .cpp files in the same directory, or having a dedicated source directory?

I don't have strong preference, however having some organised structure is good improvement. But I fear we might have problems with our build system.

[bulislaw]

Not all headers are public, and source doesn't only apply to cpp/c files. I think having API files in the top level directory rtos/ and system implementation in rtos/src makes sense and makes things clearer as to what's there to look at/use and what's the implementation.

[geky]

And modules like mbed-events will have a source directory with a single c++ file?
https://github.com/bulislaw/mbed-os/tree/6198cdc857a86dd72185463c240a68c65d81661d/events

I'm still not sold on the src/inc split. c/h files represent a single unit and the directory split makes it harder to jump between them. Also C++ tosses the idea that header files are APIs only out the windows.

I would much rather see logical structure to directories, like lwip's eth/sys/core directories and the filesystem's fat/bd directories, since these add information into how the module is designed. src/inc directories don't really add information and adding directory requirements is going to discourage developers from creating logical structures.

A good example is mbed itself. mbed 2 used to contain and api/common split, which turned into just headers and source. As mbed grew, it gained a lot of non-driver code (callbacks, files, retarget code), but creating logical boundaries between driver code and platform code was difficult until we flattened the mbed directory and seperated it into driver/platform directories: #2878

This restructure is also going to take a significant amount of effort to adopt, and probably cause problems for tools/external links. Without a really good reason, I think this restructure is going to cause more problems then it is worth.

[bulislaw]

And modules like mbed-events will have a source directory with a single c++ file?
https://github.com/bulislaw/mbed-os/tree/6198cdc857a86dd72185463c240a68c65d81661d/events

I don't think that carries any overhead or penalty to have directories, even for single file. But what i'm saying is I don't care that much about source files of APIs, also I don't have strong opinion about it, I just don't want to have system guts mixed with API, it's confusing and messy.

[theotherjimmy]

I don't think that carries any overhead or penalty to have directories, even for single file.

Incorrect. See #3847

We have to be careful of what precedents we set. If we add a few more directories, We may end up in a situation that is even worse that we are in now.

I just don't want to have system guts mixed with API

Our recommendation for most APIs has always been: #include "mbed.h".

Further, We already have this, pretty bad. Tracking the first include path I see:

mbed.h -> drivers/DigitalIn.h -> hal/gpio_api.h -> <your-target-here>/device.h -> <your-hal-structs>

For the NCS36510 (not to pick on them, just to pick a concrete example)

mbed.h -> drivers/DigitalIn.h -> hal/gpio_api.h -> targets/TARGET_ONSEMI/TARGET_NCS36510/device.h -> targets/TARGET_ONSEMI/TARGET_NCS36510/objects.h

I would like to point out that we can't avoid this one: We need the size of these structs so that we can statically allocate them as part of the driver objects.

TL;DR: adding folders does not help with the API mixing with the guts, and may hurt "clonability" of mbed-os. I would love to see a method for removing the dependency between the internal HAL and the Drivers APIs.

[bulislaw]

It's not about including headers, it's about making the code easy to follow and use by other people. Having directories with mixed content: API and internals of the system makes it hard to navigate. Yes we have docs, but i don't find it as an good enough excuse not to structure the code in a clean way. A lot of people use headers rather than docs anyway.

I don't think that carries any overhead or penalty to have directories, even for single file.
Incorrect. See #3847

That doesn't really fit this case, even if I agree that having 7 directory deep with 160 character paths relative to mbed-os is too much. I would say it's a fallout from our build system or/and 'features' approach. In normal cases adding second level of directory names shouldn't cause troubles and exceed path length limits.

[bulislaw]

Maybe moving out implementation of RTOS C++ API out of this new directory and renaming it to something other than src would be more palatable? All suggestions are welcomed.

[theotherjimmy]

Oh, and FYI, you have conflicts now.

[sg-]

-1 for separating source and headers of things we develop until a later and well planned out time.

[bulislaw]

How about now? I moved API implementation to where it used to be rtos/ and created rtos/kernel directory, that contains RTX and RTX specific files.

[theotherjimmy]

Why do you need this?

[bulislaw]

That's the second commit, we only want to compile RTX for ARM cores and not for example for the host.

[theotherjimmy]

We can't compile for anything other than cortex cores ATM. You probably don't need this yet.

[bulislaw]

Not yet, but @c1728p9 is working on some host side testing.

[theotherjimmy]

I would expect to see this commit, and the directory changes, in that PR

[c1728p9]

Does this need to be changed from src to kernel?

[sg-]

While better I still dont understand what we're doing here other than fiddling. This commit RTOS: RTX should be compiled only for ARM cores indicates we'd not compile RTX but what about all the RTX calls in the rtos API? Those would be link errors, no? And everything else in features, etc that make use of RTOS API calls. I must be missing the bigger intention.

[bulislaw]

Moving the system implementation away from the API is for readability sake, so when you look at rtos directory you see some sort of structure there.

This commit RTOS: RTX should be compiled only for ARM cores indicates we'd not compile RTX but what about all the RTX calls in the rtos API? Those would be link errors, no? And everything else in features, etc that make use of RTOS API calls. I must be missing the bigger intention.

That's per @c1728p9 request not to compile RTX for host builds. And you are right it won't work for host until we have some sort of mocked implementation of RTOS2 API.

[theotherjimmy]

@c1728p9 Should add the Cortex Generic target in his own pull request when he implements host tests. I think it's out of scope for this PR.

[c1728p9]

I added Cortex Generic to #4776

[bulislaw]

I've removed the cortex generic changes from this PR.

[theotherjimmy]

Cool, that makes sense.

So we are left with rtos/rtx* => rtos/kernel/rtx*?

[theotherjimmy]

@bulislaw @geky @0xc0170 @c1728p9 Do we still want this?

[bulislaw]

I still would like to separate the RTX (+specific files) from the API.

[0xc0170]

I am closing this as the referenced PR should also close this one. In case not, please reopen