Mqtt auto connect - #26
Merged
Merged
Conversation
Three real defects from review. Everything else it turned up -- stale doc wording, a dead compile definition, include-scope tidying, deduplicating the two capture handlers, replacing the mocks' sleep with a port probe -- is left alone: none of it affects a running device. link_dead was set for every non-success MQTT_ProcessLoop status, so the clean DISCONNECT was skipped on links that were still alive. MQTTBadResponse (a malformed packet arrived) and MQTTIllegalState (QoS bookkeeping hit an impossible state) are protocol faults on a healthy socket; suppressing the DISCONNECT there strands the session on the broker until the 60 s keepalive expires, and since the clientId is the devid, the next reconnect races that stale session. Only MQTTRecvFailed / MQTTSendFailed / MQTTKeepAliveTimeout now qualify, and a completed exchange clears the flag again. That direction matters as much: the flag is read at teardown, possibly long after the failure, so latch-only would let one transient error the caller retried past suppress the DISCONNECT for the rest of a demonstrably live session -- the same race from the other end. All four coreMQTT call sites report their status, not just the process loop. Publish was missing and matters most: every DP report goes through it, so it is the likeliest place an app discovers a dropped link, and the teardown that follows would push a DISCONNECT into a socket that cannot carry it. The subscribe path matters for the same reason -- a link lost during the 5 s SUBACK wait is torn down immediately after. The log-capture handlers read past their stack buffer: vsnprintf returns the length it WOULD have written, so any routed line longer than the local buffer made memcpy copy beyond it. That corrupts the capture with unrelated stack and can fake or mask the substring under assertion, and it was reachable -- one coreHTTP parse error interpolates up to a whole response buffer. Clamped in both handlers, in place; deduplicating them is cleanup, not a fix. test_connack_reason_is_logged asserted on the bare string "MQTTServerRefused", which coreMQTT's own routed line already contains, so it passed even with mqtt.c reverted to printing a raw "%d" -- it protected nothing. It now matches the SDK's own message; reverting that line drops iot_mqtt_test to 15/16. The CHANGELOG entry for the DISCONNECT suppression is amended rather than supplemented: it describes the un-narrowed behaviour and has not shipped, so a second entry would have left two conflicting accounts in one release. Not covered by tests, stated rather than implied: the DISCONNECT suppression itself (a just-killed peer usually absorbs one more send, so the failing send does not reproduce against a mock) and the flag-clearing path (it needs a transient failure followed by a success the mocks cannot stage). link_dead is private to mqtt.c, so a test cannot read it directly either. Verified: full build clean; ctest --timeout 180 13/13; iot_mqtt_test 16/16, iot_atop_call_test 14/14; run_leaks_check.sh reports 0 leaks for both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…by default BREAKING CHANGE: mqtt_auto_connect is replaced by mqtt_disable_auto_connect on both iot_client_config_t and iot_on_boarding_config_t. Bringing the MQTT link up was not something an app could do. The only entry point was iot_client_message_connect(), declared in src/iot_client_message.h -- a header that is not installed. iot_client.h's own config comment told callers to invoke exactly that function, and four examples reached into the private header to get it, which compiled only because they build from inside the repo. An app built against the installed headers could neither connect manually nor re-establish a dropped link. Two changes together, because either alone leaves the API awkward: - iot_client_connect() / iot_client_disconnect() are now public, thin wrappers over the message layer in the same shape as the existing iot_client_process() / iot_client_publish() pair. A reconnect loop needs them no matter what the default is -- that is why publishing them is not made redundant by the second change. - Auto-connect is on by default. The rename is what makes that expressible at all: a zero-initialized struct gives a bool false, so a field named mqtt_auto_connect can only ever default to off. Reversing the sense follows mqtt_disable_tls, which already reads "false (default) = TLS on" in the same two structs. Renaming rather than flipping also means existing code breaks at compile time instead of silently changing behaviour: `.mqtt_auto_connect = false` becomes `.mqtt_disable_auto_connect = true`, and `.mqtt_auto_connect = true` can simply be deleted. All eight in-tree call sites are updated. The failure path is deliberately unchanged: a failed auto-connect still releases the client and returns NULL from iot_client_init(). That now applies by default, so a device whose network may not be up at boot -- or one that only speaks ATOP over HTTP with no broker at all, like examples/posix/ota-demo -- must set mqtt_disable_auto_connect and drive the link itself, or it will fail to initialize rather than coming up and retrying. The reference doc says so where it documents init. Publishing connect also exposed a latent leak, fixed here: connecting an already-connected client built a second link and orphaned the first. iot_client_message_try_connect() assigns client->mqtt unconditionally, so the previous mqtt client, its packet buffer and its open socket were left with nothing pointing at them -- reachable precisely because this API is now public and now connects by default, so an app calling it on a live client is the expected case rather than a mistake. A second call is success without rebuilding; disconnect first to force a fresh link. The guard sits in the private entry point so every path is covered, including init's auto-connect. Also correct a promise while making it public: both headers claimed a TLS handshake failure "refreshes the MQTT CA certificate and retries once". No such code exists -- iot_client_message_try_connect() destroys the client and returns OPRT_TLS_HANDSHAKE_FAILED. Cert recovery is the app's job (iot_get_ca_certificate() + reassign client->cacert); a reconnect loop written against the old wording retries the same doomed handshake forever after a broker cert rotation. The four examples (dp-management, ota-confirm, unbind-demo, ai/rtc-tcp-client) now use the public API and no longer include the private header, so they compile the way an external consumer would. Tests: test_public_connect_wrappers_forward pins the forwarding on a verdict only the callee reaches (the missing mqtt_url/devid rejection) -- a NULL check alone would pass against an empty stub. test_connect_twice_keeps_one_link pins the handle across a second connect; reverting that guard drops iot_message_test to 17/18. The new default is NOT covered, and that is worth stating plainly for something every caller inherits: both auto-connect tests use an empty devid, so mqtt_url stays empty and the branch short-circuits before the flag is read. Inverting the predicate leaves iot_message_test at 18/18 -- checked. Proving it needs a DNS mock and a broker mock in one suite, which none has today. Verified: full build clean; ctest --timeout 180 13/13; iot_message_test 18/18 (2 new); the posix examples that configure this field build against the new API. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Merged
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.
mqtt_auto_connectis replaced bymqtt_disable_auto_connecton bothiot_client_config_tandiot_on_boarding_config_t.Existing code breaks at compile time rather than silently changing
behaviour. That is the reason for renaming instead of flipping the value.
Why
Bringing the MQTT link up was not something an app could do. The only entry
point was
iot_client_message_connect(), declared insrc/iot_client_message.h— a header that is not installed. Meanwhileiot_client.h's own config comment told callers to invoke exactly thatfunction, and four examples reached into the private header to get it, compiling
only because they build from inside the repo.
Two changes, because either alone leaves the API awkward
iot_client_connect()/iot_client_disconnect()are now public — thinwrappers over the message layer, same shape as the existing
iot_client_process()/iot_client_publish()pair. A reconnect loop needs themregardless of the default, which is why publishing them is not made redundant by
the second change.
Auto-connect is on by default. The rename is what makes that expressible at
all: a zero-initialized struct gives a
boolfalse, so a field namedmqtt_auto_connectcan only ever default to off. Reversing the sense followsmqtt_disable_tls, which already reads "false (default) = TLS on" in the sametwo structs.
The failure path is unchanged — read this if your device boots offline
A failed auto-connect still releases the client and returns
NULLfromiot_client_init(). That now applies by default. A device whose network maynot be up at boot, or one that only speaks ATOP over HTTP with no broker at all
(
examples/posix/ota-demo), must setmqtt_disable_auto_connectand drive thelink itself — otherwise it fails to initialize instead of coming up and
retrying.
A latent leak, fixed here
Connecting an already-connected client built a second link and orphaned the
first:
iot_client_message_try_connect()assignsclient->mqttunconditionally, so the previous mqtt client, its packet buffer and its open
socket were left with nothing pointing at them. Reachable precisely because this
API is now public and connects by default — an app calling it on a live client
is the expected case, not a mistake. A second call is now success without
rebuilding; disconnect first to force a fresh link. The guard sits in the private
entry point so every path is covered, including init's auto-connect.
A false promise removed
Both headers claimed a TLS handshake failure "refreshes the MQTT CA certificate
and retries once". No such code exists —
try_connect()destroys the client andreturns
OPRT_TLS_HANDSHAKE_FAILED. Cert recovery is the app's job(
iot_get_ca_certificate()+ reassignclient->cacert); a reconnect loopwritten against the old wording retries the same doomed handshake forever after a
broker cert rotation.
Verification
ctest --timeout 18013/13;iot_message_test18/18 (2 new)test_connect_twice_keeps_one_linkchecked to bite: reverting the guard dropsiot_message_testto 17/18The new default has no test coverage
Worth stating plainly for something every caller inherits: both auto-connect
tests use an empty devid, so
mqtt_urlstays empty and the branchshort-circuits before the flag is read. I inverted the predicate and
iot_message_teststill passed 18/18. Proving the default would need a DNS mockand a broker mock in one suite, which none has today.