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.

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

heap-management: Use C memory management APIs #57

Merged
merged 2 commits into from
Mar 11, 2024

Conversation

AhmedIsmail02
Copy link
Contributor

Description

We shouldn't be allocating heap using C and FreeRTOS heap management APIs as we might end up with both FreeRTOS heap and C heap. This can lead to several unexpected issues.

C standard heap functions are used to unify heap allocations as we can't use FreeRTOS heap management APIs because this would require patching lots of file as part of
ml-embedded-evaluation-kit library which use C standard heap functions for heap allocations.

Heap space is increased by 0x10000 as C standard heap management APIs requires more heap space for the existing FRI applications because it is less optimised than FreeRTOS heap management APIS (heap_4).

Test Steps

Checklist:

  • I have tested my changes. No regression in existing tests.
  • I have modified and/or added unit-tests to cover the code changes in this Pull Request.

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.

@AhmedIsmail02 AhmedIsmail02 requested a review from a team as a code owner March 1, 2024 12:33
urutva
urutva previously approved these changes Mar 1, 2024
@archigup
Copy link
Member

archigup commented Mar 1, 2024

Couldn't this just set the FreeRTOS heap to heap 3 which wraps malloc and free?
See: https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/portable/MemMang/heap_3.c

@AhmedIsmail02
Copy link
Contributor Author

AhmedIsmail02 commented Mar 1, 2024

Couldn't this just set the FreeRTOS heap to heap 3 which wraps malloc and free? See: https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/portable/MemMang/heap_3.c

@archigup Indeed, I've tried to use heap_3 sample implementation. However, it did not work in our case as we would still need the symbols for xPortGetFreeHeapSize() and xPortGetMinimumEverFreeHeapSize() functions as they are used as part of FreeRTOS plus TCP library even if heap3 implementation is used where in case of heap3 implementation, these functions are not defined. The part of the code that uses xPortGetFreeHeapSize() and xPortGetMinimumEverFreeHeapSize() is not used in our project but Arm Compiler for Embedded toolchain requires symbols to be defined even if the function containing the symbols is to removed by the linker. Please refer to this comment where i explained why we've added these dummy implementations.

@urutva
Copy link
Contributor

urutva commented Mar 2, 2024

@archigup In addition to issue explained by @AhmedIsmail02, the main driver for the decision to switch to C memory management instead of FreeRTOS memory management is that, the middleware used by the FRI like ml-embedded-evaluation-kit and tflite-micro uses C memory management APIs. In order to use heap_3, the middlewares need to an override of C memory management APIs to call pvPortMalloc and vPortFree, which in turn calls the overridden C memory management APIs. Also, having two heap implementation (C and FreeRTOS memory management) is not recommended as it can lead to unexpected issues. Therefore, we decided to only use C memory management APIs.

archigup
archigup previously approved these changes Mar 5, 2024
@AhmedIsmail02 AhmedIsmail02 force-pushed the unify-heap-allocations branch from b5f3193 to 9e76bd3 Compare March 6, 2024 10:31
@hugueskamba
Copy link
Contributor

@archigup @chinglee-iot @aggarg
I hope you are all well.
Are you able to merge this?

It was noticed that sometimes speech-recognition
application fails on the CI because the OTA update
is not completed by the time `timeout-seconds` is reached.
Hence, increasing the timeout for test applications job to 45
minutes instead of 30 minutes.

Signed-off-by: Ahmed Ismail <[email protected]>
We shouldn't be allocating heap using C and FreeRTOS
heap management APIs as we might end up with both FreeRTOS heap
and C heap. This can lead to several unexpected issues.

C standard heap functions are used to unify heap allocations
as we can't use FreeRTOS heap management APIs because this
would require patching lots of file as part of
ml-embedded-evaluation-kit library which use C standard heap
functions for heap allocations.

Heap space is increased by `0x10000` as C standard heap
management APIs requires more heap space for the existing
FRI applications because it is less optimised than FreeRTOS heap
management APIS (heap_4).

Signed-off-by: Ahmed Ismail <[email protected]>
@AhmedIsmail02 AhmedIsmail02 dismissed stale reviews from archigup and urutva via 169d721 March 7, 2024 15:12
@AhmedIsmail02 AhmedIsmail02 force-pushed the unify-heap-allocations branch from 9e76bd3 to 169d721 Compare March 7, 2024 15:12
@amazonKamath amazonKamath self-requested a review March 11, 2024 10:25
@urutva urutva merged commit 9bc2295 into FreeRTOS:main Mar 11, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants