FreeRTOS / FreeRTOS-Kernel

FreeRTOS kernel files only, submoduled into https://github.com/FreeRTOS/FreeRTOS and various other repos.
https://www.FreeRTOS.org
MIT License
2.52k stars 1.05k forks source link

cmake issue #963

Closed barnatahmed closed 5 months ago

barnatahmed commented 5 months ago

I have been working with freeRTOS to integrate it in a project using cmake and i came across an issue that persists while working with FreeRTOS_PORT: GCC_ARM_CM4_MPU.

Host: windows 11 Target : stm32f411retx FreeRTOS_PORT: GCC_ARM_CM4_MPU

Description

I have read the CMakeLists.txt files of FreeRTOS to find out that it is creating two static libraries:

  1. port.a : containing the source files under /portable like port.c (or mpu_wrappers.c when working with mpu)
  2. kernel.a : containing the source files of the scheduler or the kernel( task.c, queue.c, timers.c, croutine.c, list.c, croutine.c...)

when working with some targets that don't use MPU, the kernel.a lib uses api from the port.a. However; The port.a is independent from the kernel.a. Therefore, in the linking phase would be successful if we link the kernel.a before the port.a. The order would be crutial.

the issue show up when i tried the integration on targets that uses MPU. While building, the project, the linking phase fails reporting that there are undefined refrences to some api's in kernel.a library and here is why:

-> the port.a library now contains "mpu_wrappers.c" source file that uses the apis developped in the kernel.a. As result the two static libraries ( port.a and kernel.a ) now depend from each other and whatever the order we give to linker it will eventually fail.

Proposed solution

All i had to do to fix this issue in my project is changing the target "freertos_kernel_port" from "STATIC" to "OBJECT" . the cmake will create only object files while building its targets. However, in /FreeRTOS_kernel/CMakeLists.txt line 258, the "freertos_kernel_port" target is being linked to "freertos_kernel" target created earlier in the same file (Line 233) which is "STATIC" library.

As conclusion, this modification will result only one final "STATIC" called "freertos_kernel.a" library containing all the sources of the freeRTOS kernel including the "portable" sources. Eventually, the issue is gone.

I tested this modification on other FREERTOS ports like GCC_ARM_CM4_MPU / GCC_ARM_CM4F / GCC_ARM_CM3_MPU / GCC_ARM_CM3 / GCC_ARM_CM0 / GCC_ARM_CM7 and it was working fine.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (8622bd5) 93.64% compared to head (591671b) 93.64%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #963 +/- ## ======================================= Coverage 93.64% 93.64% ======================================= Files 6 6 Lines 3194 3194 Branches 885 885 ======================================= Hits 2991 2991 Misses 91 91 Partials 112 112 ``` | [Flag](https://app.codecov.io/gh/FreeRTOS/FreeRTOS-Kernel/pull/963/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FreeRTOS) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/FreeRTOS/FreeRTOS-Kernel/pull/963/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FreeRTOS) | `93.64% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FreeRTOS#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Skptak commented 5 months ago

Hey @barnatahmed, thanks for this contribution, and nice catch on this issue with the CMake system!

This looks good to me, based off what I know about CMake. I've asked @archigup and @paulbartell to take a look at it as well since they're a more knowledgeable with CMake than I am

paulbartell commented 5 months ago

@barnatahmed I agree fully with your conclusion here. A single static library makes much more sense. However, there is an issue with your implementation. As-is, the objects generated by freertos_kernel_port will not be linked into the freertos_kernel static lib.

Could you do the following:

  1. Create a separate INTERFACE target for header files / includes of the kernel port (say freertos_kernel_port_headers)
  2. Update the comments / error messages in the root CMakeLists.txt to refer to freertos_kernel_port as an object library rather than a static library.
  3. add freertos_kernel_port to freertos_kernel as a PRIVATE dependency rather than a PUBLIC dependency.
  4. add freertos_kernel_port_headers to freertos_kernel as a PUBLIC dependency.

Thanks much, Paul

barnatahmed commented 5 months ago

Hello @paulbartell and @Skptak, I hope you are feeling great today. Thank you very much for your replies. I checked my implementation and i made sure that the objects of the freertos_port target are actually linked to freertos_kernel static library. I dumped the symbols of freertos_kernel.a in text file and i found the files of freertos_port and its functions actually exist.

Anyway, I am much more convinced with the implementation suggested by @paulbartell. I will test and commit the new changes as soon as possible.

Thank you very much once again. Ahmed BARNAT

text file that contains the symbols of freeRTOS_kernel.a built after my first implementation. nul.txt

sonarcloud[bot] commented 5 months ago

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

barnatahmed commented 5 months ago

Hello @paulbartell, i hope you are feeling great.

i modifed the /portable/CMakeLists.txt and now the "freertos_kernel_port_headers" is linked to "freertos_kernel_port" via PUBLIC keyword as you requested.

Thank you very much Ahmed BARNAT