Use all free RAM for FreeRTOS heap

[pipe01]

With this change you no longer need to fiddle with FreeRTOS's configTOTAL_HEAP_SIZE to make it fit into the RAM and instead it will always automatically use all available RAM for the heap.

This is achieved by using two symbols defined by the linker script: __HeapLimit and __StackLimit, which indicate where the used heap ends and where the stack "begins" (really ends) in memory respectively. It's not yet ready for production since I'm still not 100% sure there aren't any side effects (the emulator works fine with this build but if anyone with a devkit can run this for a while that'd be great), and also it currently only uses about 39.5 KB for the heap for reasons I'm still not quite sure about.

Here's a rough representation of the RAM:

|---------------|
|  0x20010000   |
|   <stack>     |
|   <stack>     |
|   <stack>     |
| __StackLimit  |
| <free space>  |
| <free space>  |
| <free space>  |
| <free space>  |
| <free space>  |
| __HeapLimit   |
|   <heap>      |
|   <heap>      |
| __HeapBase    |
| <global vars> |
| <global vars> |
| <global vars> |
|  0x20000000   |
|---------------|

Note that in this case <heap> doesn't refer to FreeRTOS's heap but rather GCC's heap which is 0 bytes long in our case, so __HeapLimit == __HeapBase.

Before this change, FreeRTOS's heap was statically allocated in the <global vars> region and the goal was to manually increment the heap size in order to make the <free space> region as small as possible, since that region is essentially wasted RAM space. Now we instead put the FreeRTOS heap right where the <heap> region ends and make it stretch right until the top of the stack (__StackLimit), which ensures we waste no RAM.

---

I did have to overwrite the _sbrk method because apparently there's a printf definition somewhere that uses a different implementation of malloc that doesn't use the FreeRTOS heap and instead allocates around 1 KB of RAM for itself, this is probably worth looking into just by itself.

[github-actions]

Build size and comparison to main:

Section	Size	Difference
text	372816B	-1792B
data	948B	0B
bss	22536B	-40968B

[mark9064]

This has been a wishlist item for me for ages! Awesome to see it realised

Could you explain the _sbrk/printf stuff a bit more? How did you find this out?

[pipe01]

I found out about it when testing using InfiniEmu, it crashed because something was overwriting the FreeRTOS heap and it turned out to be that printf was mallocing without using the FreeRTOS heap, I'm not sure how or why it's using its own printf and malloc implementations though.

[pipe01]

Now LFS is using the NRF_LOG functions instead of printf so there's no longer an issue with any mallocs, however the solution is kind of ugly since we have to copy the entire lfs_util.h over and the CRC implementation, but it seems like there's no better way to customize littlefs other than forking it.

[mark9064]

Why not define LFS_TRACE/DEBUG/WARN/ERROR? That way printf can never get included

[pipe01]

That's what I'm doing here, the issue is that in order to define those macros before littlefs uses them we need to define LFS_CONFIG in cmake which will make littlefs load our file instead of its own lfs_util.h as seen here, but that means that we need to copy over the entire file to our project and also the CRC function for some reason.

[mark9064]

Ah yeah I see the issue now, defining the macro at the right time is annoying.
I guess the one alternative is declaring them in FS.h (or before) before LFS is included for the first time? Not super pretty either though

[pipe01]

Yeah that's what my first idea was but unfortunately it doesn't work because it wouldn't be included when compiling the lfs.c.

[mark9064]

Ah, you're right :)
Probably would've been too hacky anyway

[mark9064]

Any chance you could take a look at the build failure BTW? Also would I be right in saying that the sbrk implementation can be removed now with littlefs' printf removed?

[pipe01]

I would still leave it in because otherwise if were to ever get called it would corrupt the FreeRTOS heap. Maybe we could replace the body with a hard fault so that at least it crashes right away, to be honest I'm not sure if the implementation using pvPortMalloc behaves as expected.

[mark9064]

Yeah I'm liking the sound of a hard fault there, nothing should ever be calling it. And same, I'm not sure malloc'ing there is correct, looking at the manpage shows some things that don't line up with it just being malloc (particularly the behaviour when decreasing the program break or checking it). I don't know if we could add some linker error to prevent the sbrk even being linked in? - it might be theoretically required by something anyway.

[pipe01]

Sounds good. It does seem like sbrk is being used my something else, more research is needed.

[pipe01]

It seems like there's no way to make printf and its variants use our malloc since it forcefully calls _malloc_r directly with no way to change it since we don't compile that code and instead it comes from the newlib binaries. I've written a _sbrk implementation that I'm pretty sure works how it's supposed to but I think it might be best to make it fault as discussed. I'm leaving the code here for posterity, just in case.

_sbrk implementation

static int brk_size = 0;
static void *brk = NULL;

void* _sbrk(int size) {
  if (size == 0) {
    return brk;
  }
  brk_size += size;
  brk = pvPortRealloc(brk, brk_size);
  if (brk == NULL) {
    return (void*)-1;
  }
  return brk;
}

The printf implementation only calls malloc on certain cases, so hopefully it should be fine but more testing is needed.

[mark9064]

Very interesting reading on the topic https://nadler.com/embedded/newlibAndFreeRTOS.html
Looks like there are lots of approaches including wrapping malloc, linker tricks to not compile etc.
I need to find some time to understand them all, but maybe you'll find these useful?

[pipe01]

That's very useful indeed, thanks! I'll try the ideas from the article and report back.

[pipe01]

Seems to be working well now, I'm getting around 41KB of heap.

[mark9064]

Looks great, lines up nicely with current heap size of 40960!

[mark9064]

Hmm I see the lfs config breaking the formatting check. I haven't thought too much about where it belongs, but would it make sense to move it to the libs directory along with lv_conf.h (everything in libs is not format checked)? There might well be good reasons not to so I'm not sure

Edit: Also is there some way to avoid duplicating the original lfs_config.c? Can we add an include somewhere?

[pipe01]

I've made the CRC file include the original implementation as you suggested, thank you for the suggestion!

I feel like moving it to the libs folder probably makes the most sense as far as I understand it, especially given that there's already a similar file there. I've also realized that we don't really need to modify the LFS config anymore since we now have a functioning malloc replacement, however I've noticed that replacing the log calls reduces the RAM usage by about 1.4kB, so it might be worth keeping it as-is.

EDIT: Nevermind about the first point, I found a better (if hackier) way to include the original CRC implementation here.

[mark9064]

ohhhh nice yeah that undefine trick is good. Probably needs some comments to explain it. I'm all good to move the lfs_config.h to libs then

[mark9064]

Booted fine on my watch. Will daily drive for the forseeable future and report back any issues

[mark9064]

LGTM

No further comments for now, I'll approve after some wrist time (all looking good so far)

Edit: Might want to draft up an InfiniSim PR at some point, no rush though

[mark9064]

Been working flawlessly. Is this ready in your eyes?

[pipe01]

Been working fine on my end too, I think it's good to go!

[FintasticMan]

Looks good to me too!

[pipe01]

@FintasticMan made me realize that there's a better way to do the lfs_config.h stuff, I think it should work the same way.

[FintasticMan]

This looks good now!