Skip to content

Conversation

Links2004
Copy link
Contributor

@Links2004 Links2004 commented Sep 20, 2025

rework I2C sensor init.

The goal is to only instantiate sensors that are pressend to save memory.

Side effacts:

  • easier sensor integration (less C&P code, sensor only needs to be "registerd" at one place)
  • nodeTelemetrySensorsMap can be removed when all devices are migrated to save more RAM

🤝 Attestations

  • I have tested that my proposed changes behave as described.
  • I have tested that my proposed changes do not cause any obvious regressions on the following devices:
    • Heltec (Lora32) V2.1
    • Heltec (Lora32) V3
    • LilyGo T-Deck
    • LilyGo T-Beam
    • RAK WisBlock 4631
    • Seeed Studio T-1000E tracker card
    • Other (please specify below)

@thebentern thebentern requested review from caveman99 and Copilot and removed request for caveman99 September 20, 2025 12:31
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request replaces static allocation of the bsecState buffer with dynamic allocation using malloc() to save 238 bytes of static memory. The change moves the buffer allocation from a class member to local variables within the methods that use it.

Key Changes

  • Removed static bsecState array from BME680Sensor class member variables
  • Added dynamic allocation with malloc() in loadState() and updateState() methods
  • Added proper error handling and memory cleanup with free()

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
BME680Sensor.h Removes static bsecState array declaration from class members
BME680Sensor.cpp Implements dynamic allocation with malloc/free in loadState() and updateState() methods

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@thebentern thebentern added the enhancement New feature or request label Sep 20, 2025
@mverch67
Copy link
Collaborator

A much larger portion of static allocation could be saved if all the sensor instances are declared as pointer and allocated in heap space during init. Then no changes in this BSEC sensor class are required, e.g. see here:

@Links2004
Copy link
Contributor Author

Links2004 commented Sep 20, 2025

true, had a look at EnvironmentTelemetry.cpp and it looks like there is a lot more possibility for optimizion.
In my opinion there is a loot to gain there, the current desigen looks very inefficient over all.

looked a bit deeper and I think the I2C device handling overall needs some work.

@Links2004 Links2004 marked this pull request as draft September 20, 2025 20:13
@Links2004 Links2004 changed the title use malloc for bsecState - saves 238 byte of static allocation [WIP] rework sensor allocation to saves all the static allocation Sep 21, 2025
@Links2004
Copy link
Contributor Author

have working code that elimenates the need for nodeTelemetrySensorsMap and only creates the sensor instances when the I2C scan found a device.

moved over 4 sensors to show case the idea, but want some feedback before doing all of them.
moving this 4 sensors over saved already 1152 bytes of static allocations.

I dont see that we get memory fragmentation problem with this code, since the only cases where we delete / free something is when the sensor failed it initialisation.

tested this with a BMP-280 connected to a Heltec V2.1

please review and give feedback if this code is something that can be merged (will migrate the other sensors then).

@Links2004 Links2004 marked this pull request as ready for review September 21, 2025 09:57
@Links2004 Links2004 changed the title [WIP] rework sensor allocation to saves all the static allocation [WIP][needs feedback] rework sensor allocation to saves all the static allocation Sep 21, 2025
the goal is to only instantiate sensors that are pressend to save memory.
side effacts:
 - easyer sensor integration (less C&P code)
 - nodeTelemetrySensorsMap can be removed when all devices are migrated
@Links2004 Links2004 mentioned this pull request Sep 21, 2025
8 tasks
@Links2004 Links2004 changed the title [WIP][needs feedback] rework sensor allocation to saves all the static allocation [WIP][needs feedback] rework sensor instantiation to saves memory by removing the static allocation Sep 21, 2025
@jp-bennett jp-bennett requested a review from fifieldt September 21, 2025 17:57
@Links2004
Copy link
Contributor Author

did a second batch of sensors

RAM -816
Flash -916

@Links2004 Links2004 changed the title [WIP][needs feedback] rework sensor instantiation to saves memory by removing the static allocation rework sensor instantiation to saves memory by removing the static allocation Sep 27, 2025
not sure what magic is used but it works
@thebentern
Copy link
Contributor

Tagging @oscgonfer for visibility

@Links2004
Copy link
Contributor Author

The PR now covers all sensors that where allocated staticly in EnvironmentTelemetry.

Total saved:

RAM -2160
Flash -4332

we will save CPU cycles too since we do a lot less hasSensor calls.

some sensors like the INA ones are part of PowerTelemetry and EnvironmentTelemetry not sure why but I did not touch them.

Ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants