FreeRTOS-Kernel icon indicating copy to clipboard operation
FreeRTOS-Kernel copied to clipboard

Adding in ability to support a library for freertos_config and a custom port

Open phelter opened this issue 3 years ago • 4 comments

Description

DEPRECATE: CMake: FREERTOS_CONFIG_FILE_DIRECTORY configuration and replace it with expected library freertos_config Rationale - some configuration settings can now be added to the freertos_config target eg:

target_compile_definitions(freertos_config
  INTERFACE
    projCOVERAGE_TEST=0
)

Added support for FREERTOS_PORT=A_CUSTOM_PORT - this allows the inclusion of a port as a library as well that can be linked to the FreeRTOS-Kernel appropriately.

Assorted fixes:

  • spelling: Compiller -> Compiler
  • missing link to Threads::Threads (pthreads) for GCC_POSIX build.

Test Steps

Setup a local cmake project and use the following project file to include FreeRTOS-Kernel

FetchContent_Declare( freertos_kernel
  GIT_REPOSITORY [email protected]:phelter/FreeRTOS-Kernel.git
  GIT_TAG        feature/cmake-updates
)

FetchContent_Declare( freertos_plus_tcp
  GIT_REPOSITORY [email protected]:phelter/FreeRTOS-Plus-TCP.git
  GIT_TAG        cmake-support
  GIT_SUBMODULES "" # Don't grab any submodules since not latest
)

add_library(freertos_config INTERFACE)
add_library(FreeRTOS::Config ALIAS freertos_config)

target_sources(freertos_config
  INTERFACE
    inc/FreeRTOSConfig.h
    inc/FreeRTOSIPConfig.h
)

target_include_directories(freertos_config SYSTEM
INTERFACE
    inc
)

target_compile_definitions(freertos_config
  INTERFACE
    projCOVERAGE_TEST=0
)
# -----------------------------------------------------------------------------
set( FREERTOS_HEAP "4" CACHE STRING "" FORCE)
set( FREERTOS_PORT "GCC_POSIX" CACHE STRING "" FORCE)
if (CMAKE_CROSSCOMPILING)
  set(FREERTOS_PORT "GCC_ARM_CA9" CACHE STRING "" FORCE)
endif()

FetchContent_MakeAvailable(freertos_kernel)

# -----------------------------------------------------------------------------
set(FREERTOS_PLUS_TCP_BUFFER_ALLOCATION "1" CACHE STRING "" FORCE)
set(FREERTOS_PLUS_TCP_COMPILER "" CACHE STRING "" FORCE)
set( FREERTOS_PLUS_TCP_NETWORK_IF "POSIX" CACHE STRING "" FORCE)
if (CMAKE_CROSSCOMPILING)
  set(FREERTOS_PLUS_TCP_NETWORK_IF "ZYNQ" CACHE STRING "" FORCE)
endif()
set(FREERTOS_PLUS_TCP_TEST_CONFIGURATION "CUSTOM" CACHE STRING "" FORCE)
FetchContent_MakeAvailable(freertos_plus_tcp)

Related Issue
-----------
See #558 


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

phelter avatar Sep 30 '22 00:09 phelter

Codecov Report

Base: 94.30% // Head: 94.30% // No change to project coverage :thumbsup:

Coverage data is based on head (cb4b792) compared to base (cd1f51c). Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #571   +/-   ##
=======================================
  Coverage   94.30%   94.30%           
=======================================
  Files           6        6           
  Lines        2370     2370           
  Branches      579      579           
=======================================
  Hits         2235     2235           
  Misses         85       85           
  Partials       50       50           
Flag Coverage Δ
unittests 94.30% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov[bot] avatar Oct 04 '22 22:10 codecov[bot]

Thanks for your comments . I've responded with all of them, and have allowed the FREERTOS_CONFIG_FILE_DIRECTORY to be backwards compatible - but with a warning to update when used.

If you like I can separate the ports each into separate CMakeLists.txt as well which would help simplify and limit the case statements in the port directory to just one (which one it selects).

phelter avatar Oct 11 '22 19:10 phelter

Note - I have a follow-on PR for fixing the compiler warnings here: https://github.com/phelter/FreeRTOS-Kernel/pull/1 will change target to master once this PR is completed.

phelter avatar Oct 13 '22 20:10 phelter

@paulbartell @aggarg - Any chance we can move the needle on this PR?

I've performed all of the changes you've requested. And am waiting for this one to be merged to start the next one.

Thanks

phelter avatar Oct 23 '22 19:10 phelter

@paulbartell @aggarg - Ping again.

Need a maintainer to approve running the workflows for this PR and would appreciate some time to identify any further issues that need to be resolved.

Thanks

phelter avatar Oct 30 '22 23:10 phelter

IMO, defining and linking a target similar to freertos_config should be left to the user.

@paulbartell - You've requested a change. I am unclear as to what that change is that will get approval for this PR.

With no changes:

  • defining a target similar to freertos_config - is left to the user. DONE
  • linking a target similar to freertos_config - is done inside the FreeRTOS-Kernel/CMakeLists.txt to:
    • provide a standardized name of the target, so that it is known by the user what is needed and required for integration
    • error out at CMake Configure time (initial call of cmake that configures the build environment ) if those integration requirements are NOT met.

For Linking - with this PR - if freertos_config is not defined, the CMake Configure will fail to complete and provide a detailed description of what is needed by the user to integrate FreeRTOS-Kernel.

Are you asking NOT to link freertos_config to freertos_kernel ?

If this is the case - then the user will have NO indication that there is missing configuration required while performing the CMake Configure, and will it will only fail when they attempt the build the FreeRTOS-Kernel with missing header files - an no means of telling the user (while building) what the problem really is. This also goes against the programming methodology of Fail-Early-Fail-Fast.

If you are still requiring a change here, can you please provide a proposal of how to provide a standardized way a user is to link a target similar to the freertos_config and provide more details as to what they must do to integrate their config with freertos_kernel.

I am unclear as to what must be done next to move this PR along.

Thanks

phelter avatar Nov 01 '22 17:11 phelter

@AniruddhaKanhere , @aggarg or @archigup - would it be possible for one of you to approve this PR now that all of the issues/changes requested by @paulbartell are now resolved.

Thanks.

phelter avatar Nov 08 '22 01:11 phelter

@AniruddhaKanhere , @aggarg or @archigup - Ping again. Would appreciate a review of the changes requested by @paulbartell

phelter avatar Nov 12 '22 02:11 phelter

@AniruddhaKanhere , @aggarg or @archigup - Ping once again. Just so you know this has been opened closing in on almost 2 months now.

phelter avatar Nov 18 '22 07:11 phelter

@phelter, apologies for the delay, I was out of office and was travelling. I approve of your change!

Thank you for your patience and your efforts in creating this PR.

Thanks, Aniruddha

AniruddhaKanhere avatar Nov 18 '22 08:11 AniruddhaKanhere