search

USCRPL / mbed-cmake

Use the power of CMake to create your MBed applications
36 stars 9 forks source link

Proposal: `mbed-cmake` as a subdirectory and generating config outside of the repo #3

Closed ProExpertProg closed 4 years ago

ProExpertProg commented 4 years ago

Motivation

Current setup

Currently, mbed-cmake works as a basis for a new MBed project, meaning that this repository should be pulled, and then source files should be added directly into the root folder of this repo. While that works as a setup for simple projects, it creates a tight coupling between this build tool and the project.

Disadvantages

This has numerous disadvantages:

and more.

Generally, separation of concerns is a very powerful programming paradigm, and it should also be leveraged by this project.

Why generate config files externally

Currently, mbed-cmake works in two general steps:

1. Hardware target setup In this step, we run configure_for_target.py to generate target-specific CMake and header files. Generally, a project isn't going to be switching targets too often, so we only do this during the initial project setup. 2. Build We will be doing this every time we change our source files, but we want to fully leverage the incremental build of CMake and make.

Hence, we want to commit the generated files, so that other contributors to the repository don't need to generate them on project setup. In the case of git submodules, it's very sketchy to commit a file inside a git submodule to the parent repository. In the direct download case, it still provides a better separation of concerns.

Proposed directory structure

We could easily achieve this with the following directory structure:

project-root/
+-- main.cpp
+-- CMakeLists.txt
+-- mbed-cmake/
|   +-- // this repository
+-- mbed-cmake-config/
|   +-- cmake/
|   |   +-- ... generated files
|   +-- config-headers/
|   |   +-- ... generated files

We can either include mbed-cmake as a git submodule or we can pull the whole repo as a .zip file and commit it as a whole.

The new setup

  1. Create an empty project
  2. Add the following CMakeLists.txt: 
    
cmake_minimum_required(VERSION 3.16) # or any other version

add_subdirectory(mbed-cmake)

cannot happen before you include mbed-cmake, as cmake will verify the toolchain before flags are set.

project(mbed-cmake-example LANGUAGES C CXX ASM)

add other subdirectories here

demo hello world program -- remove this once your builds and uploads are tested and working

add_mbed_executable(hello_world HelloWorld.cpp)

3. Add a `HelloWorld.cpp`
```cpp
#include "mbed.h"

Serial pc(USBTX, USBRX);

int main() {
    pc.baud(115200);

    while(true)
    {
        pc.printf("Hello world!\n");
        wait(1);
    }
}

Customizing the location of generated files

It is possible to customize the location of the generated files using the -p, --generated-path option on the python script, and via the ${MBED_CMAKE_GENERATED_CONFIG_PATH} in CMake, both of which default to ../mbed-cmake-config. However, the build will fail if those are modified.

Because MBed's build will look for mbed_config.h and mbed_target_config.h inside mbed-src/config-headers/, I put proxy header files there that point directly at ../../../mbed-cmake-config/config-headers/.... This means that those need to be modified should someone want to customize the location of the generated files.

I believe this is an acceptable downside:

  1. if you want to generate files externally, a fixed location is appropriate.
  2. if you want to generate files internally, you're already going to be modifying the internals of mbed-cmake, so changing the paths inside those two proxy files shouldn't be a problem.

Another possibility would be to invest more time and provide MBed's CMakeLists.txt the include path of the generated files. I had a quick look and estimated that it would take a lot of time, as the files are included in weird ways (mbed-cmake\mbed-src\CMakeLists.txt:24):

set(MBED_COMPILE_OPTIONS "SHELL:-include ${CMAKE_CURRENT_SOURCE_DIR}/config-headers/mbed_config.h" "SHELL:-include ${CMAKE_CURRENT_SOURCE_DIR}/config-headers/mbed_target_config.h")

I would prefer not to modify this CMake file to avoid further coupling between MBed and mbed-cmake

Future steps

Verification

I have successfully built the target, but I would really appreciate it if you guys could test this on hardware and with different targets.

Wiki updates

We should update the setup instructions if this PR is to be approved and merged. I'd be happy to help with those.

Further decoupling

Just as I submitted this PR, I realized that the .json and .mbedignore files need to be modified inside mbed-cmake, which should be OK as they're only needed for the generation step. Hence, committing them isn't an absolute must, but having them inside is certainly not ideal. I would appreciate guidance on how to approach separating those out.

My dream is that we can also decouple mbed-src from this repo. so that we can support upgrades to newer versions of MBed OS. Unclear whether that is possible due to the unignore modifications, but I believe that's a good long-term goal to have.

Add Contributing.md for this repo

It would be useful if there are standardized instructions on how to create PRs for this repo.

Lastly, thank you all for all the effort you put into this! It's a really great tool and I'm sure people will start using it massively. It is already very user-friendly, but we could make it even more so, and this PR is a step in that direction in my opinion.

multiplemonomials commented 4 years ago

Thanks for the help! I definitely think this is a cleaner structure, and I especially like how clean the top level CMakeLists becomes. However there is def a lot to do to get this merged. I agree that having mbed_app.json and .mbedignore in mbed-src/ is kinda ugly but I also can't think of a better solution, especially since some users would want to create .mbedignores in several subfolders of mbed-src/,

I'll test it out in the next couple days and see if it works OK for our targets here.

Once everything checks out, it would be great if you could help with some of the wiki updates. There's actually a lot of good content in this PR that would be good to add there,

ProExpertProg commented 4 years ago

@multiplemonomials thoughts on the semi-fixed location for generated files?

ProExpertProg commented 4 years ago

I removed the proxy config files as per your suggestion. Let me know if there's anything else to do.

multiplemonomials commented 4 years ago

oh lol whoops I just saw you already asked about that -include option, I missed that. Well looks like things are fixed now at any rate. If you were wondering, all those options do is force the preprocessor to include the indicated file at the start of each code file. The SHELL thing is some extra CMake weirdness though.

thoughts on the semi-fixed location for generated files?

I think how you implemented it is good! We do need to make it clear in the docs that you need to use both --generated-path and MBED_CMAKE_GENERATED_CONFIG_PATH or neither, but I think most people won't need to bother with that.

ProExpertProg commented 4 years ago

Yeah, the half-fixed config only applied with proxy files. Now, the generated files can be put anywhere.

ProExpertProg commented 4 years ago

@multiplemonomials just wondering what kind of turnaround we can expect? I was hoping we can start using it this Saturday, so do you think we could get this merged in before then? Anything I can do to help?

Also fine if that's not possible, just trying to plan ahead a bit.

multiplemonomials commented 4 years ago

Okay, checking it out now. For starters, it looks like add_subdirectory(mbed-cmake) creates some weird issues related to variable scope. I think we need to change it to an include file instead. I also found a couple other minor errors that pop up when trying to include mbed-cmake as a subproject, like the module path being wrong. Gah, I broke my own rule to never use CMAKE_SOURCE_DIR...

multiplemonomials commented 4 years ago

OK I'm gonna merge this, then apply my own changes.