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

Ensure xPortGetFreeHeapSize reports DRAM #8680

Merged
merged 7 commits into from
Oct 11, 2022

Conversation

mhightower83
Copy link
Contributor

@mhightower83 mhightower83 commented Sep 27, 2022

Create dedicated function for xPortGetFreeHeapSize() that only reports on DRAM.
NONOS SDK API system_get_free_heap_size() relies on xPortGetFreeHeapSize() for the free Heap size.

Possible breaking change for multiple Heap Sketches calling system_get_free_heap_size(); it will now always report free DRAM Heap size.

Update and export umm_free_heap_size_lw() to report the free Heap size of the current Heap.
Updated ESP.getFreeHeap() to use umm_free_heap_size_lw().

Updated build options to supply exported umm_free_heap_size_lw() via either UMM_STATS or UMM_INFO.

Improved build option support via the SketchName.ino.globals.h method for Heap options: UMM_INFO, UMM_INLINE_METRICS, UMM_STATS, UMM_STATS_FULL, UMM_BEST_FIT, and UMM_FIRST_FIT. While uncommon to change from the defaults, you can review umm_malloc_cfgport.h for more details, which may help reduce your Sketch's size in dire situations. Assuming you are willing to give up some functionality.
For debugging UMM_STATS_FULL can offer additional stats, like Heap low water mark (umm_free_heap_size_min()).

See umm_malloc/Notes.h entry Sep 26, 2022 for overview.

uncrustified umm_poison.c

Edited: revised to track PR changes

Create dedicated function for `xPortGetFreeHeapSize()` that only reports on DRAM.

Update and export `umm_free_heap_size()` to report the free Heap space of the current Heap. Updated ESP.getFreeHeap() to use.

Replace internal function `umm_free_heap_size_lw()` with `umm_free_heap_size()`.

See umm_malloc/Notes.h entry Sep 26, 2022 for more specifics.

uncrustified `umm_poison.c`
@mcspr
Copy link
Collaborator

mcspr commented Sep 28, 2022

Why not 'alias' for both variants?
Wouldn't there be less preprocessor if-def in both umm_info.c and umm_local.c if we have different functionality of stat retrieval?

Specifically, function name change in _info.c where we have it either be umm_free_heap_size or umm_free_heap_size_info. And the _local.c at the same time may or may not define it.

@mhightower83
Copy link
Contributor Author

@mcspr Okay, I have made some changes I'll push them tomorrow after I review them again. I did some build option variations and found some issues. Another function needs to be broken out. Also, improved support for comment block build options.

While touching and regression testing of build macros for umm_info...
and umm_stats..., improved/fixed macros for better support of
build options specified through Sketch.ino.globals.h.
Copy link
Collaborator

@mcspr mcspr left a comment

Choose a reason for hiding this comment

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

Ran a couple of tests switching build opts, everything works as described.

Only (minor?) issue is we still could make getHeapStats do nothing when umm info / stats become empty macro calls. This was already happening before the PR, though.

cores/esp8266/umm_malloc/umm_malloc_cfgport.h Show resolved Hide resolved
cores/esp8266/umm_malloc/umm_malloc_cfgport.h Outdated Show resolved Hide resolved
@mhightower83
Copy link
Contributor Author

Only (minor?) issue is we still could make getHeapStats do nothing when umm info / stats become empty macro calls. This was already happening before the PR, though.

@mcspr what do you think it should do? The only info I could return would be free heap size; however,
I thought that it would be confusing to have a crippled function better to completely not work. If they were looking for free heap size they could use the other method. I suppose a build conditional with deprecated attributes might be a good way to go.

@mcspr
Copy link
Collaborator

mcspr commented Oct 3, 2022

Only (minor?) issue is we still could make getHeapStats do nothing when umm info / stats become empty macro calls. This was already happening before the PR, though.

@mcspr what do you think it should do? The only info I could return would be free heap size; however, I thought that it would be confusing to have a crippled function better to completely not work. If they were looking for free heap size they could use the other method. I suppose a build conditional with deprecated attributes might be a good way to go.

Fail on link, since we have a clear dependency? Empty macro inside UMM still makes sense to skip some actions, but external one probably doesn't.

If we don't use ESP.getHeapStats, we don't care whether anything related to stats is implemented.
If we do use it but mess up umm flags, linker won't find anything and show undefined reference.

Improve empty function situation around build option UMM_INFO.
cores/esp8266/Esp.h Outdated Show resolved Hide resolved
@mcspr
Copy link
Collaborator

mcspr commented Oct 11, 2022

Something about that umm double include does not feel right... cfgport may come either first, or last. Kind of a weird dependency.
But, lgtm otherwise, if this is done with updates

@mhightower83
Copy link
Contributor Author

When I make changes I migrate closer to upstream usage where umm_malloc_cfg.h is the upstream library version and all local port defines/changes go in umm_malloc_cfgport.h. Before, port changes were mixed in with umm_malloc_cfg.h.
It is weird to put umm_malloc_cfgport.h first in Arduino.h; however, it seems to have all the options defined (and normalized) for the port in one place which is what was needed for the build. Having Esp.h reference umm_malloc build options made things complicated.

I have more changes for aligning with umm_malloc upstream plus a few enhancements, I now need to work through those and see what I have broken (if any) with this shuffle.

I think I am done with this one. It was a lot more change than I wanted for a quick fix.

@mcspr mcspr merged commit d3eddeb into esp8266:master Oct 11, 2022
@mhightower83 mhightower83 deleted the pr-xPortGetFreeHeapSize branch October 11, 2022 16:51
@mcspr mcspr added this to the 3.1 milestone Nov 2, 2022
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.

None yet

2 participants