BREAKING CHANGE: Fix: use uniqueId for battery HA auto-discovery#2442
BREAKING CHANGE: Fix: use uniqueId for battery HA auto-discovery#2442AndreasBoehm wants to merge 1 commit intodevelopmentfrom
Conversation
|
✅ Actions performedReview triggered.
|
AndreasBoehm
force-pushed
the
andreasboehm/feature/ha-auto-discovery-multiple-batteries
branch
from
700f3ff to
c928bcc
Compare
Build ArtifactsFirmware built from this pull request's code:
Notice
|
|
@Manos1966 probiers mal mit dem build von diesem PR. Das sollte jetzt klappen. |
WalkthroughModified battery ID generation in MQTT Home Assistant integration by replacing hardcoded pseudo-serial field with a dynamic Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Habe ich dir schon mal erzählt, wie frustrierend es ist, stundenlang zu versuchen, etwas zu verstehen (ohne Erfolg), und dann kommt jemand anderes und liefert die Lösung innerhalb von vier Stunden? SEHR FRUSTIEREND! 🤣 Es funktioniert! 🥇 Zwei Batterien (zwei verschiedene OpenDTU-OnBattery sofort erkannt: 
|
|
Perfekt! ❤️ 
|
|
Among the four, one uses the old Firmware. 
|
|
UPDATE @AndreasBoehm ich habe heute die Firmware der vierten DTU mit der neuen Firmware upgedated. Also, KEIN "Breaking change" 
Ich hatte bis jetzt keine Zeit gehabt zu pruefen, ob die anderen Daten getroffen sind UPDATE2: es funktioniert wunderbar!
Vielen Dank Andreas! 🥇 |
Previously the battery topics where based on the hardcoded serial '0001' which leads to overlaps when multiple DTUs are connected to the same HA instance.
AndreasBoehm
force-pushed
the
andreasboehm/feature/ha-auto-discovery-multiple-batteries
branch
from
c928bcc to
dcb983d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/battery/HassIntegration.cpp (1)
54-55: CachecreateBatteryId()once per method to reduce temporaryStringchurn.Use a local
batteryIdin each publish method and reuse it for bothconfigTopicanduniq_id.♻️ Proposed refactor
void HassIntegration::publishSensor(const char* caption, const char* icon, const char* subTopic, const char* deviceClass, const char* stateClass, const char* unitOfMeasurement, const bool enabled) const { String sensorId = sanitizeUniqueId(caption); + const String batteryId = createBatteryId(); - String configTopic = "sensor/" + createBatteryId() + String configTopic = "sensor/" + batteryId + "/" + sensorId + "/config"; @@ - root["uniq_id"] = createBatteryId() + "_" + sensorId; + root["uniq_id"] = batteryId + "_" + sensorId; } @@ void HassIntegration::publishBinarySensor(const char* caption, const char* icon, const char* subTopic, const char* payload_on, const char* payload_off, const bool enabled) const { String sensorId = sanitizeUniqueId(caption); + const String batteryId = createBatteryId(); - String configTopic = "binary_sensor/" + createBatteryId() + String configTopic = "binary_sensor/" + batteryId + "/" + sensorId + "/config"; @@ - root["uniq_id"] = createBatteryId() + "_" + sensorId; + root["uniq_id"] = batteryId + "_" + sensorId; }Also applies to: 64-64, 108-110, 118-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/battery/HassIntegration.cpp` around lines 54 - 55, Cache the result of createBatteryId() into a local variable (e.g., batteryId) at the start of each publish method and reuse that variable when building configTopic and when setting uniq_id instead of calling createBatteryId() multiple times; update places that construct configTopic (currently using "sensor/" + createBatteryId() + "/" + sensorId) and any lines that set uniq_id to reference batteryId, and apply the same change to the other occurrences noted (around the other publish calls at the indicated ranges).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/battery/HassIntegration.cpp`:
- Around line 54-55: The Home Assistant discovery IDs were changed to use
createBatteryId(), which will make HA treat entities as new and leave old ones
orphaned; add migration documentation to release notes explaining the reason
(fixing hardcoded '0001' collisions), expected upgrade behavior (new entities,
old ones unavailable), and explicit cleanup steps (publish an empty retained
payload to legacy discovery topics such as
homeassistant/sensor/0001/<sensorId>/config to remove orphaned entities). Also
implement an optional one-time migration routine in HassIntegration (on first
startup after upgrade) that iterates legacy discovery topic patterns (e.g.,
sensor/0001/<sensorId>/config) and publishes empty retained payloads to remove
old entities, ensuring it only runs once and records completion to avoid
repeated runs; reference createBatteryId(), configTopic, uniq_id and device ids
when locating the related code to update.
---
Nitpick comments:
In `@src/battery/HassIntegration.cpp`:
- Around line 54-55: Cache the result of createBatteryId() into a local variable
(e.g., batteryId) at the start of each publish method and reuse that variable
when building configTopic and when setting uniq_id instead of calling
createBatteryId() multiple times; update places that construct configTopic
(currently using "sensor/" + createBatteryId() + "/" + sensorId) and any lines
that set uniq_id to reference batteryId, and apply the same change to the other
occurrences noted (around the other publish calls at the indicated ranges).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5a4a55bf-a431-4186-8e81-ba42383af4c2
📒 Files selected for processing (2)
include/battery/HassIntegration.hsrc/battery/HassIntegration.cpp
| String configTopic = "sensor/" + createBatteryId() | ||
| + "/" + sensorId |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Home Assistant MQTT discovery, what happens when an existing entity's uniq_id changes after a firmware update? Are old entities/devices automatically migrated or left as stale registry entries?
💡 Result:
Web search failed: Server error: no LLM provider could handle the message
🏁 Script executed:
# First, find and examine the HassIntegration.cpp file
fd -t f "HassIntegration.cpp" --exec wc -l {} \;Repository: hoylabs/OpenDTU-OnBattery
Length of output: 517
🏁 Script executed:
# Get the specific lines mentioned in the review
cat -n src/battery/HassIntegration.cpp | sed -n '50,170p'Repository: hoylabs/OpenDTU-OnBattery
Length of output: 4165
🌐 Web query:
Home Assistant MQTT discovery uniq_id change migration behavior
💡 Result:
unique_idis the identity key. Home Assistant uses an entity’sunique_idto tie MQTT-discovered entities to the Entity Registry (so you can later rename theentity_idand keep that rename). (home-assistant.io)- If you change the
unique_id, Home Assistant will treat it as a different entity. Practically, that means you’ll get a new entity in HA, and the old one will remain as an orphan (often unavailable) until you remove it; HA will not automatically “migrate” the old entity’sentity_id, area, history settings, automations, etc., to the newunique_id. (This follows from how discovery creates/updates entities keyed byunique_id, and that removals are explicit.) (home-assistant.io) - Changing the discovery topic’s
<object_id>is not the same as changingunique_id. The<object_id>segment in the discovery topic does not control the resultingentity_id; best practice is to set that<object_id>to theunique_id(so you don’t accidentally create parallel entities when you change topics). (home-assistant.io) - How to clean up the old entity (recommended “migration” pattern):
- Publish the new discovery config (with the new
unique_id) so the new entity appears. - Publish an empty (retained) payload to the old discovery
.../configtopic to delete the old discovered entity and clear the retained config. (home-assistant.io)
- Publish the new discovery config (with the new
- Related but different:
{"migrate_discovery": true}is a special mechanism for migrating single-component discovery topics to device discovery while retaining settings; it’s not a general “unique_id change migration.” (home-assistant.io)
Citations:
- 1: https://www.home-assistant.io/integrations/mqtt
- 2: https://www.home-assistant.io/integrations/mqtt
- 3: https://www.home-assistant.io/docs/mqtt/
- 4: https://www.home-assistant.io/integrations/mqtt
- 5: https://www.home-assistant.io/integrations/mqtt
🌐 Web query:
Home Assistant MQTT entity unique ID change upgrade retained topics
💡 Result:
Web search failed: Server error: no LLM provider could handle the message
🏁 Script executed:
# Search for migration or cleanup related code
rg -i "migration|cleanup|legacy|orphan" src/battery/ --type cpp --type hRepository: hoylabs/OpenDTU-OnBattery
Length of output: 182
🏁 Script executed:
# Check for CHANGELOG or release notes mentioning battery IDs or migration
fd -t f "CHANGELOG|RELEASE|NEWS|MIGRATION" --iglob "*" | head -20Repository: hoylabs/OpenDTU-OnBattery
Length of output: 271
🏁 Script executed:
# Search for any cleanup of old discovery topics in the codebase
rg "publish.*config.*empty\|empty.*payload\|retained" src/ --type cppRepository: hoylabs/OpenDTU-OnBattery
Length of output: 51
🏁 Script executed:
# Check git history for when this change was made and if there were migration notes
git log --oneline -20 src/battery/HassIntegration.cpp 2>/dev/null || echo "Git history not available"Repository: hoylabs/OpenDTU-OnBattery
Length of output: 125
🏁 Script executed:
# Get the full commit details to see what changed
git show dcb983d8 --no-patchRepository: hoylabs/OpenDTU-OnBattery
Length of output: 413
🏁 Script executed:
# Check what the old battery ID format was
git show dcb983d8:src/battery/HassIntegration.cpp 2>/dev/null | head -100Repository: hoylabs/OpenDTU-OnBattery
Length of output: 2954
🏁 Script executed:
# Verify cpplint compliance for the affected file
# Check if cpplint is available
which cpplint || echo "cpplint not available"Repository: hoylabs/OpenDTU-OnBattery
Length of output: 151
🏁 Script executed:
# At minimum, check for obvious style issues in the affected lines
cat -n src/battery/HassIntegration.cpp | sed -n '54,56p; 64p; 108,110p; 118p; 146p'Repository: hoylabs/OpenDTU-OnBattery
Length of output: 495
Add migration documentation for Home Assistant discovery identity changes.
The battery discovery uniq_id (lines 64, 118) and device ids (line 146) now use createBatteryId() to fix multi-DTU collisions. However, on upgrades, Home Assistant will treat these as new entities and leave old ones orphaned—automations and dashboards bound to the old IDs will break, and HA does not auto-migrate them.
To mitigate this for users, add release notes documenting:
- The change reason (fixing hardcoded '0001' collisions in multi-DTU setups)
- Expected behavior on upgrade (new entities, old ones become unavailable)
- Cleanup procedure: publish an empty retained payload to the old discovery topics (e.g.,
homeassistant/sensor/0001/<sensorId>/config) to explicitly remove orphaned entities from the HA registry
Alternatively, consider a one-time cleanup routine on first startup after the ID changes to publish empty payloads for any legacy discovery topics.
Also applies to: 54–55, 108–110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/battery/HassIntegration.cpp` around lines 54 - 55, The Home Assistant
discovery IDs were changed to use createBatteryId(), which will make HA treat
entities as new and leave old ones orphaned; add migration documentation to
release notes explaining the reason (fixing hardcoded '0001' collisions),
expected upgrade behavior (new entities, old ones unavailable), and explicit
cleanup steps (publish an empty retained payload to legacy discovery topics such
as homeassistant/sensor/0001/<sensorId>/config to remove orphaned entities).
Also implement an optional one-time migration routine in HassIntegration (on
first startup after upgrade) that iterates legacy discovery topic patterns
(e.g., sensor/0001/<sensorId>/config) and publishes empty retained payloads to
remove old entities, ensuring it only runs once and records completion to avoid
repeated runs; reference createBatteryId(), configTopic, uniq_id and device ids
when locating the related code to update.
2 participants
Fixes #1587
Previously the battery topics where based on the hardcoded serial '0001' which leads to overlaps when multiple DTUs are connected to the same HA instance.
TODO