Migrate to aiomqtt (rebased)#457
Merged
Merged
Conversation
Update test stubs and publish-behavior tests to match the new abstract method signatures (positional retain=False, qos=0) and rewrite the publisher tests to use AsyncMock against __async_publish instead of patching the synchronous gmqtt client.publish.
- Re-add keyword-only * separator before retain and qos params so callers cannot accidentally pass them positionally - Restore retain=True as the default; flipping to False would silently stop retaining messages for all existing callers - Fix __publish private helper which was also missing the True default - Update tests to match corrected defaults and use keyword args
publish_str/int/bool/float were silently dropping the retain parameter, causing the debug log to always show retain=True regardless of what the caller passed.
_properties.get("retain", 0) called message.properties which is
paho.mqtt.properties.Properties|None, not a dict. On MQTT v3.1.1
(common default) this is None, raising AttributeError on every
incoming message and silently dropping all /set commands.
Change _on_message to accept retained: bool directly, and pass
message.retain (a bool field on aiomqtt.Message) at the call site.
The aiomqtt migration dropped the on_mqtt_reconnected() call that the old gmqtt __on_connect callback made. MqttGateway.on_mqtt_reconnected() resets HomeAssistantGatewayDiscovery and calls reset_ha_discovery() on every vehicle handler, so without this call HA entities would not be re-announced after a broker disconnect/reconnect cycle.
The aiomqtt migration dropped the imported_energy_topic subscription block from __enable_commands. vin_by_imported_energy_topic was initialised in __init__ but never populated, making __handle_imported_energy dead code and silently breaking OpenWB energy-import tracking.
LOG.error("...{e}") was a plain string, always logging the literal
"{e}" instead of the exception value, making subscribe failures
impossible to diagnose from logs.
When self.host is empty __run_loop returned immediately without setting __connected, leaving connect() blocked forever on await self.__connected.wait(). Move the empty-host check into connect() so it returns before creating the task.
test_clear_topic_publishes_none_not_retained asserted retain=True, contradicting its own name. Clearing a retained MQTT topic requires publishing a null payload with retain=True; rename to match.
The suffix was a debug leftover from the initial aiomqtt migration. There is only one MqttPublisher instance so no client ID conflict exists.
Bad credentials, invalid client ID, and auth refusals (MQTT 3.1.1 rc 1, 2, 4, 5) are permanent — retrying every 5 seconds forever gives no useful signal. Raise SystemExit on these, matching the old gmqtt behaviour. rc 3 (server unavailable) is transient and still reconnects.
…1 spec constants Avoids depending on paho-mqtt (a transitive dep) for named CONNACK refusal codes. Values are stable — they are defined by the MQTT 3.1.1 specification (section 3.2.2.3) and will not change.
Fixed 5-second interval hammered a DNS lookup + TCP attempt every 5 s indefinitely against a down broker. Now doubles on each failure (5 s → 10 → 20 → … → 300 s) and resets to the minimum on a successful connection.
run_coroutine_threadsafe is the cross-thread API — calling it from the same event loop thread allocates an unnecessary concurrent.futures.Future and discards it, silently swallowing any unexpected exceptions. Replace with loop.create_task() which is the correct same-loop primitive and whose unhandled exceptions are surfaced by the asyncio runtime.
Closed
Two bugs in the permanent-refusal handling: 1. isinstance(e.rc, int) is always False — aiomqtt passes a ReasonCode object, not a plain int. ReasonCode.__eq__ supports int comparison, so the guard is redundant and was silently making the entire branch dead code. Remove the isinstance check. 2. raise SystemExit inside an asyncio Task is caught by the event loop and stored on the task, never propagated. connect() was left hanging on await __connected.wait() forever. Fix: store the error and set __connected so connect() unblocks, then re-raise there.
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.
No description provided.