feat(mqtt): add opt-in Home Assistant MQTT discovery - #5543
Conversation
✅ Deploy Preview for teslamate ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
This has been irritating me for a while. Was created by AI. Not tested or reviewed yet :-) Not tried to add new config to nix module yet either. Don't want to conflict with #5337. But nix config might be required in order for me to test it properly. |
|
Opt-in seems to be sensible, to not auto-break existing configurations. |
|
Nice addition!
I fixed a small issue, successfully tested in production with UNIX socket connection, and merged it you can rebase to main.
I can review in the coming days. |
JakobLichterfeld
left a comment
There was a problem hiding this comment.
Thanks Brian, this is a long-requested feature and the implementation looks solid — opt-in by default, the entity definitions mirror our documented manual setup nicely, and the test coverage is great. 👍
A few things I'd like to see addressed before merging:
-
HomeAssistant.clear/3is never called outside its own test. Its docstring promises entities are removed "when discovery is disabled or a vehicle is removed", but nothing wires it up — users who turn discovery off will keep the retained config payloads (and thus the entities in HA) forever. Either hook it up (e.g. clear once on init when discovery is disabled) or document how to clean up manually. -
Migration from the manual YAML config: the generated
unique_ids (teslamate_1_battery_level, …) are identical to the ones in our documentedmqtt_sensors.yaml, so running both at the same time will produce duplicate-ID errors in HA. Also, the docs claim "the entity IDs match those produced by the manual mqtt_sensors.yaml", but withobject_id: "speed"HA should generatesensor.speed, while the manual config usessensor.tesla_speed— that would break existing dashboards/automations on migration. Could you verify the resulting entity IDs, and add a note to the docs that the manual configuration must be removed first? -
Markdown table in
environment_variables.mdgot mangled: the separator row gained an extra| --- |column and theMQTT_HOME_ASSISTANT_DISCOVERY_PREFIXrow has a trailing empty cell — this won't render correctly.
Smaller points, non-blocking:
healthyis in@do_not_retain, so the discovered binary sensor staysunknownafter an HA restart until the value changes. It could also serve as anavailabilitytopic for all entities — fine as a follow-up.MQTT_NAMESPACEis validated viavalidate_namespace!, the newMQTT_HOME_ASSISTANT_DISCOVERY_PREFIXisn't — an empty value produces topics with a leading/.charge_energy_addedusesstate_class: total; since the value resets per charging session, please double-check whethertotal_increasingis the better fit for the HA energy dashboard.- The env var docs say
MQTT_HOME_ASSISTANT_DISCOVERYshould be "set together withMQTT_HOME_ASSISTANT_DISCOVERY_URL", but the URL is optional — maybe reword. home_assistant_test.exsusesTeslaMate.DataCasebut never touches the database — plainExUnit.Casewould do.
🤖 Review drafted with Claude Code (Fable 5 high) — sponsored by Claude for Open Source
|
One more thing I noticed while cross-checking against the full topic list in The entity list mirrors the manual
( Since the discovery configs are retained and published per entity, extending coverage later is cheap — so apart from 🤖 Review drafted with Claude Code (Fable 5 high) — sponsored by Claude for Open Source |
|
Thanks! Will have a look at this ASAP. The one remaining thing I can think of right now will be updating the nix module. Probably add the config values, but give them good default values so they can be optional. |
|
I intend to look into this as my next priority. Right after I fix the mess I created in my personal project ;-) |
|
Rebased against latest main branch; one conflict resolve. |
|
I plan to address each concern in a separate commit. Hopefully this will make it easier to review. Changes can be squashed before merging. AI summary of the concerns, turned into TODO items for easy tracking. I hope I didn't miss anything (will edit as required):
|
Fix the header separator row, which gained an extra column, and the MQTT_HOME_ASSISTANT_DISCOVERY_PREFIX row, which had a trailing empty cell. Addresses review comment on #5543.
MQTT_HOME_ASSISTANT_DISCOVERY_URL is optional; clarify that setting MQTT_HOME_ASSISTANT_DISCOVERY does not require it. Addresses review comment on #5543.
Validate the discovery prefix like MQTT_NAMESPACE: empty values fall back to the default so topics never get a leading '/', and MQTT wildcards are rejected. Addresses review comment on #5543.
MQTT_HOME_ASSISTANT_DISCOVERY_URL is optional; clarify that setting MQTT_HOME_ASSISTANT_DISCOVERY does not require it. Addresses review comment on #5543.
Validate the discovery prefix like MQTT_NAMESPACE: empty values fall back to the default so topics never get a leading '/', and MQTT wildcards are rejected. Addresses review comment on #5543.
Call HomeAssistant.clear/3 so retained discovery configs (and the entities in Home Assistant) are removed when discovery is disabled. Addresses review comment on #5543.
|
Calling clear when vehicle is removed might be tricky. Wondering if it is worth it. AI says:
That first solution feels kind of yuck, but haven't thought about it in great detail yet. |
|
Presumably what we need is to process every vehicle in Might require some restructuring of code. Opinions appreciated :-) |
|
Thanks for your continued work on this! I'd advise against diffing against I'd frame the invariant differently: discovery configs should exist exactly for the cars TeslaMate publishes state topics for — and that set is simply the running vehicle processes, i.e. Concretely: in One related thing I noticed while looking at this: the discovery topics ( 🤖 Review drafted with Claude Code (Fable 5 high) — sponsored by Claude for Open Source |
Prefix the discovery object_id with tesla_ so Home Assistant generates the same entity IDs as the documented manual configuration (e.g. sensor.tesla_speed instead of sensor.speed), avoiding broken dashboards and automations on migration. Drop the _km suffix from the battery range sensors to match the manual unique_ids. Document that the manual mqtt_sensors.yaml must be removed before enabling discovery to avoid duplicate unique_id errors. Addresses review comment on #5543.
|
Hopefully I have addressed most of the concerns now. As per #5543 (comment) there are additional topics I can add if required. I added charging_state only for now. I see a test failure - which I suspect is not related to these changes, and a dialyzer failure. Will investigate. |
|
Now fixed the nix module too. |
There was a problem hiding this comment.
Impressive turnaround, thanks Brian! I went through all nine new commits — the startup cleanup is exactly the approach we discussed, the entity ID alignment checks out against the manual YAML (I verified the unique_ids match, including the dropped _km suffixes), the prefix validation mirrors MQTT_NAMESPACE (empty falls back to the default, wildcards raise at boot), the namespace scoping is correctly applied to node, unique_id, and device identifier, and charging_state (nice touch adding the manual YAML equivalent too), the nix module (defaults make the options properly optional), and the doc updates all look good.
One real issue, from the interplay of two of the new commits: the startup cleanup in PubSub.clear_removed_vehicles/2 passes only Keyword.take(opts, [:discovery_prefix]) to HomeAssistant.clear/3 — it predates the namespace-scoping commit and wasn't updated (the disable-path in VehicleSubscriber.clear_discovery/1 does pass the namespace correctly). On an instance with MQTT_NAMESPACE set this means:
- its own stale configs (under
teslamate_<ns>_<car_id>) are never cleared, and - worse, it publishes empty retained payloads to the un-namespaced
teslamate_<car_id>/...topics — potentially wiping the entities of a sibling instance on the same broker, which is exactly the interference the namespace commit prevents.
Should be a one-liner (Keyword.take(opts, [:discovery_prefix, :namespace])) plus a namespace case in pubsub_test.exs.
Non-blocking points:
- The
Task.startcleanup runs concurrently with the supervisor children starting up, so it may fire before the MQTT connection is established — failures are only logged and cleanup then happens on the next restart. Since it's idempotent and retried every start that's fine, but a short code comment noting this would help future readers. - The
drain_discovery_configstest helper is now copied into three test files (vehicle_subscriber_test,vehicle_sync_test, and a variant inhome_assistant_test) — maybe worth extracting into a shared helper while squashing. Each call also idles for 200 ms, which adds a bit of suite runtime. - The
state_class: total→total_increasingchange also touches the pre-existing manual YAML in the docs. Semantically correct, but worth a mention in the release notes since it changes HA long-term statistics behavior for manual-config users who re-sync their YAML. - Noticed in passing:
validate_namespace!rejects/but not the MQTT wildcards+/#, while the newvalidate_discovery_prefix!does. A namespace containing a wildcard already breaks the regular state topics onmain, so this is pre-existing and out of scope — but if you're touchingruntime.exsanyway, aligning the two would be a cheap hardening.
I also checked the performance impact with our low-end targets (e.g. Raspberry Pi 3B+) in mind, since the boot-time clears run for everyone, including users who never enable discovery: steady state adds only a single boolean check per summary; the one-time discovery burst (~72 small JSON payloads) goes through the same publisher path as the ~60 values of every regular summary publish, so it's on the order of one extra summary; and the per-boot clears run in handle_continue / a background task, so supervision tree startup isn't blocked and boot time is unchanged. If the broker is unreachable, Tortoise fails fast and reduce_while halts — no timeout pile-up. So: no concerns.
Once the namespace fix is in, this looks ready to me. Regarding the remaining topics from the coverage list: fine to leave as a follow-up, the per-entity configs make extending cheap.
🤖 Review drafted with Claude Code (Fable 5 high) — sponsored by Claude for Open Source
29609ed to
039adb3
Compare
|
To save a round-trip I pushed the remaining review points myself:
All affected test files pass locally and 🤖 Review drafted with Claude Code (Fable 5 high) — sponsored by Claude for Open Source |
Add an MQTT_HOME_ASSISTANT_DISCOVERY env var that, when enabled, makes VehicleSubscriber publish HA discovery config payloads (one per entity) to homeassistant/<component>/teslamate_<car_id>/<object_id>/config on startup, mirroring the manual configuration documented in website/docs/integrations/home_assistant.md.
Fix the header separator row, which gained an extra column, and the MQTT_HOME_ASSISTANT_DISCOVERY_PREFIX row, which had a trailing empty cell. Addresses review comment on #5543.
MQTT_HOME_ASSISTANT_DISCOVERY_URL is optional; clarify that setting MQTT_HOME_ASSISTANT_DISCOVERY does not require it. Addresses review comment on #5543.
Validate the discovery prefix like MQTT_NAMESPACE: empty values fall back to the default so topics never get a leading '/', and MQTT wildcards are rejected. Addresses review comment on #5543.
Call HomeAssistant.clear/3 so retained discovery configs (and the entities in Home Assistant) are removed when discovery is disabled. Addresses review comment on #5543.
Prefix the discovery object_id with tesla_ so Home Assistant generates the same entity IDs as the documented manual configuration (e.g. sensor.tesla_speed instead of sensor.speed), avoiding broken dashboards and automations on migration. Drop the _km suffix from the battery range sensors to match the manual unique_ids. Document that the manual mqtt_sensors.yaml must be removed before enabling discovery to avoid duplicate unique_id errors. Addresses review comment on #5543.
Pass the namespace to HomeAssistant.clear/3 in clear_removed_vehicles/2 so a namespaced instance clears its own discovery topics instead of the un-namespaced ones of a sibling instance sharing the broker. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Align with validate_discovery_prefix!: a namespace containing + or # would produce unpublishable state and discovery topics, so fail fast at boot. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
039adb3 to
75da857
Compare
…5611) * fix(mqtt): don't require the namespace option in VehicleSubscriber Since #5543, Mqtt.init drops nil options before starting PubSub, so the :namespace key is absent when MQTT_NAMESPACE is unset and the previous Keyword.fetch! crashed every such instance at boot (KeyError crash loop). Use Keyword.get and add a regression test starting a subscriber without the key. Fixes #5608 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: update changelog --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Add an MQTT_HOME_ASSISTANT_DISCOVERY env var that, when enabled, makes VehicleSubscriber publish HA discovery config payloads (one per entity) to homeassistant//teslamate_<car_id>/<object_id>/config on startup, mirroring the manual configuration documented in website/docs/integrations/home_assistant.md.