Add option to not include heap in CMakeLists.txt #595

jasonpcarroll · 2022-11-30T22:20:27Z

Description

Added an option to disable adding a heap implementation to the FreeRTOS-Kernel in response to #594.

Test Steps

Related Issue

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

codecov · 2022-11-30T22:28:21Z

Codecov Report

Patch and project coverage have no change.

Comparison is base (d43062b) 93.62% compared to head (f22169d) 93.62%.

❗ Current head f22169d differs from pull request most recent head 2aff1a1. Consider uploading reports for the commit 2aff1a1 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #595   +/-   ##
=======================================
  Coverage   93.62%   93.62%           
=======================================
  Files           6        6           
  Lines        2508     2508           
  Branches      598      598           
=======================================
  Hits         2348     2348           
  Misses        107      107           
  Partials       53       53

Flag	Coverage Δ
unittests	`93.62% <ø> (ø)`

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

paulbartell · 2022-12-01T22:23:10Z

CMakeLists.txt

-    # If FREERTOS_HEAP is digit between 1 .. 5 - it is heap number, otherwise - it is path to custom heap source file
-    $<IF:$<BOOL:$<FILTER:${FREERTOS_HEAP},EXCLUDE,^[1-5]$>>,${FREERTOS_HEAP},portable/MemMang/heap_${FREERTOS_HEAP}.c>
+# Check if user requested to not inlude a heap implementation
+if(NOT DEFINED FREERTOS_DO_NOT_INCLUDE_HEAP)


Perhaps we should just not include a heap implementation if FREERTOS_HEAP is not set or set to 0?

I'm not sure it will compile without the heap definitions. You will have linker issues with missing symbols.

If you do not want a heap then create a custom heap source that errors out whenever the heap is used/consumed by the API it defines, and do:

set(FREERTOS_HEAP "heap_disabled.c")

Where heap_disabled.c is defined as:

void * pvPortMalloc( size_t xWantedSize ) { (void)xWantedSize; #if ( configUSE_MALLOC_FAILED_HOOK == 1 ) { vApplicationMallocFailedHook(); } #endif return NULL; } /*-----------------------------------------------------------*/ void vPortFree( void * pv ) { ( void ) pv; /* Force an assert as it is invalid to call this function. */ configASSERT( pv == NULL ); } /*-----------------------------------------------------------*/ void vPortInitialiseBlocks( void ) { } /*-----------------------------------------------------------*/ size_t xPortGetFreeHeapSize( void ) { return 0; }

If you want it in the FreeRTOS repo, then suggest making this an alternative heap implementation - as @paulbartell suggested and change the name to heap_0.c (NullOpt - a no-heap implementation of the heap API). - and place it with the other heap locations. Then you'd also have to change:

$<IF:$<BOOL:$<FILTER:${FREERTOS_HEAP},EXCLUDE,^[0-5]$>>,${FREERTOS_HEAP},portable/MemMang/heap_${FREERTOS_HEAP}.c>

To allow heap_0.c to be allowed.

@paulbartell @jasonpcarroll thoughts?

Just a note since I opened the issue #594: It will compile without linker issues if configSUPPORT_DYNAMIC_ALLOCATION is set to 0 in the config header.

If adding the heap_0.c as suggested above, then I guess I would be relying on the linker to discard those sections. That is ok, but it would be neat in my opinion if that was the default. It seems weird to have to pick a heap implementation if the project is set up to not support dynamic allocation.

I still stand by my previous comment - adding an additional FREERTOS_HEAP option for the enumeration is better than adding yet another configuration for the same feature FREERTOS_DO_NOT_INCLUDE_HEAP.

Since there is a linkage between FREERTOS_HEAP and SUPPORT_DYNAMIC_ALLOCATION then I suggest you make the linkage within the config via CMakeDependentOption - so that it is explicitly known that if SUPPORT_DYNAMIC_ALLOCATION is set to 1, you must configure FREERTOS_HEAP. Another method of fixing is to remove the SUPPORT_DYNAMIC_ALLOCATION and only use FREERTOS_HEAP=0 instead.

That way you have the error/issue identified even before compile - when building up the configuration instead.

configSUPPORT_DYNAMIC_ALLOCATION should be sufficient to deciding to include a heap implementation. The portable.h header should be changed so the heap functions are not declared when configSUPPORT_DYNAMIC_ALLOCATION==0. It does make sense to add a dependent section to the CMakeLists to make a heap selection IF configSUPPORT_DYNAMIC_ALLOCATION==1 THOUGH, I would prefer that application decisions like heap & port be made at the application level CMake and not at the kernel level CMake. This also has the advantage of a simpler path to a custom heap should one be desired.

In the CMake you do not have access to configSUPPORT_DYNAMIC_ALLOCATION and configSUPPORT_STATIC_ALLOCATION (from FreeRTOSConfig.h), right? I made a PR to change the default behaviour of FREERTOS_HEAP , see #807 . (I was unaware that there were active pull requests attempting to address the same issue.). I would like to contribute to fix this issue.

CMakeLists.txt

phelter · 2023-02-22T23:40:52Z

CMakeLists.txt

-    # If FREERTOS_HEAP is digit between 1 .. 5 - it is heap number, otherwise - it is path to custom heap source file
-    $<IF:$<BOOL:$<FILTER:${FREERTOS_HEAP},EXCLUDE,^[1-5]$>>,${FREERTOS_HEAP},portable/MemMang/heap_${FREERTOS_HEAP}.c>
+# Check if user requested to not inlude a heap implementation
+if(NOT DEFINED FREERTOS_DO_NOT_INCLUDE_HEAP)


I'm not sure it will compile without the heap definitions. You will have linker issues with missing symbols.

If you do not want a heap then create a custom heap source that errors out whenever the heap is used/consumed by the API it defines, and do:

set(FREERTOS_HEAP "heap_disabled.c")

Where heap_disabled.c is defined as:

void * pvPortMalloc( size_t xWantedSize ) { (void)xWantedSize; #if ( configUSE_MALLOC_FAILED_HOOK == 1 ) { vApplicationMallocFailedHook(); } #endif return NULL; } /*-----------------------------------------------------------*/ void vPortFree( void * pv ) { ( void ) pv; /* Force an assert as it is invalid to call this function. */ configASSERT( pv == NULL ); } /*-----------------------------------------------------------*/ void vPortInitialiseBlocks( void ) { } /*-----------------------------------------------------------*/ size_t xPortGetFreeHeapSize( void ) { return 0; }

If you want it in the FreeRTOS repo, then suggest making this an alternative heap implementation - as @paulbartell suggested and change the name to heap_0.c (NullOpt - a no-heap implementation of the heap API). - and place it with the other heap locations. Then you'd also have to change:

$<IF:$<BOOL:$<FILTER:${FREERTOS_HEAP},EXCLUDE,^[0-5]$>>,${FREERTOS_HEAP},portable/MemMang/heap_${FREERTOS_HEAP}.c>

To allow heap_0.c to be allowed.

amazonKamath · 2023-03-03T04:08:42Z

@phelter Thanks for the review and feedback.

…upported. Update the CMakeLists.txt to account for this

Skptak · 2023-09-21T17:22:18Z

/bot run formatting

Skptak · 2023-09-21T17:31:17Z

/bot run formatting

…ot just if it's used from this directoy

phelter · 2023-09-21T17:57:16Z

CMakeLists.txt

+include(${CMAKE_CURRENT_LIST_DIR}/include/CMakeLists.txt)
+include(${CMAKE_CURRENT_LIST_DIR}/portable/CMakeLists.txt)


Revert back - improper use of CMake.

Suggested change

include(${CMAKE_CURRENT_LIST_DIR}/include/CMakeLists.txt)

include(${CMAKE_CURRENT_LIST_DIR}/portable/CMakeLists.txt)

add_subdirectory(include)

add_subdirectory(portable)

Can you explain why the usage of CMAKE_CURRENT_LIST_DIR is an improper use of CMAKE?

The idea with this change is you no longer need to include the FreeRTOS-Kernel as a sub-directory, it can then be placed anywhere in your project as all of our cmake files aren't position dependent then. So instead of now where the kernel must be included in a sub-directory of your CMake File you would instead be able to include it from anywhere in your project

It's not that it is wrong, it is superfluous (not necessary) and is not required for what you were trying to achieve (The idea with this change...).

In any cmake project, one can add_subdirectory(free_rtos_kernel_dir) with the appropriate configuration beforehand and that free_rtos_kernel_dir can be anywhere in your project. I'm not sure why you would do anything other than this? Could you please elaborate your use case?

In recent changes - the CMakeFiles.txt of the various FreeRTOS components were updated to allow a very common way of importing libraries via CMake itself: FetchContent

FetchContent_Declare( freertos_kernel GIT_REPOSITORY https://github.com/FreeRTOS/FreeRTOS-Kernel.git GIT_TAG V10.6.1 ) ... set(FREERTOS_HEAP "4" CACHE STRING "" FORCE) set(FREERTOS_PORT "GCC_POSIX" CACHE STRING "" FORCE) FetchContent_MakeAvailable(freertos_kernel)

This will download the V10.6.1 version of the Freertos-Kernel into build/_deps
And then run the CMake from a local build directory in build/_deps.

To change the version - you then only change one line in the CMakeLists.txt that has the above example.

A great resource is Professional CMake for how to properly use the tool.

phelter · 2023-09-21T17:58:10Z

CMakeLists.txt

+    ${CMAKE_CURRENT_LIST_DIR}/croutine.c
+    ${CMAKE_CURRENT_LIST_DIR}/event_groups.c
+    ${CMAKE_CURRENT_LIST_DIR}/list.c
+    ${CMAKE_CURRENT_LIST_DIR}/queue.c
+    ${CMAKE_CURRENT_LIST_DIR}/stream_buffer.c
+    ${CMAKE_CURRENT_LIST_DIR}/tasks.c
+    ${CMAKE_CURRENT_LIST_DIR}/timers.c


Revert back -improper use of CMake. No need to include CURRENT_LIST_DIR unless it is in a generate expression.

Suggested change

${CMAKE_CURRENT_LIST_DIR}/croutine.c

${CMAKE_CURRENT_LIST_DIR}/event_groups.c

${CMAKE_CURRENT_LIST_DIR}/list.c

${CMAKE_CURRENT_LIST_DIR}/queue.c

${CMAKE_CURRENT_LIST_DIR}/stream_buffer.c

${CMAKE_CURRENT_LIST_DIR}/tasks.c

${CMAKE_CURRENT_LIST_DIR}/timers.c

croutine.c

event_groups.c

list.c

queue.c

stream_buffer.c

tasks.c

timers.c

phelter · 2023-09-21T18:05:10Z

CMakeLists.txt

-    # If FREERTOS_HEAP is digit between 1 .. 5 - it is heap number, otherwise - it is path to custom heap source file
-    $<IF:$<BOOL:$<FILTER:${FREERTOS_HEAP},EXCLUDE,^[1-5]$>>,${FREERTOS_HEAP},portable/MemMang/heap_${FREERTOS_HEAP}.c>
+# Check if user requested to not inlude a heap implementation
+if(NOT DEFINED FREERTOS_DO_NOT_INCLUDE_HEAP)


I still stand by my previous comment - adding an additional FREERTOS_HEAP option for the enumeration is better than adding yet another configuration for the same feature FREERTOS_DO_NOT_INCLUDE_HEAP.

Since there is a linkage between FREERTOS_HEAP and SUPPORT_DYNAMIC_ALLOCATION then I suggest you make the linkage within the config via CMakeDependentOption - so that it is explicitly known that if SUPPORT_DYNAMIC_ALLOCATION is set to 1, you must configure FREERTOS_HEAP. Another method of fixing is to remove the SUPPORT_DYNAMIC_ALLOCATION and only use FREERTOS_HEAP=0 instead.

That way you have the error/issue identified even before compile - when building up the configuration instead.

chinglee-iot · 2023-12-06T08:03:16Z

Close this PR due to it is included in #807.

Add option to not include heap in CMakeLists.txt

cdb854f

jasonpcarroll requested a review from a team as a code owner November 30, 2022 22:20

paulbartell reviewed Dec 1, 2022

View reviewed changes

phelter reviewed Feb 22, 2023

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

phelter suggested changes Feb 22, 2023

View reviewed changes

Mancent linked an issue Mar 5, 2023 that may be closed by this pull request

https://docs.github.com/articles/keeping-your-account-and-data-secure/ #638

Closed

This was linked to issues Mar 25, 2023

M #648

Closed

G #654

Closed

H #655

Closed

Mancent linked an issue May 21, 2023 that may be closed by this pull request

> - [ ] ``` #678

Closed

Skptak added 3 commits September 20, 2023 15:57

Merge branch 'main' into main

90ef953

Fix a spelling mistake in CMakeLists.txt

f22169d

Wrap the heap based parts of portable.h in if dynamic allocation is s…

8c9b032

…upported. Update the CMakeLists.txt to account for this

Skptak force-pushed the main branch from 9b62418 to 92278d1 Compare September 21, 2023 17:25

Work on making it so this cmake file can be included from anywhere, n…

2aff1a1

…ot just if it's used from this directoy

Skptak force-pushed the main branch from 8b6e89e to 2aff1a1 Compare September 21, 2023 17:41

phelter suggested changes Sep 21, 2023

View reviewed changes

jputcu mentioned this pull request Sep 24, 2023

Remove default behaviour of FREERTOS_HEAP. #807

Merged

2 tasks

chinglee-iot closed this Dec 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to not include heap in CMakeLists.txt #595

Add option to not include heap in CMakeLists.txt #595

jasonpcarroll commented Nov 30, 2022

codecov bot commented Nov 30, 2022 •

edited

Loading

paulbartell Dec 1, 2022 •

edited

Loading

phelter Feb 22, 2023

amazonKamath Mar 3, 2023

CrustyAuklet Jun 15, 2023

phelter Sep 21, 2023 •

edited

Loading

n9wxu Sep 22, 2023

conara Sep 24, 2023 •

edited

Loading

phelter Feb 22, 2023

amazonKamath commented Mar 3, 2023

Skptak commented Sep 21, 2023

Skptak commented Sep 21, 2023

phelter Sep 21, 2023

Skptak Sep 22, 2023

phelter Sep 23, 2023 •

edited

Loading

phelter Sep 21, 2023

phelter Sep 21, 2023 •

edited

Loading

chinglee-iot commented Dec 6, 2023

		include(${CMAKE_CURRENT_LIST_DIR}/include/CMakeLists.txt)
		include(${CMAKE_CURRENT_LIST_DIR}/portable/CMakeLists.txt)

Add option to not include heap in CMakeLists.txt #595

Add option to not include heap in CMakeLists.txt #595

Conversation

jasonpcarroll commented Nov 30, 2022

Description

Test Steps

Related Issue

codecov bot commented Nov 30, 2022 • edited Loading

Codecov Report

paulbartell Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phelter Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conara Sep 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amazonKamath commented Mar 3, 2023

Skptak commented Sep 21, 2023

Skptak commented Sep 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phelter Sep 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phelter Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

chinglee-iot commented Dec 6, 2023

codecov bot commented Nov 30, 2022 •

edited

Loading

paulbartell Dec 1, 2022 •

edited

Loading

phelter Sep 21, 2023 •

edited

Loading

conara Sep 24, 2023 •

edited

Loading

phelter Sep 23, 2023 •

edited

Loading

phelter Sep 21, 2023 •

edited

Loading