feat: MQTT binary publish and topic-scoped binary receive - #26
Merged
Merged
Conversation
esp_mqtt_client_publish treats a zero length as "call strlen(data)", and every MqttTransport::publish overload passed 0. Payloads containing NUL bytes were truncated at the first zero; a buffer with no terminator at all was read past the end of its allocation and published onto a topic. Adds publishBinary() with an explicit length, which rejects nullptr, len == 0 and len > INT_MAX rather than arranging for IDF's happy path — an intentionally empty payload is publish(topic, "", qos, retain). Adds the receive side now rather than later: MQTT 3.1.1 carries no content-type, so subscribeBinary() declares which filters are opaque, and those payloads reach onBinary(topic, data, len) only — never onMessage, never Client's JSON lane. Retrofitting a receive hook onto a released transport would be breaking; adding it before the release is free. The lane is chosen when loop() drains, on the task that owns the subscription list, so the ESP-IDF event task does no per-message classification. Publishing from a second task (a dedicated realtime sender) is now safe. esp-mqtt already serialises its own API calls, but esp_mqtt_client_destroy takes no lock and frees the handle, so a reconnect escalation could free the client under an in-flight publish. Lock.h adds the two mutexes for that, with the deadlock rule against IDF's own API lock written down. Also: subscriptions are re-subscribed at their registered QoS after a reconnect (subscribeAll used QoS 0), and out_buffer_size / network_timeout_ms are exposed, both defaulting to the IDF values. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Prompted by a request from the hawthorn MQTT broker hardening work, which wants voice audio to ride the MQTT connection the device already holds so firmware can drop its second WebSocket (and with it a permanent TLS session).
The bug this starts from
esp_mqtt_client_publishdoesif (len <= 0 && data != NULL) len = strlen(data), and everyMqttTransport::publishoverload passed0. So:publishBinary(topic, data, len, qos, retain)passes a real length and rejectsnullptr,len == 0andlen > INT_MAXrather than arranging for IDF's happy path to be taken. A binary caller passing zero length is a bug, and silently emitting an empty publish would hide it at the one place best positioned to catch it. An intentionally empty payload is a text publish:publish(topic, "", qos, retain).Binary receive, now rather than later
MQTT 3.1.1 has no content-type on the wire, so the subscriber declares the lane:
Payloads on a binary-declared filter reach
onBinaryonly — neveronMessage, neverClient's JSON lane, so nothingdeserializeJsons them once MQTT is the default transport.Config::binaryTopicsdeclares the same at construction.The justification for building this before there is a caller is narrow: retrofitting a receive hook onto a released transport is a breaking change, whereas adding it before the release is free. That argument covers the seam, not the capacity — see Known limits.
Two things this needed that aren't obvious:
subscribeBinaryregisters a filter but the broker delivers a concrete topic, so an exact-string lookup never matches and the frame falls back into the JSON lane — precisely the failure the opt-in exists to prevent. HenceMqttTransport::topicMatches, public because callers dispatching insideonMessageneed the same rule.loop()drain time, on the task that owns the subscription list — not on the ESP-IDF event task, which would have meant a cross-task read of astd::vectorper inbound message.Publishing from a second task
esp-mqtt is thread-safe for concurrent API calls:
publish,subscribe_multiple,unsubscribe,startandstopeach take an internalMQTT_API_LOCK. What it does not protect is the handle's lifetime —esp_mqtt_client_destroytakes no lock and frees the transport list, the outbox, the buffers, the API lock itself, and then the client. It structurally can't be lock-protected.That is courier's actual gap:
publish()reads_clientand enters IDF while a reconnect escalation on the app task frees it.Lock.hadds two mutexes for that, with the deadlock rule against IDF's own API lock written down in the header._clientLockis taken with a 250 ms bound by the publish path (returnsfalserather than waiting out a teardown, whichstop()can stretch across a TLS connect) and unbounded bybegin/disconnect/suspend/resume._topicsLockcovers_topicsonly and is read by the IDF task once per connect.It is not a congestion signal. A lone publisher entering a busy client still waits on IDF's unbounded lock; no Courier-side lock can change that.
Config::network_timeout_msis the knob for that worst case.Also
subscribe(topic, qos)applied the QoS on the initial call, butsubscribeAll()re-subscribed everything at QoS 0.Config::out_buffer_sizeandConfig::network_timeout_ms, both defaulting to0= leave the IDF value. A 1920-byte frame against the default 1024 buffer is two writes and two TLS records; sizing past the frame makes it one.Known limits
The MQTT inbound path is a depth-8 SPSC queue with a heap-allocated topic per message, drained on the app task at
loop()cadence.onBinaryis the right shape for control-rate blobs. It is not sized for a sustained stream — a 16.7 fps audio downlink overflows it within half a second of a slowloop(). Documented indocs/api.mdas a known boundary; a TTS-to-device downlink would be separate work on the inbound path, probably dispatching on the MQTT task rather than through the app-task queue.Testing
./tools/run-tests.py unit— 181 pass. 18 new, including: embedded-NUL round trip asserted on.size()and never as a C string; the three rejection cases; binary-lane routing; wildcard routing; client-hook bypass; multi-chunk reassembly; survival across reconnect; and three threaded tests that park a second task inside a mocked publish to check the bounded lock refuses rather than blocks../tools/run-tests.py static-analysis— clean../tools/run-tests.py build— the two Arduino-framework examples compileMqttTransport.cppand pass. The threearduino, espidfexamples could not run locally: PlatformIO's bundledtool-ninjais an x86_64 binary and the dev machine is arm64 (bad CPU type in executable). Pre-existing and unrelated to this change, but it means the ESP-IDF-component builds are unverified outside CI.The device-side locking is argued from esp-mqtt's source and exercised by host tests; it has not run on hardware with a real send task racing a reconnect. Worth watching in first bring-up.
🤖 Generated with Claude Code