Overhaul RCC6 service dashboards - #10
Merged
Merged
Conversation
Five independent, behavior-preserving reductions. Measured on
Heltec_v3_repeater_observer_mqtt (non-PSRAM) and
ThinkNode_M7_repeater_observer_mqtt (PSRAM), 267/267 native tests green.
Drop the unused static-task bookkeeping. StaticTask_t _mqtt_task_tcb (344 B)
and StackType_t* _mqtt_task_stack were never used: there is no
xTaskCreateStatic call, the pointer was assigned nullptr immediately before
xTaskCreatePinnedToCore, and the two psram_free() calls on it were dead.
Size the wire-format scratch buffers from the protocol maximum. raw_hex[1024]
at three sites becomes 2*MAX_TRANS_UNIT+1, and raw_buf/reconstructed[512]
become MAX_TRANS_UNIT. Both writeTo() sites now check getRawLength() first:
writeTo() does not bounds-check and returns uint8_t, so the old 512-byte
buffers were the only thing absorbing a malformed payload_len, and the
post-hoc "raw_len > sizeof(buf)" test ran after the overrun.
Pass the already-known serialized length into publishToSlot() instead of
re-running strlen() per destination slot (up to 2 KB per packet per slot, and
NEIGHBORS_JSON_BUFFER_SIZE per neighbor snapshot). Same for the direct
publish in publishStatusToSlot().
Share one JSON buffer and one document across packet, raw, and status. All
publish paths serialize on the bridge task, so the separate status buffer and
document were never concurrent. Status keeps STATUS_JSON_BUFFER_SIZE as its
serialization ceiling, so which oversized status documents get dropped is
unchanged.
Route the document's pools through a PSRAM-preferring allocator. Under
ArduinoJson 7 StaticJsonDocument<N> is a deprecated empty subclass of
JsonDocument whose template argument only feeds capacity(); the object is
64 B and each pool block (4096 B here) came from plain malloc(), i.e. the
internal DRAM the mbedTLS working set needs. Mirrors NeighborsDocAllocator.
The old comment claiming an inline pool has been corrected.
Measured:
sizeof(MQTTBridge) non-PSRAM 13208 -> 12032 B (-1176, internal heap)
PSRAM 10492 -> 10080 B (-412, plus one fewer
768 B PSRAM allocation)
stack, non-PSRAM buildPacketJSON[FromRaw] 1264 -> 736 B
buildRawJSON 1136 -> 608 B
publishPacket 688 -> ~432 B
packetToHex 560 -> ~304 B
deepest publish chain ~2.6 -> ~1.8 KB of 8 KB
flash 1592513 -> 1592573 B (+60)
static RAM 74656 B unchanged -- MQTTBridge is heap-allocated, so
these savings are internal heap, not the linker figure
Deferred from the review: the QueuedPacket wire-only redesign (reward is
1.56 KB on non-PSRAM only, and Packet::readFrom() rejects payload_len == 0),
demand-driven slot clients/JWT tokens, and pool retention across publishes --
JsonDocument::to<T>() always calls clear(), which destroys pools, so
"retain pools by clearing the root object" needs a string-pool lifetime
analysis first.
initSlotClients() created a PsychicMqttClient for every one of
RUNTIME_MQTT_SLOTS at begin(), without consulting slot.enabled or the active
cap -- even though presets are applied and _max_active_slots is computed
earlier in the same function. RUNTIME_MQTT_SLOTS is deliberately cap+1
(6/5 with PSRAM, 3/2 without), so at least one client was always unusable,
and a lightly-configured board wasted several.
Replaced with ensureSlotClient(index), called from setupSlot() -- i.e. only
for a slot that is enabled, inside the cap, and ready to connect. A client
that never reaches setupSlot() is completely inert: the reconnect ladder is
gated on initial_connect_done, which only setupSlot() sets. Retained for the
bridge lifetime as before, so reconfigure/reconnect still reuse one mbedTLS
context; that context is built by connect(), not by the constructor, so
deferring costs nothing but the 1284-byte object.
Internal DRAM saved, by configured slot count:
non-PSRAM (3 runtime / 2 cap) 1 configured 2568 B
2-3 1284 B
PSRAM (6 runtime / 5 cap) 1 configured 6420 B
2 5136 B
5-6 1284 B
A BOARD_HAS_PSRAM board whose PSRAM fails to init gets cap 2 against 6
runtime slots, so it saves at least 5136 B -- on the board that just lost its
PSRAM.
Two gates used "client != nullptr" as a proxy for "configured". Under eager
allocation that conjunct was always true and therefore harmless; with lazy
allocation it would have made shouldQueuePacketType() drop every packet
received before the post-NTP-sync slot setup, which is precisely the window
the queue exists to cover. Both now key on slot.enabled, matching the
documented intent above eligiblePacketSlots() that a configured-but-
disconnected broker is still a target.
formatSlotDiagReply() gains the !isSlotReady() -> "wait" branch that
get mqtt.status and getSlotStatusSnapshot() already have; that state
previously fell through to "disc" and read as a network fault when it is
really a missing token/IATA/credential. "no client" now means only what its
name says: the slot is ready but the client could not be allocated. That
state is newly reachable, so the allocation uses new (std::nothrow) -- this
framework enables C++ exceptions, and a throwing new on exhaustion would
panic the node instead of degrading one slot.
flash non-PSRAM 1592573 -> 1592697 B (+124)
PSRAM 1555001 -> 1555109 B (+108)
267/267 native tests pass; both observer envs build clean.
MQTTSlot carried an inline char auth_token[768] -- 768 of its 1192 bytes --
for every runtime slot, whether the slot was JWT, username/password, disabled,
or above the active-slot cap. Now a char* allocated by ensureSlotAuthToken()
from createSlotAuthToken(), the sole writer, which runs only once a slot is
confirmed to have a JWT audience.
Because _slots[] is a fixed member array, this shrinks the bridge object
unconditionally at construction and re-adds only what is used:
MQTTSlot 1192 -> 428 B (-764)
MQTTBridge non-PSRAM 12032 -> 9740 B (-2292)
PSRAM 10080 -> 5496 B (-4584)
The object is allocated as one contiguous block at boot, so a smaller boot-time
chunk is easier to satisfy and leaves a larger contiguous remainder; JWT slots
then take 768 B each as separate blocks. Net steady-state saving is 768 B per
slot that never creates a token -- 768 B on a fully configured board, up to
3840 B on a PSRAM board with one JWT slot. 22 of the 29 presets are
MQTT_AUTH_JWT, so this is mostly reclaiming unconfigured slots rather than
non-JWT ones.
Lifetime rules, which are the whole risk here:
- setCredentials() stores this pointer in _mqtt_cfg rather than copying, and
esp-mqtt re-reads it whenever a later connect() re-applies a dirtied config.
So the buffer is freed only alongside the client, in destroySlotClients(),
after the delete. MQTTSlot::broker_uri already carries an "avoids dangling
pointer" comment from this same hazard with setServer().
- teardownSlot() still clears the token to an empty string but keeps the
buffer: the client survives teardown and does not clear cfg->password (only
setupSlot()'s reconfigure branch does). Freeing there would dangle.
- Never freed per reconnect, preserving the churn-avoidance the inline buffer
was there for.
Allocation goes through MQTTRuntimeBufferLifecycle (already host-tested) using
plain malloc, so the buffer stays in internal DRAM exactly where it was when
inline. Failure propagates through createSlotAuthToken()'s existing bool.
The two call sites that create a token and then test it (preset JWT in
setupSlot, and the custom-slot audience path) now null-check first; the five
other readers are all inside if (createSlotAuthToken(...)) success branches and
need no change. destroySlotClients() releases the token unconditionally rather
than after its client null-check, so a token can never outlive its slot.
flash non-PSRAM 1592697 -> 1592801 B (+104)
PSRAM 1555109 -> 1555213 B (+104)
267/267 native tests pass; both observer envs build clean.
ensureSlotAuthToken()/releaseSlotAuthToken() now use the bridge's existing
psram_malloc/psram_free rather than malloc/free, so a JWT slot's 768-byte token
comes out of PSRAM instead of internal DRAM. On PSRAM boards this is the larger
half of the auth_token work: the previous commit reclaims 768 B per slot that
never creates a token, while this reclaims 768 B per slot that does -- up to
3840 B of internal DRAM on a fully configured five-slot board, which is where
the mbedTLS working set is competing for space.
No effect on non-PSRAM boards: psram_malloc falls back to internal DRAM, so the
buffer stays exactly where the previous commit left it. Same for a
BOARD_HAS_PSRAM board whose PSRAM fails to initialise.
Safe to move because every access is a CPU copy on the bridge task, never DMA,
an ISR, or a cache-disabled window: JWTHelper memcpy's the token into this
buffer, and esp-mqtt copies it out of _mqtt_cfg into its own internal-DRAM
storage when connect() applies the config. This is unlike the PSRAM-backed MQTT
task stack that was tried and reverted for resetting Heltec V4 boards, where
the fault was PSRAM execution context rather than a plain buffer read.
Split from the previous commit so it can be reverted alone if hardware soak
shows any PSRAM-related instability on the JWT path.
flash non-PSRAM 1592801 -> 1592865 B (+64)
PSRAM 1555213 -> 1555237 B (+24)
267/267 native tests pass; both observer envs and an nRF52 repeater build clean.
[P1] A failed setup no longer strands the slot. setupSlot() returns bool and the startup loops count only successful activations, so a slot that fails on a client allocation neither consumes an active-slot position (starving a later healthy broker on capped hardware) nor sits dead forever: maintainSlotConnections() previously skipped clientless slots and the reconnect ladder is gated on initial_connect_done, so nothing retried it. It now retries an enabled but unactivated slot on a 60 s timer, one per cycle, gated on the same _slots_setup_done ordering so the NTP-deferred setup sequence is preserved. [P2] JWT setup no longer proceeds without a usable token. Both the preset and custom-audience paths returned after ignoring createSlotAuthToken()'s result, then called connect() and latched initial_connect_done -- so the token-allocation failure introduced by the previous commit produced an unauthenticated attempt exactly when memory was exhausted. They now return false and let the retry path handle it. [P2] ensureSlotAuthToken() no longer clears an existing token. It cleared unconditionally, so every renewal wiped the current token before JWTHelper ran; a renewal that then failed left an empty password where the inline buffer used to preserve working credentials (JWTHelper writes only on success). Only freshly allocated buffers are initialised now. [P2] Raw publications reuse the shared document. buildRawJSON() reached MQTTPayloadBuilder::buildRawMessage(), which constructed its own default JsonDocument and therefore malloc'd and freed an internal-heap variant pool per message -- on the highest-rate topic. The document is threaded through both builders and the bridge passes _json_scratch_doc. [P3] The writeTo() guard validates the source fields, not just the destination. A corrupt payload_len of MAX_PACKET_PAYLOAD + 1 still leaves getRawLength() inside MAX_TRANS_UNIT, so writeTo() read past packet->payload. Sizing and validation moved to a pure MQTTWireScratch header with host tests covering the accept/reject edges, matching the MQTTPacketFilter/MQTTConnectionPolicy pattern. Two findings fell out: MAX_PATH_SIZE one-byte hops is not encodable (the hop count is 6 bits, so 64 & 63 == 0; 32 two-byte hops is the widest real path), and a zero-payload packet serializes but does not survive readFrom() -- pinned as a test because it constrains any future wire-only queue. [P3] Corrected the pool-size comment: these targets are 32-bit, so ARDUINOJSON_SLOT_ID_SIZE is 2 and a pool block is 128 slots / 1024 bytes, not 4096. The 4096 figure came from a pre-existing comment near NEIGHBORS_DOC_POOL_BUDGET, which is left alone -- its byte measurements are empirical and still stand, only the block-size attribution is wrong. Activation is now centralized in activatedSlotCount()/canActivateSlot(), used by both startup loops, the retry path, and applySlotPreset(). That closes the pre-existing divergence where a live preset change called setupSlot() without consulting _max_active_slots, letting a non-PSRAM board reach three concurrent TLS sessions against a cap of two. BEHAVIOUR CHANGE: a reconfigure that would exceed the cap now logs and leaves the slot inactive instead of connecting. Reconfiguring an already-active slot still works, because teardownSlot() releases its position first. 272/272 native tests pass (5 new); both observer envs and an nRF52 repeater build clean. Flash 1593249 B non-PSRAM, 1555625 B PSRAM.
reconnect_attempted_this_cycle is computed once before the maintenance loop and only maintainSlotConnection() was setting it. The deferred-setup retry armed the 15 s cross-slot guard via _last_slot_reconnect_ms but left the local flag false, so a disconnected slot later in the same pass still saw "no reconnect yet" and started a second TLS handshake. Two concurrent ~40 KB sessions is exactly the contention the guard prevents, and it landed in the one situation where internal heap is already known to be short -- a failed allocation is why the retry runs. Set on success only: setupSlot() returns true only after client->connect(), so success means a handshake was launched. A failed retry launches nothing and continues to spend only setup_retry_this_cycle, which rate-limits the allocation attempts without consuming the handshake allowance.
last_reconnect_attempt starts at zero and teardownSlot() re-zeroes it, so the retry gate in maintainSlotConnections() reduced to "uptime >= SLOT_SETUP_RETRY_INTERVAL". Past 60 s of uptime a failed setup was therefore retried on the next maintenance pass rather than 60 s later -- in the same task iteration for a live reconfigure, since reconfigure processing runs before maintenance in the loop. setupSlot() now stamps last_reconnect_attempt on each failure that represents a real attempt (client allocation, and both JWT token paths), so all three callers get the interval measured from the failure. The bounds check and the !enabled early return are not attempts and stay unstamped. The reconnect ladder reads this field only for slots with initial_connect_done set, which a failed setup never sets, so reconnect timing is unaffected. The retry path's own pre-call stamp is kept as a backstop for any future false-returning path that does not stamp itself.
The post-NTP-correction refresh looped to _max_active_slots, which is a count of activation positions and never an index bound. The indices holding those positions are not contiguous: a slot passed over by isSlotReady() -- or, since the demand-driven work, by a failed setup -- leaves a higher index activated. On a two-position board that meant slots 1 and 2 could be live while only indices 0 and 1 were scanned, so slot 3 kept a JWT issued against the pre-correction clock until its own expiry or a reconnect regenerated it. Now bounded by RUNTIME_MQTT_SLOTS. The existing guard already skips disabled, non-JWT, and clientless slots, so widening the range cannot touch a slot that was never set up. Pre-existing (the loop predates the demand-driven work) and kept as its own commit so it can be picked separately. Audited the other _max_active_slots uses: all are count comparisons or log arguments, so this was the only misuse.
canSerialize() validated payload_len and the destination size but not whether the path encoding is one writePath() will actually emit. writePath() self-guards against overrunning the path array, but it does so by writing nothing and returning 0 — a correctness problem, not a safety one, because getRawLength() still counts the path. An over-long or reserved encoding therefore passed the size check and then serialized to a truncated frame that was published as the packet. Worst case is path_len 0xFF with no payload: 63 hops of 4 bytes counts as 254 bytes, inside the 255-byte buffer, while writeTo() emits just the 2-byte header. The `raw` field would carry 4 hex chars presented as the frame. Reserved 4-byte hash encodings passed too, producing frames Packet::readFrom() rejects. Now gated on Packet::isValidPathLen(), which rejects the reserved 4-byte hash size and any count * size above MAX_PATH_SIZE in one predicate. It is the same check readFrom() applies to every received packet, and TX packets are built via setPathHashSizeAndCount() with real hash sizes, so no decodable packet is turned away. Two tests added for the cases a destination-size check cannot reach. The existing truncation test passed for the wrong reason -- its payload_len of 4 pushed getRawLength() to 258 and tripped the size check, masking the hole -- so it is split into the >0xFF truncation case and the counted-length-fits case, with the 254/2-byte asymmetry asserted explicitly so it cannot be masked again. 274/274 native tests; both observer envs and an nRF52 repeater build clean.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce362be635
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }).join(""):"No servers configured."; | ||
| st.lastStats=s; | ||
| if(st.role!=="room_server"&&++st.neighborPoll%4===1)pollNeighbors(); | ||
| if(++st.neighborPoll%4===1)pollNeighbors(); |
There was a problem hiding this comment.
When the newly enabled room-server neighbor poll receives a valid location on the equator or prime meridian, the existing Number(n.lat) && Number(n.lon) checks in pollNeighbors() and drawNeighborMap() treat the zero coordinate as false. Such repeaters are counted as unlocated and omitted from the advertised-places view even though the new room-server response sets located:true; use the located flag plus finite-number validation instead of coordinate truthiness.
Useful? React with 👍 / 👎.
Comment on lines
+1422
to
+1423
| [10,5,0,-10].forEach(function(db,i){var radius=maxR*(i+1)/4;g.strokeStyle="rgba(57,231,255,.14)";g.beginPath();g.arc(cx,cy,radius,0,Math.PI*2);g.stroke();g.fillStyle="rgba(141,152,167,.62)";g.font="9px system-ui";g.fillText(db+" dB",cx+7,cy-radius+12)}); | ||
| nodes.forEach(function(n,i){var id=String(n.id||n.name||i),seed=0;for(var j=0;j<id.length;j++)seed=(seed+id.charCodeAt(j)*(j+3))%65535;var angle=(seed%360)*Math.PI/180,rssi=Number(n.rssi),snr=Math.max(-20,Math.min(12,Number(n.snr||-20))),distance=isFinite(rssi)?(-Math.max(-130,Math.min(-70,rssi))-70)/60:(12-snr)/32,radius=maxR*Math.max(.12,Math.min(1,distance)),x=cx+Math.cos(angle)*radius,y=cy+Math.sin(angle)*radius,age=Math.max(0,Number(n.age||0)),alpha=Math.max(.3,1-Math.min(1,age/21600));g.globalAlpha=alpha;g.fillStyle="#ff8a3d";g.shadowColor="#ff8a3d";g.shadowBlur=10;g.beginPath();g.arc(x,y,4,0,Math.PI*2);g.fill();g.shadowBlur=0;if(i<18){g.fillStyle="#f5f8fb";g.font="10px system-ui";g.fillText(String(n.name||"Unnamed").slice(0,18),x+8,y-6)}g.globalAlpha=1}); |
There was a problem hiding this comment.
The plotted radius uses (12 - snr) / 32, so the four rings at 25%, 50%, 75%, and 100% correspond to approximately 4, -4, -12, and -20 dB, not the displayed 10, 5, 0, and -10 dB. This makes nearly every neighbor appear against the wrong signal reference; derive the labels from the radius formula or change the mapping to match the stated ring values.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
/api/neighborsresponse with names and advertised locations.8d1a0eb3.Why
Repeater and room-server operators need one readable dashboard for RF, MQTT, room activity, capacity, and nearby nodes without technical or AI-style marketing language. The MQTT fixes lower runtime memory demand and close retry/token/path correctness issues without changing the configured broker set.
Deliberately excluded
Checks
python scripts/verify_rcc6_room_server.pypython scripts/configure_rcc6.py --self-testpio test -e native: 282/282 passedgit diff --checkThe full existing Actions firmware matrix is required before merge.