Stored 0 was the shipped default from 2026-01-02 to 2026-03-28, and every
association path has run it with power save off since. Reading it as
MIN_MODEM, as F11 did, would have put every node set up in that window
into modem sleep on its next association. 0 now reads as `none`, and
`set wifi.powersave min` stores a new value 3. /mqtt.json accepts 0..3;
older firmware repairs 3 to `none` on load. Binary snapshots are no longer
written, so 3 never reaches a legacy layout.
The upstream/dev merge kept our side of variants/thinknode_m7/platformio.ini,
losing -D ENABLE_WIFI_INTERFACE and +<helpers/wifi/*.cpp>. Companion WiFi is
now gated on that flag, so the env built silently without WiFi.
Today if a user builds firmware with an incorrect WiFi SSID or password,
they get a generic `WiFi disconnected.` error message.
Extend logging to descirbe the reason that WiFi was disconnected.
I gathered the updated logs for the 3 scenarios below.
```
WiFi: Attempting manual WiFi reconnect...
WiFi: WiFi disconnected (reason=ASSOC_LEAVE). Flagging for reconnect...
WiFi: WiFi disconnected (reason=4WAY_HANDSHAKE_TIMEOUT). Flagging for reconnect...
WiFi: WiFi disconnected (reason=4WAY_HANDSHAKE_TIMEOUT). Flagging for reconnect...
WiFi: WiFi disconnected (reason=4WAY_HANDSHAKE_TIMEOUT). Flagging for reconnect...
```
```
WiFi: WiFi disconnected (reason=NO_AP_FOUND). Flagging for reconnect...
```
```
WiFi: WiFi connected successfully!
WiFi: Got connection
```
The review asked for the pinned SNTP implementation's response checking to be
established rather than assumed. It is `SNTP_CHECK_RESPONSE = 0`: the lwIP
default in `lwip/src/include/lwip/apps/sntp_opts.h`, not overridden in this
build's ESP32 `lwipopts.h` or any sdkconfig — and lwIP ships precompiled in the
SDK, so no `-D` of ours can change it. At 0 it checks neither that the reply came
from the server it queried nor that the originate timestamp matches the request
it sent. Those are two of the checks that make the new validated probe
trustworthy, so SNTP was strictly weaker than the path it backed up.
It was reachable three ways, all of which set the system clock:
- the fallback inside `syncTimeWithNTP()`, which ran precisely when the
validated probe had failed — i.e. when interference is most likely;
- `refreshNTP()`, hourly, for the life of the node;
- `configTime()` after a *successful* validated sync, whose timezone side effect
was all that was wanted but which also left a background SNTP poller running
that would go on accepting unvalidated replies.
All three are gone. The periodic refresh runs the same validated probe as every
other sync, and the timezone is set directly with `setenv("TZ", "UTC0")` +
`tzset()`. Nothing is lost operationally: SNTP queried the same servers over the
same UDP/123 with a weaker parser. The RTC/system-clock fallback is a separate
decision and stays.
Review of the branch found one merge-blocking lifecycle hole and three
correctness gaps where the implementation stopped short of contracts the design
had already written down. All four are real; each was confirmed against the
source (two of them against hardware) before anything changed.
**P1 — a quarantined client could still produce a clean bridge stop.** The
cooperative teardown set `_teardown_complete` unconditionally, so a slot whose
`esp_mqtt_client_stop()` had not completed — deliberately skipped by
`destroySlotClients()` and marked Quarantined — still let the trampoline publish
the acknowledgement. The owner then freed the queue and buffers and allowed a
restart while that SDK task might still be running: exactly the ownership
ambiguity StopUnproven exists to remove. The ack is now withheld unless EVERY
client is proven stopped, so one unproven client leaves the whole bridge
unproven. The rule lives in MQTTClientState.h (`mqttStopMayBeAcknowledged`) with
host tests, alongside the state predicates moved out of the bridge.
**P2 — the F04 protection did not cover a client that was still connecting.**
`softDisconnect()` returns ESP_OK immediately when the client is not connected,
so for a slot mid-DNS/TLS/CONNECT it cancelled nothing: the attempt ran on and
its CONNECTED event arrived after the new configuration was applied, and with
callbacks registered once per client and esp-mqtt events carrying no generation,
nothing could tell it from the new attempt's. A reconfigure that lands on a
`Starting` client now stops it, joining its SDK task, before applying the new
configuration. A Connected client still takes the cheap softDisconnect path,
which is where the fragmentation argument applies. One helper
(`closeLiveClientForReconfigure`) so the two call sites cannot drift.
**P2 — a failed renewal bounce still advanced the effective expiry.** Minting
updates `token_expires_at` immediately and the renewal decision read it, so a
bounce that failed looked complete: the next pass saw a fresh future expiry and
never retried, and clearing `last_token_renewal` re-armed nothing. Slots now
carry `applied_token_expires_at` — the expiry of the credential the CONNECTION
is using — which only advances when a connect or reconnect has carried it. A
failed bounce leaves it on the old credential, so the renewal stays due.
**P2 — config-committed and start-accepted were conflated.** `connect()`
returned one result for both, so a start that failed after the configuration had
committed left `applied_config` describing the previous configuration, and the
next recreate-or-reuse decision could reuse a client whose trust policy was not
the one it believed. `applyConfig()` is now its own wrapper operation;
`applied_config` records the commit, activation records the start. The reconnect
ladder resets there too rather than in `teardownSlot()` — the old endpoint's
history still applies until a replacement configuration actually commits.
Two of my own bugs surfaced on hardware while testing this, both fixed here:
- `recreateSlotClient()` called the full `teardownSlot()`, which cleared
`broker_uri`, the just-minted token and both expiries out from under a
configuration that had already been decided, so a recreate handed the SDK an
empty URI and an empty token. It now stops the client and swaps the object,
touching nothing else, and the apply step refuses to configure a URI that
changed under it rather than passing it on.
- `stopSlotClient()` quarantined on any non-OK result, but `ESP_FAIL` from
`esp_mqtt_client_stop()` means "client is in invalid state", i.e. not started:
there was no task to join, the safest state there is. It was observed
quarantining healthy clients on hardware. The case that genuinely cannot be
proven is a stop that never RETURNS, which cannot surface here at all — it
hangs the task, which is what the bridge-level timeout contains.
Hardware (Heltec V4, 5 live slots): a reconfigure landing on a connecting client
logs `reconfigure during connect - stopping to cancel the attempt`; a broker
holding the CONNACK sees the client close the socket and the disabled slot never
connects; `wss`→`mqtt`→`wss` recreate cycles reconnect each way; a blackholed
endpoint recovers. 499/499 native tests, four envs clean.
The stale-configuration fix, and the reason the four preceding commits went
first.
`setupSlot()` applied configuration field by field, interleaved with the
decisions that produced it, and cleared stale fields only when
`slot.initial_connect_done` was set — which `teardownSlot()` clears at the top
of every reconfigure. So live reconfiguration always skipped the cleanup. On a
5-slot board this was reproducible in one command: moving a slot from a JWT
preset to an anonymous custom endpoint sent the previous configuration's
`v1_<pubkey>` username in the CONNECT to the new broker (95-byte CONNECT
captured at a throwaway broker on the LAN). The same config after a reboot sent
26 bytes with no username, which is what pinned it to the reconfigure path.
That cleanup could not have worked anyway: it nulled the wrapper's pointers, and
IDF's `esp_mqtt_set_if_config()` treats NULL as "leave unchanged", so an
SDK-held credential survives it. An empty string does overwrite it, and leaves
the CONNECT's username flag clear.
So configuration is now decided in full first, then applied in one place:
- every owned field is written on every apply — server, trust material,
username, password — with absent credentials written as "" rather than
omitted;
- `mqttConfigRecreateDecision()` (previous commit) picks reuse or recreation.
Credentials, auth-mode changes and endpoint moves within one scheme reuse the
client, because a create/destroy cycle on the reconnect and renewal paths is
the fork's documented internal-heap fragmentation driver. A transport
(scheme) change, a trust-policy or CA change, and buffer-capacity growth
recreate it, because those are the fields a write cannot safely replace;
- `recreateSlotClient()` will not destroy a client whose stop was not proven —
it quarantines it and fails the setup instead;
- the applied configuration is recorded only after the client actually starts.
Verified on hardware (Heltec V4, 5 live slots, both paths):
- reuse path — a custom JWT slot (639-byte CONNECT carrying `v1_<pubkey>` plus
the token) with its audience cleared now sends 26 bytes with no credentials,
with no client recreation;
- recreate path — `mqtt://` to `wss://` logged `recreating client
(transport-change)` and reconnected.
Also fixes a cap regression the hardware run caught: `canActivateSlot()` now
counts the positions held by the *other* slots. A slot being reconfigured keeps
its esp-mqtt task alive, so once the count included live clients it counted
itself out of its own position and a full board refused to reconfigure a slot —
disabling it instead. Asking "is there room for this slot" rather than "is
there room for one more" is also what makes a resource-based count safe.
`client->connected()` answers "is the network up", and it was being used for
"is there anything to stop". A client resolving DNS, negotiating TLS or waiting
after a failed CONNECT reports not-connected, so `teardownSlot()` skipped it:
selecting `none` cleared the bridge flags while the SDK task kept running, and
when the in-flight handshake completed the callback marked the disabled slot
connected and scheduled its status.
Each slot now carries the client's SDK lifecycle state (Absent, Configured,
Starting, Connected, Disconnected, Stopped, Quarantined) and a generation
counter for the logs. The bridge task owns that state, which makes it the
authority the callbacks consult:
- a CONNECTED event for a slot that is disabled, never started, or already
stopped is logged and dropped instead of marking the slot connected;
- teardown stops any *live* client, so a mid-handshake client can no longer
outlive its configuration.
Teardown also gains a reason, because "stop the task" and "close the transport"
are different needs:
- `Disable` (preset `none`, shutdown) stops the client — the stop is the point;
- `Reconfigure` closes the transport with `softDisconnect()` and keeps the
esp-mqtt task. Stopping it there would return its 6 KiB stack into the hole
the two 16 KiB mbedTLS record buffers just vacated, which is the fork's
documented internal-heap fragmentation driver — and per the soak campaign's
own conclusion, feeding `softDisconnect()` into the reconfigure path is the
fix for it, not serialisation. The campaign closed 2026-08-20, so the
reconfigure churn is no longer anyone's measurement lever.
A stop that does not complete now quarantines that client: its SDK task was
never joined, so it is never reused, never destroyed, and the token buffer its
config still points at is never freed. `get mqttN.diag` reports `quarantined`.
Two knock-ons this required:
- `setupSlot()` starts or reconnects according to the SDK state.
`esp_mqtt_client_start()` fails on an already-started client, and now that
its result is honoured, a reconfigure that kept the task would otherwise
leave the slot permanently unactivated.
- the active-slot cap counts resource holders as well as configured ones. It
keyed off `initial_connect_done`, which teardown clears, so a started or
quarantined client stopped counting against the cap and a board could
oversubscribe past the concurrent-TLS limit the cap exists to enforce.
`connect()`, `reconnect()`, `disconnect()`, `softDisconnect()` and `forceStop()`
all returned void, so every caller in the bridge treated "asked" as "done":
- `setupSlot()` marked a slot activated after `connect()` whatever happened. A
failed `esp_mqtt_client_start()` therefore consumed one of the scarce
active-slot positions, handed the slot to a reconnect ladder that is gated on
activation, and was never retried by the deferred-setup path.
- `reconnect()` explicitly proceeded after `esp_mqtt_set_config()` failed,
reconnecting on the previous configuration — the renewed token in the buffer,
the old one on the wire.
- the renewal path recorded the new expiry before the bounce succeeded, so a
failed bounce left the live session on the old credential with the next
renewal not due for a whole token lifetime.
- `softDisconnect()` logged its timeout and told its caller nothing.
- `disconnect()` waited for the DISCONNECTED event with no bound, on the very
task whose stop acknowledgement the shutdown waits for.
Now every one of them returns `esp_err_t`, a failed configuration transaction
aborts rather than starting or reconnecting on a half-updated config, and
`disconnect()`'s wait is bounded (it still stops the client, and reports
ESP_ERR_TIMEOUT when the event never arrived).
Bridge consequences:
- a failed start leaves the slot unactivated, so the existing deferred-setup
retry revisits it and it holds no active-slot position;
- a failed renewal bounce re-arms the renewal instead of recording it, so the
next maintenance pass retries;
- a reconnect that fails *locally* rolls back the backoff advance made for it.
The ladder and the breaker bound broker and network faults; an uninitialised
client or an uncommitted config transaction is neither, and inflating the
ladder for it was how a local fault could trip a breaker meant for a broker.
Non-PSRAM boards allocated 512-byte MQTT buffers for non-JWT slots, so the
desired buffer size was a per-slot variable while the allocated size was fixed
at `esp_mqtt_client_init()`: IDF 4.4 does not resize those buffers in
`esp_mqtt_set_config()`, and the wrapper's own reassembly buffer is allocated
once for the client's lifetime. A slot reconfigured from non-JWT to JWT
therefore kept 512-byte buffers, and a valid JWT CONNECT (frame plus a 768-byte
token) could not fit — with no error naming the cause. Stopping and starting the
same handle could not change the capacity either.
Give every client 896 bytes on every board. The cost is 384 bytes per client on
a non-PSRAM board, which caps at 2 active slots, against a TLS handshake that
needs 16 KB of contiguous internal DRAM. In exchange the capacity transition
stops existing, so the reconfigure path never needs to recreate a client to grow
its buffers.
The decision half of the client-configuration fix, as a pure header so every
transition is a host test rather than a hardware run.
`MqttEffectiveConfig` is the complete owned description of what a slot's client
should be configured with — URI (its own copy, because `slot.broker_uri` is
rewritten in place), transport, trust policy, auth mode, credential pointers,
buffer size, keepalive. Two things are deliberate:
- the transport is derived from the URI scheme, and the trust policy is
normalised against it: an unencrypted transport verifies nothing, whatever
certificate material was requested, so a plaintext endpoint can never look
like it retained a verified policy;
- absent credentials are represented as nullptr and written as "", never
omitted. IDF's `esp_mqtt_set_if_config()` treats NULL as "leave unchanged",
so a cleared wrapper pointer cannot erase an SDK-held credential, while an
empty string overwrites it and leaves the CONNECT's username flag clear
(verified on an ESP32-S3 against a local broker on IDF 4.4).
`mqttConfigRecreateDecision()` says whether the existing client can be
reconfigured in place. Reuse covers every credential and auth-mode change and
any endpoint move within one scheme — the cases that matter for reconnect and
token renewal, where a client create/destroy cycle is the fork's documented
internal-heap fragmentation driver. Recreation is reserved for the three fields
that cannot be overwritten safely: a transport (scheme) change, a trust-policy
or CA-certificate change, and growth beyond the allocated buffer capacity,
which IDF 4.4 fixes at client init.
Client certificates are excluded on purpose: `setClientCertificate()` has no
caller in the firmware, so modelling mutual TLS here would be untested
configuration surface. Noted in the header.
Nothing consumes this yet; the bridge is migrated onto it after the result
propagation and generation-guard steps, which it depends on.
The timeout fallback did the two things that cannot be done safely to a task
that may still be inside mbedTLS or holding esp-mqtt's API mutex: it deleted
that task from the loop task, then force-stopped and deleted the clients it
owned, then freed the queue and buffers it can reach. `esp_mqtt_client_stop()`
waits on the API mutex and the task's stopped event with no bound, so killing
its caller strands the next one; a second stop sees the run flag already clear
and returns failure without joining. Nothing in that path established
quiescence, and `canFlashAfterStop()` could only withhold OTA afterwards.
Changes, all of them about who owns what:
- `MQTTLifecycle` gains a `StopUnproven` state (appended, so existing values
keep their numbering). `StopTimedOut` now leads there and releases NOTHING;
it still fires `ota_release` so the OTA barrier aborts. `acceptsNewWork`,
`mayRestart` and `isStopInProgress` are false there, `mayTouchOwnedState` is
true, and `end()`'s wait loop still terminates.
- `begin()` refuses to start while a stop is unproven. A new start was never
proof that the previous clients stopped, and it used to clear the dirty latch
that withheld OTA. Because a start is now impossible from `StopUnproven`,
clearing that latch on an accepted start is sound.
- A late acknowledgement is honoured. The task publishes it only after tearing
its clients down, so it proves the same thing whenever it arrives; the
deadline is an availability policy, not a statement about what the ack means.
`pollLateStopAck()` releases the withheld resources and makes the bridge
restartable — a slow stop (a blackholed WSS broker can hold the SDK well past
the budget) no longer costs a reboot.
- The handshake flags become `std::atomic<bool>` with release/acquire, and the
ack moves into the task trampoline immediately before `vTaskDelete(nullptr)`,
gated on a `_teardown_complete` flag the loop sets. Published from inside the
loop it proved teardown returned, not that the task had stopped executing —
and `volatile` ordered none of it. This is the soak campaign's blocker #20,
the one item on its list that can corrupt memory.
- The force-stop paths are gone with their only caller: `destroySlotClients()`
and `teardownSlot()` no longer take a `force` flag.
- `get mqtt.status` reports "previous stop unproven, reboot to recover" instead
of a bare "not running", latched so it survives end() clearing the singleton.
Two lifecycle tests asserted the old contract and are re-encoded, not deleted:
a timed-out stop is no longer `Stopped`-and-released, and "repeated failed
stops leave the bridge restartable" becomes "repeated *slow* stops recover on
their late ack" plus a new case pinning that a never-acknowledged stop stays
unusable for the boot. That availability trade is the point of the change.
The applied mode had no observable trace on a running node, which is how the
CLI and the reconnect path could disagree about what `min` means for as long as
they did. Log it at the one place that applies it, reading the value back with
esp_wifi_get_ps() rather than printing what was requested.
Used to confirm F11 on hardware: with `min` stored, a Heltec V4 now reports
"WiFi power save: min (mode=1)" on association, where the old mapping applied
WIFI_PS_NONE (mode=0).
NTPClient::forceUpdate() treated any non-empty datagram arriving on its fixed
local port 1337 as a time response: it ignored the read length, the version,
the mode, the stratum, the leap indicator and any request/response correlation,
then handed the bytes at offset 40 to the bridge. The bridge only checked that
the derived epoch was at least 2026-01-01 before calling settimeofday() and
writing the RTC. A host probe against the installed library source accepted a
one-byte non-NTP datagram and produced epoch 2085978496. Anyone able to land a
UDP datagram during a query window could set a bogus clock, which then feeds JWT
issuance, certificate validity and packet timestamps.
Replace that path with probeNtpServer(): one exchange per server on a fresh
ephemeral socket, closed again on every exit. A reply is accepted only if it is
a full 48 bytes, from the address and port queried, NTPv3/v4 mode 4, from a
synchronised server (no leap alarm, stratum 1-15), echoing the random transmit
timestamp of the request, with an epoch inside a plausible range. Rejected
datagrams leave the clock alone and do not end the wait, so an early bogus
packet cannot pre-empt the real answer. Era-1 (post-2036) timestamps convert
forward instead of wrapping into 1900.
The diagnostic now runs the same validated probe, so `get mqtt.ntp.diag`
answers the question it is asked — would this server be trusted? — instead of
reporting a datagram nothing checked. It also no longer leaves its UDP socket
open after the probe, and each server's per-probe result carries the reason it
failed ("DNS failed", "unsolicited reply", "server unsynced", ...). The
diagnostic still blocks its caller; making it asynchronous is a separate change.
The acceptance rules live in NtpValidation.h (pure, host-tested), including the
review's one-byte-datagram reproduction.
Only the periodic status path consulted the setting. The connect callback armed
a status publish unconditionally and publishStatusToSlot() never checked it, so
every boot and every reconnect published a status message — including the
metadata an operator turned the setting off to suppress. The documented contract
is "Enable/disable status messages".
Read the toggle live from prefs at publish time, matching the periodic path, and
also skip a slot the operator has disabled: teardown only stops a client that
reports connected, so a slot switched off mid-connect can still complete its
handshake and arm this publish.
Four of the hand-written error labels named the wrong failure against the
installed SDK: Wi-Fi reason 201 is NO_AP_FOUND (reported as "security
mismatch"), 202 is AUTH_FAIL (reported as "auth mode rejected"), 39 is TIMEOUT
(reported as "SSID not found") and 34 is MISSING_ACKS (reported as an
"AP state mismatch"). esp-tls 0x8008 is TCP_CLOSED_FIN, not a timeout, and
0x8010 is CERT_PARTLY_OK, not a generic mbedTLS error. 0x800B, labelled
"cert verify failed", is not an esp-tls error at all. So the one line an
operator reads to find out why a node is offline could name the wrong cause.
Move the tables into MQTTErrorLabels.h (pure, host-tested) with every value
static_asserted against the SDK enum/#define actually being compiled, so a
framework bump fails the build instead of relabelling errors in the field.
Codes the SDK does not define — including 61/88/168, which had labels no
header supports — now return no label and the caller prints the raw number.
Also keep the CONNACK return code from a broker refusal and show it in
`get mqttN.diag` ("refused: bad user/password (4)"). A refusal is not a
transport failure, so the TLS/socket fields are empty and a slot with wrong
credentials previously showed no useful detail at all.
`set wifi.powersave min` stored 0 and applied WIFI_PS_MIN_MODEM, but the
bridge's post-reconnect mapping read the same 0 as WIFI_PS_NONE. A node
explicitly configured for minimum modem sleep therefore ran with power save off
after its first reconnect, while `get wifi.powersave` still reported min. The
stored default is already 1 (`none`), so nothing here changes the product
default — it stops an operator's explicit choice from being reinterpreted.
Move the value/name/mode table into WifiPowerSavePolicy (pure, host-tested) and
use it from the CLI setter and getter, the web config snapshot and the bridge.
The bridge now also applies the mode when it finds the STA already associated at
start — that path has no connect transition to carry the setting, so a bridge
restart used to leave whatever mode was set before.
The watchdog took max(last_recv, last_irq, last_tx) of three raw millis()
timestamps. Around the 32-bit wrap a transmit stamped in the upper half of the
range is numerically larger than every fresh post-wrap receive, so it kept
winning for up to ~24 days: the watchdog measured silence from that stale
transmit, concluded the radio had gone deaf, and reset a radio that was
receiving normally — once per watchdog interval, until the old value stopped
winning.
Extract the decision into RadioWatchdog (pure, host-testable). It compares each
timestamp only against its own previous value, so any change is fresh activity,
and stamps it against a monotonic 64-bit clock extended from millis(). Evidence
older than one 32-bit cycle now stays old instead of aliasing back to "recent",
which the previous unsigned-subtraction age could not do either.
Covered in test/test_radio_watchdog: pre-wrap TX with continuous post-wrap RX,
genuine silence across the wrap, silence past a full millis() cycle, no activity
since boot, disabled watchdog, TX-only traffic, and a stalled mesh loop.
Payloads of 166 or 167 bytes previously passed the frame-sized bound
(MAX_CHANNEL_DATA_LENGTH = 167) but were rejected by sendGroupData
against MAX_GROUP_DATA_LENGTH (165), causing ERR_CODE_TABLE_FULL (retry
later) to be returned instead of ERR_CODE_ILLEGAL_ARG.
Fixes#3345
agessaman