esp-hal
esp-hal copied to clipboard
feat(esp-alloc): Add heap usage stats
Thank you for your contribution!
We appreciate the time and effort you've put into this pull request. To help us review it efficiently, please ensure you've gone through the following checklist:
Submission Checklist 📝
- [x] I have updated existing examples or added new ones (if applicable).
- [x] I have used
cargo xtask fmt-packagescommand to ensure that all changed code is formatted correctly. - [x] My changes were added to the
CHANGELOG.mdin the proper section. - [x] I have added necessary changes to user code to the Migration Guide.
- [x] My changes are in accordance to the esp-rs API guidelines
Extra:
- [x] I have read the CONTRIBUTING.md guide and followed its instructions.
Pull Request Details 📖
Description
This PR provides a way to easily see the current heap usage of esp_alloc. This adds HeapRegion::stats() and EspHeap::stats(), to return RegionStats and HeapStats respectively.
Both also implement core::fmt::Display to easily pretty-print the heap usage as following:
println!("{}", esp_alloc::get_info!());
HEAP INFO
Size: 117760
Current usage: 47280
Max usage: 70060
Total freed: 103012
Total allocated: 150292
Memory Layout:
Internal | ██████████████░░░░░░░░░░░░░░░░░░░░░ | Used: 47280 / Total: 117760 (Free: 70480)
Unused | ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ |
Unused | ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ |
Testing
I have tested with a few examples locally and with esp-mbedtls, and everything works fine.
I haven't tested with PSRAM as I don't have any.
Fancy, but it should also support defmt, not everybody uses esp-println and making the user add Display2Format makes this somewhat less elegant.
Looks nice.
Given this runs a small amount of code every time allloc/dealloc is called would it make sense to have that feature gated?
@bugadani won't this work with defmt? defmt::!println!("{}", esp_alloc::get_info!());
I was thinking about feature gating the whole get_info!() + HeapStats feature, but I realized that the extra computation in alloc/dealloc is only to keep track of internal heap stats. I added a new feature named internal-heap-stats (for the lack of better name), to cover this.
The trade-off is that we have a feature that will change the amount of fields returned by the struct, if a user calls stats(), but I think we do that in a few places already.
I also found a chip that has PSRAM, so I tested with using an internal heap and a PSRAM heap, and the output looks like the following:
HEAP INFO
Size: 2148352
Current usage: 512028
Max usage: 512028
Total freed: 0
Total allocated: 512028
Memory Layout:
Internal | ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ | Used: 28 / Total: 51200 (Free: 51172)
External | ████████░░░░░░░░░░░░░░░░░░░░░░░░░░░ | Used: 512000 / Total: 2097152 (Free: 1585152)
Unused | ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ |
Fancy, but it should also support defmt, not everybody uses esp-println and making the user add Display2Format makes this somewhat less elegant.
I'm not used to defmt, can you tell what changes are required to make it work with defmt? The following was suggested by @okhsunrog in https://github.com/esp-rs/esp-hal/pull/2137#issuecomment-2344506476:
defmt::!println!("{}", esp_alloc::get_info!());
The examples aren't set up to use defmt so I'll have to create a project that uses defmt.
With the obvious syntax error, I believe the compiler would just complain about "HeapStats does not implement Format".
This would also need to be feature gated, similar to how the rest of the crates have a demft feature. We can also do it later if you don't feel comfortable experimenting just for a different printing framework.
@bugadani yeah, that was a typo, sorry. For some reason I also thought that core Debug can also be used for defmt's println. Seems like it's not the case.. After stydying the docs a bit I think now I get it right
@MabezDev maybe we can merge it as it is and I'll implement defmt correctly in a follow up PR?
@okhsunrog Could you prepare the PR based on Anthony's branch? Once both are ready I'll merge them.
@MabezDev Sure, I'm create the PR tomorrow based on this branch
I'm converting this to a draft for now, no rush with this of course. Please let us know if you'd like any help wrapping this up.
I got a trauma in the end of September and had a surgery on my elbow. I still intend to work on this as soon as I recover
Sorry to hear, get well soon :)
Any update here? It would be nice to get this merged :).
@MabezDev I'll create a new PR this weekend, Saturday or Sunday and finish the defmt part
Sorry to keep pinging here, but I'd love to see this finished up :)
Sorry, was busy at work. I've started working on this, I'll upload the PR tomorrow. I started with the fork of the current main branch, cherry-picked some commits, resolved a minor merge conflict (it was a comment for example :D), and now I'm adding the Format implementation on top of it. I've moved the formatting logic to a separate method and I'm using it in both Display and Format implementations. @MabezDev is it okay if I add a heapless dependency for heapless String?
@MabezDev is it okay if I add a heapless dependency for heapless String?
I don't think we should be building strings for formatting. I think we can resolve this with a small amount of duplicate code which does the formatting in the same was a fmt::Debug impl, but with the defmt macros instead.
Superceded by https://github.com/esp-rs/esp-hal/pull/2619, thanks @AnthonyGrondin and thanks @okhsunrog!