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

Show file tree
Hide file tree
Changes from all commits
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
28 changes: 16 additions & 12 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,22 +273,26 @@ target_compile_options(freertos_kernel PRIVATE


########################################################################
add_subdirectory(include)
add_subdirectory(portable)
include(${CMAKE_CURRENT_LIST_DIR}/include/CMakeLists.txt)
include(${CMAKE_CURRENT_LIST_DIR}/portable/CMakeLists.txt)
Comment on lines +276 to +277
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@phelter phelter Sep 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.


target_sources(freertos_kernel PRIVATE
croutine.c
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>
${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
Comment on lines +280 to +286
Copy link
Contributor

Choose a reason for hiding this comment

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

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

)

# Check if user requested to not include 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.

target_sources(freertos_kernel PRIVATE
# 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()

target_link_libraries(freertos_kernel
PUBLIC
Expand Down
2 changes: 1 addition & 1 deletion include/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ add_library(freertos_kernel_include INTERFACE)

target_include_directories(freertos_kernel_include
INTERFACE
.
${CMAKE_CURRENT_LIST_DIR}/.
# Note: DEPRECATED but still supported, may be removed in a future release.
$<$<NOT:$<TARGET_EXISTS:freertos_config>>:${FREERTOS_CONFIG_FILE_DIRECTORY}>
)
Expand Down
75 changes: 40 additions & 35 deletions include/portable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,25 +132,28 @@
#endif
#endif /* if ( portUSING_MPU_WRAPPERS == 1 ) */

/* Only include heap related functions and structs if using dynamic allocation */
#if ( configSUPPORT_DYNAMIC_ALLOCATION == 1 )

/* Used by heap_5.c to define the start address and size of each memory region
* that together comprise the total FreeRTOS heap space. */
typedef struct HeapRegion
{
uint8_t * pucStartAddress;
size_t xSizeInBytes;
} HeapRegion_t;
typedef struct HeapRegion
{
uint8_t * pucStartAddress;
size_t xSizeInBytes;
} HeapRegion_t;

/* Used to pass information about the heap out of vPortGetHeapStats(). */
typedef struct xHeapStats
{
size_t xAvailableHeapSpaceInBytes; /* The total heap size currently available - this is the sum of all the free blocks, not the largest block that can be allocated. */
size_t xSizeOfLargestFreeBlockInBytes; /* The maximum size, in bytes, of all the free blocks within the heap at the time vPortGetHeapStats() is called. */
size_t xSizeOfSmallestFreeBlockInBytes; /* The minimum size, in bytes, of all the free blocks within the heap at the time vPortGetHeapStats() is called. */
size_t xNumberOfFreeBlocks; /* The number of free memory blocks within the heap at the time vPortGetHeapStats() is called. */
size_t xMinimumEverFreeBytesRemaining; /* The minimum amount of total free memory (sum of all free blocks) there has been in the heap since the system booted. */
size_t xNumberOfSuccessfulAllocations; /* The number of calls to pvPortMalloc() that have returned a valid memory block. */
size_t xNumberOfSuccessfulFrees; /* The number of calls to vPortFree() that has successfully freed a block of memory. */
} HeapStats_t;
typedef struct xHeapStats
{
size_t xAvailableHeapSpaceInBytes; /* The total heap size currently available - this is the sum of all the free blocks, not the largest block that can be allocated. */
size_t xSizeOfLargestFreeBlockInBytes; /* The maximum size, in bytes, of all the free blocks within the heap at the time vPortGetHeapStats() is called. */
size_t xSizeOfSmallestFreeBlockInBytes; /* The minimum size, in bytes, of all the free blocks within the heap at the time vPortGetHeapStats() is called. */
size_t xNumberOfFreeBlocks; /* The number of free memory blocks within the heap at the time vPortGetHeapStats() is called. */
size_t xMinimumEverFreeBytesRemaining; /* The minimum amount of total free memory (sum of all free blocks) there has been in the heap since the system booted. */
size_t xNumberOfSuccessfulAllocations; /* The number of calls to pvPortMalloc() that have returned a valid memory block. */
size_t xNumberOfSuccessfulFrees; /* The number of calls to vPortFree() that has successfully freed a block of memory. */
} HeapStats_t;

/*
* Used to define multiple heap regions for use by heap_5.c. This function
Expand All @@ -163,34 +166,34 @@ typedef struct xHeapStats
* terminated by a HeapRegions_t structure that has a size of 0. The region
* with the lowest start address must appear first in the array.
*/
void vPortDefineHeapRegions( const HeapRegion_t * const pxHeapRegions ) PRIVILEGED_FUNCTION;
void vPortDefineHeapRegions( const HeapRegion_t * const pxHeapRegions ) PRIVILEGED_FUNCTION;

/*
* Returns a HeapStats_t structure filled with information about the current
* heap state.
*/
void vPortGetHeapStats( HeapStats_t * pxHeapStats );
void vPortGetHeapStats( HeapStats_t * pxHeapStats );

/*
* Map to the memory management routines required for the port.
*/
void * pvPortMalloc( size_t xSize ) PRIVILEGED_FUNCTION;
void * pvPortCalloc( size_t xNum,
size_t xSize ) PRIVILEGED_FUNCTION;
void vPortFree( void * pv ) PRIVILEGED_FUNCTION;
void vPortInitialiseBlocks( void ) PRIVILEGED_FUNCTION;
size_t xPortGetFreeHeapSize( void ) PRIVILEGED_FUNCTION;
size_t xPortGetMinimumEverFreeHeapSize( void ) PRIVILEGED_FUNCTION;

#if ( configSTACK_ALLOCATION_FROM_SEPARATE_HEAP == 1 )
void * pvPortMallocStack( size_t xSize ) PRIVILEGED_FUNCTION;
void vPortFreeStack( void * pv ) PRIVILEGED_FUNCTION;
#else
#define pvPortMallocStack pvPortMalloc
#define vPortFreeStack vPortFree
#endif
void * pvPortMalloc( size_t xSize ) PRIVILEGED_FUNCTION;
void * pvPortCalloc( size_t xNum,
size_t xSize ) PRIVILEGED_FUNCTION;
void vPortFree( void * pv ) PRIVILEGED_FUNCTION;
void vPortInitialiseBlocks( void ) PRIVILEGED_FUNCTION;
size_t xPortGetFreeHeapSize( void ) PRIVILEGED_FUNCTION;
size_t xPortGetMinimumEverFreeHeapSize( void ) PRIVILEGED_FUNCTION;

#if ( configUSE_MALLOC_FAILED_HOOK == 1 )
#if ( configSTACK_ALLOCATION_FROM_SEPARATE_HEAP == 1 )
void * pvPortMallocStack( size_t xSize ) PRIVILEGED_FUNCTION;
void vPortFreeStack( void * pv ) PRIVILEGED_FUNCTION;
#else
#define pvPortMallocStack pvPortMalloc
#define vPortFreeStack vPortFree
#endif

#if ( configUSE_MALLOC_FAILED_HOOK == 1 )

/**
* task.h
Expand All @@ -200,8 +203,10 @@ size_t xPortGetMinimumEverFreeHeapSize( void ) PRIVILEGED_FUNCTION;
*
* This hook function is called when allocation failed.
*/
void vApplicationMallocFailedHook( void ); /*lint !e526 Symbol not defined as it is an application callback. */
#endif
void vApplicationMallocFailedHook( void ); /*lint !e526 Symbol not defined as it is an application callback. */
#endif /* ( configUSE_MALLOC_FAILED_HOOK == 1 ) */

#endif /* if ( configSUPPORT_DYNAMIC_ALLOCATION == 1 ) */

/*
* Setup the hardware ready for the scheduler to take control. This generally
Expand Down
Loading