Omit configTOTAL_HEAP_SIZE for Application Allocated Heap

[Finwood]

This PR implements a solution for the issue described in #347.

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

[mingyue86010]

Hi @Finwood,

Thank you for pointed out this. I think it is a reasonable using senario and should be considered. But regarding to the change you made, maybe using a pointer would be better than declaring a non-size array?

extern uint8_t *ucHeap;

Regards,
Ming

[mingyue86010]

Is this better? So no need to worry about if the compiler suppot or not.

Suggested change

	extern uint8_t ucHeap[ ];
	extern uint8_t *ucHeap;

[Finwood]

That would work for the implementation of heap_4.c, since ucHeap is only ever accessed like a pointer would:

FreeRTOS-Kernel/portable/MemMang/heap_4.c

Lines 351 to 361 in 6a84f2c

	/* Ensure the heap starts on a correctly aligned boundary. */
	uxAddress = ( size_t ) ucHeap;
	if( ( uxAddress & portBYTE_ALIGNMENT_MASK ) != 0 )
	{
	uxAddress += ( portBYTE_ALIGNMENT - 1 );
	uxAddress &= ~( ( size_t ) portBYTE_ALIGNMENT_MASK );
	xTotalHeapSize -= uxAddress - ( size_t ) ucHeap;
	}
	pucAlignedHeap = ( uint8_t * ) uxAddress;

In heap_1.c this would break, since the existing implementation accesses ucHeap[ portBYTE_ALIGNMENT ], which would be invalid if ucHeap would be a pointer:

FreeRTOS-Kernel/portable/MemMang/heap_1.c

Lines 96 to 100 in 6a84f2c

	if( pucAlignedHeap == NULL )
	{
	/* Ensure the heap starts on a correctly aligned boundary. */
	pucAlignedHeap = ( uint8_t * ) ( ( ( portPOINTER_SIZE_TYPE ) & ucHeap[ portBYTE_ALIGNMENT ] ) & ( ~( ( portPOINTER_SIZE_TYPE ) portBYTE_ALIGNMENT_MASK ) ) );
	}

Since pointers and arrays aren't equivalent I would advise against having the type of ucHeap depend on configAPPLICATION_ALLOCATED_HEAP.

[mingyue86010]

Hi @Finwood,

The pointer indexing shouldn't be a problem here. But we are considering how to preperly handle configTOTAL_HEAP_SIZE things. The heap_4 uses configTOTAL_HEAP_SIZE in the init function. The current code force user to set configTOTAL_HEAP_SIZE. However with the change in this PR, it may cause some problem if user forget setting configTOTAL_HEAP_SIZE when allocate the heap memory in application. As for now please just made the change from you code to resolve your issue and we will keep you updated on this PR.

Thank you!

Regards,
Ming

[Finwood]

Changed in f31a1ac 👍

[codecov]

Codecov Report

Merging #351 (a7eb292) into main (1d86b97) will not change coverage.
The diff coverage is n/a.

❗ Current head a7eb292 differs from pull request most recent head 763f1b8. Consider uploading reports for the commit 763f1b8 to get more accurate results

@@           Coverage Diff           @@
##             main     #351   +/-   ##
=======================================
  Coverage   92.13%   92.13%           
=======================================
  Files           4        4           
  Lines        1272     1272           
  Branches      342      342           
=======================================
  Hits         1172     1172           
  Misses         53       53           
  Partials       47       47           

Flag	Coverage Δ
unittests	92.13% <ø> (ø)

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

Impacted Files	Coverage Δ
event_groups.c	79.34% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d86b97...763f1b8. Read the comment docs.

[Finwood]

I've run tests locally - for heap_2.c, this line crashes with not-constant configTOTAL_HEAP_SIZE:

FreeRTOS-Kernel/portable/MemMang/heap_2.c

Line 90 in f31a1ac

static size_t xFreeBytesRemaining = configADJUSTED_HEAP_SIZE;

Moving the assignment into prvHeapInit() could solve that:

static size_t xFreeBytesRemaining;  // or possibly initialize to zero

// [...]

static void prvHeapInit( void )
{
    xFreeBytesRemaining = configADJUSTED_HEAP_SIZE; 
    
    // [...]
}

[mingyue86010]

Hi @Finwood,

Reviewing back this PR. The above change can work but as mentioned in the last comment user who forget setting configTOTAL_HEAP_SIZE may not receive the ERROR anymore. You can do the change for heap_4 at your local copy. Or there is another way to resolve your issue by using heap_5 and you don't have to change the FreeRTOS code.

Please referce https://www.freertos.org/a00111.html#heap_5 about using Heap_5. And specific for your case, you may want:

`
HeapRegion_t xHeapRegions[] =
{
{ ( uint8_t * ) &__FreeRTOSHeapTop, ( size_t ) ( ( uint32_t )&__FreeRTOSHeapTop - ( uint32_t )&__FreeRTOSHeapBase ) },
{ NULL, 0 }
};

vPortDefineHeapRegions( xHeapRegions );

`

Close this PR and issue #347. Please feel free to reopen the issue if you have any further question.

Thank you!

Regards,
Ming Yue