-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mbed OS components as OBJECT libs #21
Conversation
b39b230
to
3c3567c
Compare
Thanks for taking a look at this! I'm not quite 100% up to date with the target refactor stuff but I think I get most of what's going on. Is this how it currently is?
As long as this is correct, maybe we should put this table in the comments somewhere because it's very confusing.
Can't you just keep the targets as they currently are (interface libraries containing flags and sources)? Then just link the targets to both |
Yes the above is correct. The table is very usefull, I'll add it to our Readme for now.
Yes, I came to the same conclusion. Once I refactored simple maxim target (consist of 3 simple subtargets with few files, compare to others we have). As result there were lot of dependencies. It become hard for me to follow what I should link where. The tree with interface libraries are easy to create and maintain. That is a win for a porting in-tree or custom targets.
That should not happen. I'll make a note about this either in the docs or the toplevel CMake. I'll continue developing this to test our tree structure if this is the best what we can get :) |
@0xc0170 let me know when yo want me to test in my setup, I'll report here. |
I was planning to do. I was able to get blinky and events example snippets working with the maxim. It failed with K64F due symbols from linker file, I'll have to fix that. If you can test with anything and report back, I'll fix. |
I fixed linker errors, I was missing how linker is dependent on a target library. K64F builds now for both toolchains. @ladislas Can you test? |
Yes, I'll do that after lunch |
I am trying it as well and hit the first problem. Symbols required by linker script are not defined :/ |
if you |
I'm hitting the same issue when trying to compile for the DISCO_F769. [2/277] Link line:
FAILED: extern/mbed-os/targets/TARGET_STM/TARGET_STM32G0/TARGET_STM32G031xx/mbed-stm32g031xx.link_script.ld
cd /Users/ladislas/dev/ladislas/mbed-cmake-template/extern/mbed-os/targets/TARGET_STM/TARGET_STM32G0/TARGET_STM32G031xx && /usr/local/bin/arm-none-eabi-gcc @/Users/ladislas/dev/ladislas/mbed-cmake-template/_build/DISCO_F769NI/extern/mbed-os/compile_time_defs.txt -E -x assembler-with-cpp -P /Users/ladislas/dev/ladislas/mbed-cmake-template/extern/mbed-os/targets/TARGET_STM/TARGET_STM32G0/TARGET_STM32G031xx/TOOLCHAIN_GCC_ARM/stm32g031xx.ld -o /Users/ladislas/dev/ladislas/mbed-cmake-template/_build/DISCO_F769NI/extern/mbed-os/targets/TARGET_STM/TARGET_STM32G0/TARGET_STM32G031xx/mbed-stm32g031xx.link_script.ld
In file included from /Users/ladislas/dev/ladislas/mbed-cmake-template/extern/mbed-os/targets/TARGET_STM/TARGET_STM32G0/TARGET_STM32G031xx/TOOLCHAIN_GCC_ARM/stm32g031xx.ld:18:
/Users/ladislas/dev/ladislas/mbed-cmake-template/extern/mbed-os/targets/TARGET_STM/TARGET_STM32G0/TARGET_STM32G031xx/TOOLCHAIN_GCC_ARM/../cmsis_nvic.h:28:2: error: #error "MBED_ROM_SIZE not defined"
28 | #error "MBED_ROM_SIZE not defined"
| ^~~~~
[3/277] Link line:
FAILED: extern/mbed-os/targets/TARGET_STM/TARGET_STM32G0/TARGET_STM32G071xx/mbed-stm32g071xx.link_script.ld
cd /Users/ladislas/dev/ladislas/mbed-cmake-template/extern/mbed-os/targets/TARGET_STM/TARGET_STM32G0/TARGET_STM32G071xx && /usr/local/bin/arm-none-eabi-gcc @/Users/ladislas/dev/ladislas/mbed-cmake-template/_build/DISCO_F769NI/extern/mbed-os/compile_time_defs.txt -E -x assembler-with-cpp -P /Users/ladislas/dev/ladislas/mbed-cmake-template/extern/mbed-os/targets/TARGET_STM/TARGET_STM32G0/TARGET_STM32G071xx/TOOLCHAIN_GCC_ARM/stm32g071xx.ld -o /Users/ladislas/dev/ladislas/mbed-cmake-template/_build/DISCO_F769NI/extern/mbed-os/targets/TARGET_STM/TARGET_STM32G0/TARGET_STM32G071xx/mbed-stm32g071xx.link_script.ld
In file included from /Users/ladislas/dev/ladislas/mbed-cmake-template/extern/mbed-os/targets/TARGET_STM/TARGET_STM32G0/TARGET_STM32G071xx/TOOLCHAIN_GCC_ARM/stm32g071xx.ld:18:
/Users/ladislas/dev/ladislas/mbed-cmake-template/extern/mbed-os/targets/TARGET_STM/TARGET_STM32G0/TARGET_STM32G071xx/TOOLCHAIN_GCC_ARM/../cmsis_nvic.h:31:2: error: #error "MBED_ROM_SIZE not defined"
31 | #error "MBED_ROM_SIZE not defined"
| ^~~~~
[19/277] Link line:
ninja: build stopped: subcommand failed.
make: *** [all] Error 1 |
But it works for the K64F board. It also works for multiple function(mbed_configure_app_target target)
# Gcc Arm requires memap to be set with app name, equally to ARMClang
# TODO: move this to toolchain and set properly
if(MBED_TOOLCHAIN STREQUAL "GCC_ARM")
- target_link_options(mbed-core
- INTERFACE
+ target_link_options(${target}
+ PRIVATE
"-Wl,-Map=${CMAKE_CURRENT_BINARY_DIR}/${target}${CMAKE_EXECUTABLE_SUFFIX}.map"
)
endif()
endfunction() Without that, only the latest add_executable can be build with as the I can make a PR just for this change, or do you prefer to make it here? |
I've updated my template and made a PR: ladislas/mbed-cmake-template#7 To try: # clone repo
git clone https://github.com/ladislas/mbed-cmake-template
# checkout branch
git checkout --track origin/feature/move-to-vanilla-cmake+OBJLIB
# clone mbed
make mbed_clone BRANCH=cmake-object-libraries-with-interface
# config
make config TARGET_BOARD=K64F
# build
make TARGET_BOARD=K64F To activate the second executable target, uncomment the following line: |
@0xc0170 maybe the |
I've made a PR for the memory map. ARMmbed#14355 |
+1
I have to investigate today. There's g0 target for F7: |
Yes! I saw that too. It seems like all the linkers are being included for the ST target. |
Correct I made a change I should not have. I reverted it and trying to find a way to generate a linker script. I'll push a rebase once I get it working. |
21a3d7b
to
8f60484
Compare
I fixed the issue with linker. The current view is that we have to do some magic to build core library due to deps with rtos and targets. Internal details, people won't touch the magic, we will document what we are doing and why. We might fix one day. What users touch - targets libs - INTERFACE libraries, same as after refactor - it should be straightforward to add/remove targets. No change there. All user facing components are OBJECT libs. I ported events so far only. I'll wait for more feedback and refactor the entire tree once agreed. |
Otherwise this would not be visible to an app.
User facing targets should be just object libraries
Link what we can link with CMake. We still need copy core sources to mbed-os-obj. And expose also link options to app as linker flags are provided via them. Normally, we would link to target library but as it's interface, we would copy the sources as well, this is not what we want.
a5ca838
to
f972aab
Compare
Rebased to the latest master. |
We do not want an app to link core and also targets. As both actually create one core component. Therefore expose include dirs from targets to core. mbed-events for instance should just link to mbed-core and it should get all it needs.
🎉 I can confirm it works! I can compile both executables for the DISCO with only 250 files/"steps" (248 + 2) |
I made only comments improvements and some clean up. This is readme section bout object libs in our tree: https://github.com/0xc0170/mbed-os/blob/1d3e74f658ff020dbbd4d16ec7f8c51afdfde21f/tools/cmake/README.md#mbed-libraries, worth reading to understand what is this about. |
Thanks @ladislas for testing. Glad that this works. |
These are part of cmsis + core, they only extend them thus reverting the change back to INTERFACE lib.
Easier to understand
455c5c0
to
d36dc4d
Compare
I'll move this to I'll fix any comments even after the closure. |
Summary of changes
Open for discussion
The only target tested is MAX32620FTHR due to it's simple tree structure (easy to adjust).
Impact of changes
Migration actions required
Documentation
Pull request type
Test results
Reviewers