Skip to content

Adds a memory_stats metrics component - #152

Closed
Davidbick wants to merge 2 commits into
mainfrom
feature/memory_metrics
Closed

Adds a memory_stats metrics component#152
Davidbick wants to merge 2 commits into
mainfrom
feature/memory_metrics

Conversation

@Davidbick

Copy link
Copy Markdown
Contributor

What changed?

  • Adds a memory_stats metrics component to the generic metrics registry, reporting FreeRTOS heap health (heap_free, heap_min_free, heap_largest_block, in bytes).

How does it make Bristlemouth better?

  • Adds fleet-wide heap visibility per node and proves the generic registry takes a second producer cleanly.
  • heap_min_free is a low-water leak signal, heap_largest_block catches fragmentation.

Where should reviewers focus?

  • Field choice: three of the seven HeapStats_t fields, free + low-water + largest-block.

Checklist

  • Add or update unit tests for changed code
  • Ensure all submodules up to date. If this PR relies on changes in submodules, merge those PRs first, then point this PR at/after the merge commit
  • Ensure code is formatted correctly with clang-format. If there are large formatting changes, they should happen in a separate whitespace-only commit on this PR after all approvals.

@Davidbick Davidbick self-assigned this Aug 6, 2026
Comment thread common/bm_freertos.c
stats->free_bytes = (uint32_t)heap_stats.xAvailableHeapSpaceInBytes;
stats->min_free_bytes = (uint32_t)heap_stats.xMinimumEverFreeBytesRemaining;
stats->largest_free_block =
(uint32_t)heap_stats.xSizeOfLargestFreeBlockInBytes;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think having xNumberOfSuccessfulAllocations and xNumberOfSuccessfulFrees would be very beneficial as well. As it can help us determine if there is a leak somewhere 🥬

Comment thread common/bm_freertos.c

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should also probably update the bm_posix.c file to make sure bm_sbc apps do not break.

@matt001k matt001k left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am a bit on the fence on whether or not this should be something that bm_core handles or bm_protocol, what was the rational of placing this here rather than bm_protocol?

Also @victorsowa12 what are your thoughts on where the memory statistics should live?

@Davidbick

Copy link
Copy Markdown
Contributor Author

I put the PR in bm_core following the ADIN component, but ADIN has its own reason to live here since core hardcodes the ADIN2111 as the network device. For memory, bm_core makes me add a bm_heap_stats() accessor to bm_os/bm_freertos; in bm_protocol I'd just call vPortGetHeapStats() directly. The one real tradeoff is that core lets memory_metrics self-register once in bristlemouth_init, whereas in bm_protocol I'd wire it into each app_main. I'm leaning bm_protocol so I am happy to move it! @matt001k @victorsowa12

@matt001k

matt001k commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

@Davidbick personally I am ok with either direction you want to go.
I could see benefit for it being in both, but we would have to come up with a good way for it to work in bm_posix.c.

@Davidbick

Copy link
Copy Markdown
Contributor Author

Ended up going with bm_protocol: bristlemouth/bm_protocol#416
The heap is an integrator/RTOS concern and bm_protocol owns the FreeRTOS heap so it keeps the change out of bm_core entirely and avoids widening the bm_os contract with a bm_heap_stats() that bm_posix.c can't faithfully implement (glibc has no equivalent for min-free/largest-block/alloc counts).

@Davidbick Davidbick closed this Aug 7, 2026
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.

2 participants