mirror/firmware
1
0
Fork 0
mirror of https://github.com/meshtastic/firmware.git synced 2026-05-18 21:56:34 -04:00
Code Issues Packages Projects Releases Wiki Activity

Smart pointers and memory management cleanup (#10400)

Browse Source 
* Refactor memory management in Syslog and StoreForwardModule

* Implement destructor for Lock

* Refactor RotaryEncoder and PacketHistory to use smart pointers for better memory management

* CH341 should use unique_ptr for improved memory management

* Fix checks in PH

* Improve Syslog::vlogf to handle variable argument lists more safely

* Fix initOk method to use nullptr for null pointer check
This commit is contained in:
Ben Meadors
2026-05-06 15:33:59 -05:00
committed by GitHub
parent 6e810741f3
commit 220bb4d186
10 changed files with 68 additions and 71 deletions
Download Patch File Download Diff File Expand all files Collapse all files

33
src/DebugConfiguration.cpp
View File

@@ -26,6 +26,8 @@ SOFTWARE.*/
#include "DebugConfiguration.h"
#include <memory>
#ifdef ARCH_PORTDUINO
#include "platform/portduino/PortduinoGlue.h"
#endif
@@ -119,27 +121,22 @@ bool Syslog::vlogf(uint16_t pri, const char *fmt, va_list args)
bool Syslog::vlogf(uint16_t pri, const char *appName, const char *fmt, va_list args)
{
char *message;
size_t initialLen;
size_t len;
bool result;
// First measure the formatted length using a copy of args; passing args directly
// to vsnprintf consumes it, and reusing a consumed va_list is undefined behavior.
va_list args_measure;
va_copy(args_measure, args);
int needed = vsnprintf(nullptr, 0, fmt, args_measure);
va_end(args_measure);
initialLen = strlen(fmt);
if (needed < 0)
return false; // encoding error
message = new char[initialLen + 1];
auto message = std::unique_ptr<char[]>(new char[static_cast<size_t>(needed) + 1]);
int written = vsnprintf(message.get(), static_cast<size_t>(needed) + 1, fmt, args);
if (written < 0)
return false;
len = vsnprintf(message, initialLen + 1, fmt, args);
if (len > initialLen) {
delete[] message;
message = new char[len + 1];
vsnprintf(message, len + 1, fmt, args);
}
result = this->_sendLog(pri, appName, message);
delete[] message;
return result;
return this->_sendLog(pri, appName, message.get());
}
inline bool Syslog::_sendLog(uint16_t pri, const char *appName, const char *message)

7
src/concurrency/Lock.cpp
View File

@@ -14,6 +14,11 @@ Lock::Lock() : handle(xSemaphoreCreateBinary())
}
}
Lock::~Lock()
{
vSemaphoreDelete(handle);
}
void Lock::lock()
{
if (xSemaphoreTake(handle, portMAX_DELAY) == false) {
@@ -30,6 +35,8 @@ void Lock::unlock()
#else
Lock::Lock() {}
Lock::~Lock() {}
void Lock::lock() {}
void Lock::unlock() {}

1
src/concurrency/Lock.h
View File

@@ -12,6 +12,7 @@ class Lock
{
public:
Lock();
~Lock();
Lock(const Lock &) = delete;
Lock &operator=(const Lock &) = delete;

11
src/input/RotaryEncoderImpl.cpp
View File

@@ -13,7 +13,6 @@ RotaryEncoderImpl *rotaryEncoderImpl;
RotaryEncoderImpl::RotaryEncoderImpl()
{
rotary = nullptr;
#ifdef ARCH_ESP32
isFirstInit = true;
#endif
@@ -23,11 +22,6 @@ RotaryEncoderImpl::~RotaryEncoderImpl()
{
LOG_DEBUG("RotaryEncoderImpl destructor");
detachRotaryEncoderInterrupts();
if (rotary != nullptr) {
delete rotary;
rotary = nullptr;
}
}
bool RotaryEncoderImpl::init()
@@ -43,8 +37,9 @@ bool RotaryEncoderImpl::init()
eventPressed = static_cast<input_broker_event>(moduleConfig.canned_message.inputbroker_event_press);
if (rotary == nullptr) {
rotary = new RotaryEncoder(moduleConfig.canned_message.inputbroker_pin_a, moduleConfig.canned_message.inputbroker_pin_b,
moduleConfig.canned_message.inputbroker_pin_press);
rotary.reset(new RotaryEncoder(moduleConfig.canned_message.inputbroker_pin_a,
moduleConfig.canned_message.inputbroker_pin_b,
moduleConfig.canned_message.inputbroker_pin_press));
}
attachRotaryEncoderInterrupts();

3
src/input/RotaryEncoderImpl.h
View File

@@ -5,6 +5,7 @@
#include "InputBroker.h"
#include "concurrency/OSThread.h"
#include "mesh/NodeDB.h"
#include <memory>
class RotaryEncoder;
@@ -28,7 +29,7 @@ class RotaryEncoderImpl final : public InputPollable
input_broker_event eventCcw = INPUT_BROKER_NONE;
input_broker_event eventPressed = INPUT_BROKER_NONE;
RotaryEncoder *rotary;
std::unique_ptr<RotaryEncoder> rotary;
private:
#ifdef ARCH_ESP32

55
src/mesh/PacketHistory.cpp
View File

@@ -16,7 +16,7 @@
#define VERBOSE_PACKET_HISTORY 0 // Set to 1 for verbose logging, 2 for heavy debugging
#define PACKET_HISTORY_TRACE_AGING 1 // Set to 1 to enable logging of the age of re/used history slots
PacketHistory::PacketHistory(uint32_t size) : recentPacketsCapacity(0), recentPackets(NULL) // Initialize members
PacketHistory::PacketHistory(uint32_t size) : recentPacketsCapacity(0) // Initialize members
{
if (size < 4 || size > PACKETHISTORY_MAX) { // Copilot suggested - makes sense
LOG_WARN("Packet History - Invalid size %d, using default %d", size, PACKETHISTORY_MAX);
@@ -25,7 +25,7 @@ PacketHistory::PacketHistory(uint32_t size) : recentPacketsCapacity(0), recentPa
// Allocate memory for the recent packets array
recentPacketsCapacity = size;
recentPackets = new PacketRecord[recentPacketsCapacity];
recentPackets.reset(new PacketRecord[recentPacketsCapacity]);
if (!recentPackets) { // No logging here, console/log probably uninitialized yet.
LOG_ERROR("Packet History - Memory allocation failed for size=%d entries / %d Bytes", size,
sizeof(PacketRecord) * recentPacketsCapacity);
@@ -34,14 +34,7 @@ PacketHistory::PacketHistory(uint32_t size) : recentPacketsCapacity(0), recentPa
}
// Initialize the recent packets array to zero
memset(recentPackets, 0, sizeof(PacketRecord) * recentPacketsCapacity);
}
PacketHistory::~PacketHistory()
{
recentPacketsCapacity = 0;
delete[] recentPackets;
recentPackets = NULL;
memset(recentPackets.get(), 0, sizeof(PacketRecord) * recentPacketsCapacity);
}
/** Update recentPackets and return true if we have already seen this packet */
@@ -205,13 +198,14 @@ PacketHistory::PacketRecord *PacketHistory::find(NodeNum sender, PacketId id)
return NULL;
}
PacketRecord *base = recentPackets.get();
PacketRecord *it = NULL;
for (it = recentPackets; it < (recentPackets + recentPacketsCapacity); ++it) {
for (it = base; it < (base + recentPacketsCapacity); ++it) {
if (it->id == id && it->sender == sender) {
#if VERBOSE_PACKET_HISTORY
LOG_DEBUG("Packet History - find: s=%08x id=%08x FOUND nh=%02x rby=%02x %02x %02x age=%d slot=%d/%d", it->sender,
it->id, it->next_hop, it->relayed_by[0], it->relayed_by[1], it->relayed_by[2], millis() - (it->rxTimeMsec),
it - recentPackets, recentPacketsCapacity);
it - base, recentPacketsCapacity);
#endif
// only the first match is returned, so be careful not to create duplicate entries
return it; // Return pointer to the found record
@@ -229,39 +223,38 @@ void PacketHistory::insert(const PacketRecord &r)
{
uint32_t now_millis = millis(); // Should not jump with time changes
uint32_t OldtrxTimeMsec = 0;
PacketRecord *base = recentPackets.get();
PacketRecord *tu = NULL; // Will insert here.
PacketRecord *it = NULL;
// Find a free, matching or oldest used slot in the recentPackets array
for (it = recentPackets; it < (recentPackets + recentPacketsCapacity); ++it) {
for (it = base; it < (base + recentPacketsCapacity); ++it) {
if (it->id == 0 && it->sender == 0 /*&& rxTimeMsec == 0*/) { // Record is empty
tu = it; // Remember the free slot
#if VERBOSE_PACKET_HISTORY >= 2
LOG_DEBUG("Packet History - insert: Free slot@ %d/%d", tu - recentPackets, recentPacketsCapacity);
LOG_DEBUG("Packet History - insert: Free slot@ %d/%d", tu - base, recentPacketsCapacity);
#endif
// We have that, Exit the loop
it = (recentPackets + recentPacketsCapacity);
it = (base + recentPacketsCapacity);
} else if (it->id == r.id && it->sender == r.sender) { // Record matches the packet we want to insert
tu = it; // Remember the matching slot
OldtrxTimeMsec = now_millis - it->rxTimeMsec; // ..and save current entry's age
#if VERBOSE_PACKET_HISTORY >= 2
LOG_DEBUG("Packet History - insert: Matched slot@ %d/%d age=%d", tu - recentPackets, recentPacketsCapacity,
OldtrxTimeMsec);
LOG_DEBUG("Packet History - insert: Matched slot@ %d/%d age=%d", tu - base, recentPacketsCapacity, OldtrxTimeMsec);
#endif
// We have that, Exit the loop
it = (recentPackets + recentPacketsCapacity);
it = (base + recentPacketsCapacity);
} else {
if (it->rxTimeMsec == 0) {
LOG_WARN(
"Packet History - insert: Found packet s=%08x id=%08x with rxTimeMsec = 0, slot %d/%d. Should never happen!",
it->sender, it->id, it - recentPackets, recentPacketsCapacity);
it->sender, it->id, it - base, recentPacketsCapacity);
}
if ((now_millis - it->rxTimeMsec) > OldtrxTimeMsec) { // 49.7 days rollover friendly
OldtrxTimeMsec = now_millis - it->rxTimeMsec;
tu = it; // remember the oldest packet
#if VERBOSE_PACKET_HISTORY >= 2
LOG_DEBUG("Packet History - insert: Older slot@ %d/%d age=%d", tu - recentPackets, recentPacketsCapacity,
OldtrxTimeMsec);
LOG_DEBUG("Packet History - insert: Older slot@ %d/%d age=%d", tu - base, recentPacketsCapacity, OldtrxTimeMsec);
#endif
}
// keep looking for oldest till entire array is checked
@@ -276,13 +269,11 @@ void PacketHistory::insert(const PacketRecord &r)
#if VERBOSE_PACKET_HISTORY
if (tu->id == 0 && tu->sender == 0) {
LOG_DEBUG("Packet History - insert: slot@ %d/%d is NEW", tu - recentPackets, recentPacketsCapacity);
LOG_DEBUG("Packet History - insert: slot@ %d/%d is NEW", tu - base, recentPacketsCapacity);
} else if (tu->id == r.id && tu->sender == r.sender) {
LOG_DEBUG("Packet History - insert: slot@ %d/%d MATCHED, age=%d", tu - recentPackets, recentPacketsCapacity,
OldtrxTimeMsec);
LOG_DEBUG("Packet History - insert: slot@ %d/%d MATCHED, age=%d", tu - base, recentPacketsCapacity, OldtrxTimeMsec);
} else {
LOG_DEBUG("Packet History - insert: slot@ %d/%d REUSE OLDEST, age=%d", tu - recentPackets, recentPacketsCapacity,
OldtrxTimeMsec);
LOG_DEBUG("Packet History - insert: slot@ %d/%d REUSE OLDEST, age=%d", tu - base, recentPacketsCapacity, OldtrxTimeMsec);
}
#endif
@@ -315,9 +306,9 @@ void PacketHistory::insert(const PacketRecord &r)
#endif
#if VERBOSE_PACKET_HISTORY
LOG_DEBUG("Packet History - insert: Store slot@ %d/%d s=%08x id=%08x nh=%02x rby=%02x %02x %02x rxT=%d BEFORE",
tu - recentPackets, recentPacketsCapacity, tu->sender, tu->id, tu->next_hop, tu->relayed_by[0], tu->relayed_by[1],
tu->relayed_by[2], tu->rxTimeMsec);
LOG_DEBUG("Packet History - insert: Store slot@ %d/%d s=%08x id=%08x nh=%02x rby=%02x %02x %02x rxT=%d BEFORE", tu - base,
recentPacketsCapacity, tu->sender, tu->id, tu->next_hop, tu->relayed_by[0], tu->relayed_by[1], tu->relayed_by[2],
tu->rxTimeMsec);
#endif
if (r.rxTimeMsec == 0) {
@@ -330,9 +321,9 @@ void PacketHistory::insert(const PacketRecord &r)
*tu = r; // store the packet
#if VERBOSE_PACKET_HISTORY
LOG_DEBUG("Packet History - insert: Store slot@ %d/%d s=%08x id=%08x nh=%02x rby=%02x %02x %02x rxT=%d AFTER",
tu - recentPackets, recentPacketsCapacity, tu->sender, tu->id, tu->next_hop, tu->relayed_by[0], tu->relayed_by[1],
tu->relayed_by[2], tu->rxTimeMsec);
LOG_DEBUG("Packet History - insert: Store slot@ %d/%d s=%08x id=%08x nh=%02x rby=%02x %02x %02x rxT=%d AFTER", tu - base,
recentPacketsCapacity, tu->sender, tu->id, tu->next_hop, tu->relayed_by[0], tu->relayed_by[1], tu->relayed_by[2],
tu->rxTimeMsec);
#endif
}

8
src/mesh/PacketHistory.h
View File

@@ -1,6 +1,7 @@
#pragma once
#include "NodeDB.h"
#include <memory>
// Number of relayers we keep track of. Use 6 to be efficient with memory alignment of PacketRecord to 20 bytes
#define NUM_RELAYERS 6
@@ -26,7 +27,7 @@ class PacketHistory
uint32_t recentPacketsCapacity =
0; // Can be set in constructor, no need to recompile. Used to allocate memory for mx_recentPackets.
PacketRecord *recentPackets = NULL; // Simple and fixed in size. Debloat.
std::unique_ptr<PacketRecord[]> recentPackets; // Simple and fixed in size. Debloat.
/** Find a packet record in history.
* @param sender NodeNum
@@ -48,11 +49,8 @@ class PacketHistory
uint8_t getOurTxHopLimit(const PacketRecord &r);
void setOurTxHopLimit(PacketRecord &r, uint8_t hopLimit);
PacketHistory(const PacketHistory &); // non construction-copyable
PacketHistory &operator=(const PacketHistory &); // non copyable
public:
explicit PacketHistory(uint32_t size = -1); // Constructor with size parameter, default is PACKETHISTORY_MAX
~PacketHistory();
/**
* Update recentBroadcasts and return true if we have already seen this packet
@@ -74,5 +72,5 @@ class PacketHistory
void removeRelayer(const uint8_t relayer, const uint32_t id, const NodeNum sender);
// To check if the PacketHistory was initialized correctly by constructor
bool initOk(void) { return recentPackets != NULL && recentPacketsCapacity != 0; }
bool initOk(void) { return recentPackets != nullptr && recentPacketsCapacity != 0; }
};

4
src/modules/StoreForwardModule.cpp
View File

@@ -80,9 +80,9 @@ void StoreForwardModule::populatePSRAM()
(this->records ? this->records : (((memGet.getFreePsram() / 4) * 3) / sizeof(PacketHistoryStruct)));
this->records = numberOfPackets;
#if defined(ARCH_ESP32)
this->packetHistory = static_cast<PacketHistoryStruct *>(ps_calloc(numberOfPackets, sizeof(PacketHistoryStruct)));
this->packetHistory.reset(static_cast<PacketHistoryStruct *>(ps_calloc(numberOfPackets, sizeof(PacketHistoryStruct))));
#elif defined(ARCH_PORTDUINO)
this->packetHistory = static_cast<PacketHistoryStruct *>(calloc(numberOfPackets, sizeof(PacketHistoryStruct)));
this->packetHistory.reset(static_cast<PacketHistoryStruct *>(calloc(numberOfPackets, sizeof(PacketHistoryStruct))));
#endif

9
src/modules/StoreForwardModule.h
View File

@@ -7,6 +7,7 @@
#include "configuration.h"
#include <Arduino.h>
#include <functional>
#include <memory>
#include <unordered_map>
struct PacketHistoryStruct {
@@ -29,11 +30,17 @@ struct PacketHistoryStruct {
class StoreForwardModule : private concurrency::OSThread, public ProtobufModule<meshtastic_StoreAndForward>
{
// packetHistory is allocated with ps_calloc / calloc, so it must be released with free(),
// not delete[].
struct CFreeDeleter {
void operator()(PacketHistoryStruct *p) const noexcept { free(p); }
};
bool busy = 0;
uint32_t busyTo = 0;
char routerMessage[meshtastic_Constants_DATA_PAYLOAD_LEN] = {0};
PacketHistoryStruct *packetHistory = 0;
std::unique_ptr<PacketHistoryStruct[], CFreeDeleter> packetHistory;
uint32_t packetHistoryTotalCount = 0;
uint32_t last_time = 0;
uint32_t requestCount = 0;

8
src/platform/portduino/PortduinoGlue.cpp
View File

@@ -18,6 +18,7 @@
#include <fstream>
#include <iostream>
#include <map>
#include <memory>
#include <set>
#include <stdexcept>
#include <sys/ioctl.h>
@@ -297,10 +298,9 @@ void portduinoSetup()
// Try CH341
try {
std::cout << "autoconf: Looking for CH341 device..." << std::endl;
ch341Hal = new Ch341Hal(0, portduino_config.lora_usb_serial_num, portduino_config.lora_usb_vid,
portduino_config.lora_usb_pid);
ch341Hal->getProductString(autoconf_product, 95);
delete ch341Hal;
auto probe = std::unique_ptr<Ch341Hal>(new Ch341Hal(0, portduino_config.lora_usb_serial_num,
portduino_config.lora_usb_vid, portduino_config.lora_usb_pid));
probe->getProductString(autoconf_product, 95);
std::cout << "autoconf: Found CH341 device " << autoconf_product << std::endl;
found_ch341 = true;