feat(mqtt): structured error reporting via onError - #27
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Mark changelog v0.8.0 section as -dev, matching the release convention (release commit renames it, bumps library.json/idf_component.yml). - Fix onError doc heading level (#### -> ###) so it renders as a sibling section instead of nested under Publishing from a second task. - Document that onError delivery only happens while Client is in TransportsConnecting or Connected (the only states from which Client calls loop()); errors queued outside those states are retained, not lost, and delivered on the next loop(). Added to both api.md and the header doc comment. - Bound the error drain in MqttTransport::loop() to ERROR_QUEUE_DEPTH iterations and hoist the callback into a local before invoking it, so a callback that calls begin() can't feed the same drain pass forever, and a callback that re-registers onError() doesn't assign to the std::function while its target is running. - Mirror the "don't re-register from inside the callback" and "disconnect() doesn't drain the queue" caveats into the header, where the rest of the onError contract already lives. - Document IDF >= 5.0's fourth error type (MQTT_ERROR_TYPE_SUBSCRIBE_FAILED) in api.md; describe() keeps falling through default: for it since the constant doesn't exist on IDF 4.4, but the default string is now more specific. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The first-pass fix gated the drain loop on `if (cb)`, which meant an unregistered onError left the queue never popped. With ERROR_QUEUE_DEPTH at 4, a device on a flaky broker would fill the queue and then hit the "error queue full, dropping error report" log path on every subsequent MQTT_EVENT_ERROR — log spam on upgrade for every user who doesn't register the hook. Move the null check to guard only the invocation, not the pop, restoring the pre-fix drain-and-discard behavior while keeping the hoisted-callback and bounded-iteration properties. Also strengthens test_error_without_registered_callback_is_safe to prove the queue actually drains: after the no-callback error, it registers a callback, fires one more error, and asserts exactly one delivery (not a backlog of two) — which would fail if the first error had been left sitting in the queue. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Surfaces ESP-IDF's structured MQTT error detail through a new
MqttTransport::onErrorhook.MQTT_EVENT_ERRORpreviously logged the string"MQTT error"and discardedevent->error_handleentirely, so a broker authorization refusal was indistinguishable from a TLS failure or a socket error. In practice a refused device was misdiagnosed as having a network fault: it burned the WiFi reconnect ladder, reported("RECONNECT", "max attempts exceeded"), and parked in terminalConnectionFailedneeding a physical reboot.What's added
MqttTransport::ErrorInfo—type,connectReturnCode, TLS/socket fields, plusisConnectionRefused(),isNotAuthorized()(CONNACK 5) anddescribe().MqttTransport::onError(cb)— populated by the IDF event task onto a depth-4SpscQueue, drained last inloop()holding no lock, so the callback may re-entrantly calldisconnect()/begin().Reporting only: Courier keeps retrying, and all recovery policy stays in the application.
docs/api.mdcarries the rescue recipe, including the trap that the callback must not block (it runs insideClient::loop()).Scope
src/Transport.h,src/Courier.h,src/Courier.cppand both WebSocket files are untouched — the reconnection ladder is deliberately unchanged.WebSocketTransporthas the same defect and is not fixed here:esp_websocket_event_data_tcarries noerror_handlebeforeesp_websocket_client1.x on ESP-IDF 5, and the PlatformIO/Arduino path resolves to ESP-IDF 4.4 — a hook there would be empty for most consumers. It follows when the Arduino floor moves to arduino-esp32 3.x.Compatibility
Additive; no breaking changes. Both ESP-IDF majors supported:
MQTT_ERROR_TYPE_SUBSCRIBE_FAILEDis never named in production code (absent on 4.4) and the IDF error-codes struct type is reached viaauto*off the null-checkederror_handlepointer.Testing
198/198 unit tests, cppcheck clean. Host mocks model neither real IDF, so the
build-platformio(4.4) andbuild-espidf(5.1.4) CI jobs are the portability evidence for this change.🤖 Generated with Claude Code