Skip to content

Commit 5c5cb09

Browse files
authored
fix(admin): stop the admin dispatcher from overflowing the ESP32 loopTask stack (#11239)
AdminModule::handleReceivedProtobuf carried a 3008-byte stack frame - 37% of the 8 KB Arduino loopTask stack. GCC inlines the eight handleGet* helpers into it, each of which builds a whole meshtastic_AdminMessage (480 B), so the dispatcher reserved a slot for every one of them at once even though only one case ever runs. Two more full AdminMessages lived directly in the function. That left any admin write short of stack for what runs beneath it: NodeDB::saveToDisk -> saveProto -> pb_encode -> SafeFile -> LittleFS -> flash. On heltec-v4 the canary fired at 7824 of 8192 bytes while writing /prefs/config.proto, so no setting ever persisted and the device rebooted. Mark the response builders NOINLINE and move the module-API and observer responses into their own functions. Pure refactor; measured on heltec-v4 the dispatcher frame drops 3008 -> 384 bytes, taking the crashing path from 368 bytes of headroom to 2992. Fixes #11237
1 parent 69b68e2 commit 5c5cb09

3 files changed

Lines changed: 65 additions & 29 deletions

File tree

src/meshUtils.h

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,16 @@
55
#include <iterator>
66
#include <stdint.h>
77

8+
/// Keep a function out of line even when the compiler would rather inline it. Use on helpers that
9+
/// hold a large object on the stack: inlining several of them into one caller makes that caller's
10+
/// frame reserve every helper's locals at once, which on our 8 KB Arduino loopTask is enough to
11+
/// overflow the stack (see issue #11237).
12+
#if defined(__GNUC__)
13+
#define NOINLINE __attribute__((noinline))
14+
#else
15+
#define NOINLINE
16+
#endif
17+
818
/// C++ v17+ clamp function, limits a given value to a range defined by lo and hi
919
template <class T> constexpr const T &clamp(const T &v, const T &lo, const T &hi)
1020
{

src/modules/AdminModule.cpp

Lines changed: 31 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -681,22 +681,41 @@ bool AdminModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshta
681681
#endif
682682

683683
default:
684-
meshtastic_AdminMessage res = meshtastic_AdminMessage_init_default;
685-
AdminMessageHandleResult handleResult = MeshModule::handleAdminMessageForAllModules(mp, r, &res);
686-
687-
if (handleResult == AdminMessageHandleResult::HANDLED_WITH_RESPONSE) {
688-
setPassKey(&res);
689-
myReply = allocDataProtobuf(res);
690-
} else if (mp.decoded.want_response) {
691-
LOG_DEBUG("Module API did not respond to admin message. req.variant=%d", r->which_payload_variant);
692-
} else if (handleResult != AdminMessageHandleResult::HANDLED) {
693-
// Probably a message sent by us or sent to our local node. FIXME, we should avoid scanning these messages
694-
LOG_DEBUG("Module API did not handle admin message %d", r->which_payload_variant);
695-
}
684+
handleViaModuleApi(mp, r);
696685
break;
697686
}
698687

699688
// Allow any observers (e.g. the UI) to handle/respond
689+
handleViaObservers(r);
690+
691+
// If asked for a response and it is not yet set, generate an 'ACK' response
692+
if (mp.decoded.want_response && !myReply) {
693+
myReply = allocErrorResponse(meshtastic_Routing_Error_NONE, &mp);
694+
}
695+
if (mp.pki_encrypted && myReply) {
696+
myReply->pki_encrypted = true;
697+
}
698+
return handled;
699+
}
700+
701+
void AdminModule::handleViaModuleApi(const meshtastic_MeshPacket &mp, meshtastic_AdminMessage *r)
702+
{
703+
meshtastic_AdminMessage res = meshtastic_AdminMessage_init_default;
704+
AdminMessageHandleResult handleResult = MeshModule::handleAdminMessageForAllModules(mp, r, &res);
705+
706+
if (handleResult == AdminMessageHandleResult::HANDLED_WITH_RESPONSE) {
707+
setPassKey(&res);
708+
myReply = allocDataProtobuf(res);
709+
} else if (mp.decoded.want_response) {
710+
LOG_DEBUG("Module API did not respond to admin message. req.variant=%d", r->which_payload_variant);
711+
} else if (handleResult != AdminMessageHandleResult::HANDLED) {
712+
// Probably a message sent by us or sent to our local node. FIXME, we should avoid scanning these messages
713+
LOG_DEBUG("Module API did not handle admin message %d", r->which_payload_variant);
714+
}
715+
}
716+
717+
void AdminModule::handleViaObservers(const meshtastic_AdminMessage *r)
718+
{
700719
AdminMessageHandleResult observerResult = AdminMessageHandleResult::NOT_HANDLED;
701720
meshtastic_AdminMessage observerResponse = meshtastic_AdminMessage_init_default;
702721
AdminModule_ObserverData observerData = {
@@ -714,15 +733,6 @@ bool AdminModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshta
714733
} else if (observerResult == AdminMessageHandleResult::HANDLED) {
715734
LOG_DEBUG("Observer handled admin message");
716735
}
717-
718-
// If asked for a response and it is not yet set, generate an 'ACK' response
719-
if (mp.decoded.want_response && !myReply) {
720-
myReply = allocErrorResponse(meshtastic_Routing_Error_NONE, &mp);
721-
}
722-
if (mp.pki_encrypted && myReply) {
723-
myReply->pki_encrypted = true;
724-
}
725-
return handled;
726736
}
727737

728738
void AdminModule::handleGetModuleConfigResponse(const meshtastic_MeshPacket &mp, meshtastic_AdminMessage *r)

src/modules/AdminModule.h

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
#include <esp_ota_ops.h>
44
#endif
55
#include "ProtobufModule.h"
6+
#include "meshUtils.h"
67
#include <sys/types.h>
78
#if HAS_WIFI
89
#include "mesh/wifi/WiFiAPClient.h"
@@ -48,16 +49,24 @@ class AdminModule : public ProtobufModule<meshtastic_AdminMessage>, public Obser
4849

4950
/**
5051
* Getters
52+
*
53+
* Each of the NOINLINE ones below builds a whole meshtastic_AdminMessage (480 bytes) on the
54+
* stack. They are only ever reached from one case of handleReceivedProtobuf()'s switch, but
55+
* when the compiler inlines them the dispatcher's frame has to reserve a slot for every one of
56+
* them at once - measured at 3008 bytes on ESP32-S3, 37% of the 8 KB Arduino loopTask stack
57+
* that also has to carry PhoneAPI, the router, nanopb and LittleFS below it. Keeping them out
58+
* of line means only the request actually being served pays for its response buffer.
59+
* See issue #11237.
5160
*/
5261
void handleGetModuleConfigResponse(const meshtastic_MeshPacket &req, meshtastic_AdminMessage *p);
53-
void handleGetOwner(const meshtastic_MeshPacket &req);
54-
void handleGetConfig(const meshtastic_MeshPacket &req, uint32_t configType);
55-
void handleGetModuleConfig(const meshtastic_MeshPacket &req, uint32_t configType);
56-
void handleGetChannel(const meshtastic_MeshPacket &req, uint32_t channelIndex);
57-
void handleGetDeviceMetadata(const meshtastic_MeshPacket &req);
58-
void handleGetDeviceConnectionStatus(const meshtastic_MeshPacket &req);
59-
void handleGetNodeRemoteHardwarePins(const meshtastic_MeshPacket &req);
60-
void handleGetDeviceUIConfig(const meshtastic_MeshPacket &req);
62+
NOINLINE void handleGetOwner(const meshtastic_MeshPacket &req);
63+
NOINLINE void handleGetConfig(const meshtastic_MeshPacket &req, uint32_t configType);
64+
NOINLINE void handleGetModuleConfig(const meshtastic_MeshPacket &req, uint32_t configType);
65+
NOINLINE void handleGetChannel(const meshtastic_MeshPacket &req, uint32_t channelIndex);
66+
NOINLINE void handleGetDeviceMetadata(const meshtastic_MeshPacket &req);
67+
NOINLINE void handleGetDeviceConnectionStatus(const meshtastic_MeshPacket &req);
68+
NOINLINE void handleGetNodeRemoteHardwarePins(const meshtastic_MeshPacket &req);
69+
NOINLINE void handleGetDeviceUIConfig(const meshtastic_MeshPacket &req);
6170
/**
6271
* Setters
6372
*/
@@ -102,6 +111,13 @@ class AdminModule : public ProtobufModule<meshtastic_AdminMessage>, public Obser
102111
/// Whether a response (variant responseVariant, module-config subtype moduleConfigTag or 0)
103112
/// from mp.from answers a request we sent; consumes the matched request so it can't be replayed.
104113
bool responseIsSolicited(const meshtastic_MeshPacket &mp, pb_size_t responseVariant, pb_size_t moduleConfigTag);
114+
115+
/// Offer an admin message we have no case for to the module API, and let observers (e.g. the UI)
116+
/// see every admin message. Both build a response on the stack, so like the getters above they
117+
/// stay out of line to keep handleReceivedProtobuf()'s frame small.
118+
NOINLINE void handleViaModuleApi(const meshtastic_MeshPacket &mp, meshtastic_AdminMessage *r);
119+
NOINLINE void handleViaObservers(const meshtastic_AdminMessage *r);
120+
105121
void handleStoreDeviceUIConfig(const meshtastic_DeviceUIConfig &uicfg);
106122
void handleSendInputEvent(const meshtastic_AdminMessage_InputEvent &inputEvent);
107123
void reboot(int32_t seconds);

0 commit comments

Comments
 (0)