Skip to content
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.

Sign up for GitHub

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

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

Closed
wants to merge 5 commits into from
Closed

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

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cdb854f
Add option to not include heap in CMakeLists.txt
jasonpcarroll Nov 30, 2022
90ef953
Merge branch 'main' into main
Skptak Sep 20, 2023
f22169d
Fix a spelling mistake in CMakeLists.txt
Skptak Sep 20, 2023
8c9b032
Wrap the heap based parts of portable.h in if dynamic allocation is s…
Skptak Sep 21, 2023
2aff1a1
Work on making it so this cmake file can be included from anywhere, n…
Skptak Sep 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,16 +224,25 @@ endif()

add_subdirectory(portable)

add_library(freertos_kernel STATIC
list(APPEND FREERTOS_KERNEL_SOURCES
Skptak marked this conversation as resolved.
Show resolved Hide resolved
event_groups.c
list.c
queue.c
stream_buffer.c
tasks.c
timers.c
)

# 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)
Copy link
Contributor

@paulbartell paulbartell Dec 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paulbartell @jasonpcarroll thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@phelter phelter Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@conara conara Sep 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

list(APPEND FREERTOS_KERNEL_SOURCES
# 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>
)
endif()

add_library(freertos_kernel STATIC
${FREERTOS_KERNEL_SOURCES}
)

target_include_directories(freertos_kernel
Expand Down