Skip to content
This repository has been archived by the owner on Aug 22, 2021. It is now read-only.

[EXPLORATIONAL] Fix issues with deprecated discovery syntax in HomeAssistant 0.84 #53

Closed
wants to merge 1 commit into from

Conversation

donkawechico
Copy link
Contributor

This is an explorational PR that attempts to resolve Issue #51 in such a way that won't break lights for users of older HomeAssistant versions (< 0.84).

ISSUE

Home Assistant 0.84 removed the "mqtt_json" platform type, replacing it with a combination of "platform: mqtt" with "schema: json".

This breaks AiLight auto-discovery for users of HA >= 0.84 since current AiLight firmware only sends the now-legacy "mqtt_json" as a platform.

The resolution is simple, but could break lights for AiLights users running older versions of HA./

PROPOSED SOLUTION

Add a variable in config.h that allows user to override the "new way" for HA discovery:

#define MQTT_HOMEASSISTANT_DISCOVERY_ENABLED false
#define MQTT_HOMEASSISTANT_DISCOVERY_PREFIX "homeassistant"

#define MQTT_HOMEASSISTANT_USE_LEGACY_DISCOVERY false

@donkawechico donkawechico changed the title fix(discovery): Fix issues with deprecated discovery syntax in HomeAssistant 0.84 Fix issues with deprecated discovery syntax in HomeAssistant 0.84 Jan 28, 2019
@donkawechico donkawechico changed the title Fix issues with deprecated discovery syntax in HomeAssistant 0.84 [EXPLORATIONAL] Fix issues with deprecated discovery syntax in HomeAssistant 0.84 Jan 28, 2019
@stelgenhof
Copy link
Owner

Thank you very much for your PR!

I think it covers all areas, however this change adds a configuration item to the EEPROM. Not sure if that is actually necessary.

Another approach is to allow people make a choice on compile time, without the need of storing this in the EEPROM. In fact, having this item in the EEPROM will not do anything, as you would need to recompile anyway in case you want to have it changed :)

Cheers! Sacha

@donkawechico
Copy link
Contributor Author

donkawechico commented Jan 29, 2019

@stelgenhof I understood some of those words! :P Which stuff is on EEPROM and which stuff isn't?

What would it look like to "allow people to make a choice at compile time"? Here's my best guess:

  1. Copy light.ino to a new file called light_legacy.ino (keeping light.ino)
  2. Change light.ino to use "platform: mqtt" and "schema: json" (in other words, no if legacy conditionals)
  3. Add something like [env:prod-legacy] to platformio.ini that's set up to build using light_legacy.ino rather than light.ino

Is that way off?

@stelgenhof
Copy link
Owner

@donkawechico On the EEPROM the configuration is stored. The configuration contains settings that are needed when the light is running. Often these are values stored when you (user) interacts with the device. For example it holds the settings for the MQTT server, topics, that can be changed via the UI.

The decision to have settings stored in the configuration vs settings when compiling the firmware are arbitrary: it all depends on the application :)

As such I don't think it wouldn't be necessary for users to alter a legacy configuration on runtime. My proposal is just have compile directive so people that haven't yet upgraded to HA 0.84 can compile a version for themselves that is compatible. The default code (and provided binaries) will be only working for HA > 0.84.

So you would only need a directive:

MQTT_HOMEASSISTANT_USE_LEGACY_DISCOVERY false

and then in the light.ino file this:

#ifdef MQTT_HOMEASSISTANT_USE_LEGACY_DISCOVERY
    md_root["platform"] = "mqtt_json";
#else
   md_root["platform"] = "mqtt";
   md_root["schema"] = "json";
#endif

(Untested code)

Hopefully it's clear :)

Cheers! Sacha

@donkawechico
Copy link
Contributor Author

@stelgenhof just FYI, I'm not going to be actively working on this in the next week or so -- I bit the bullet and just added all my lights manually in Home Assistant.

If you, or anyone else wants to take up the torch, feel free.

@stelgenhof
Copy link
Owner

@donkawechico No worries! Thanks for the research and PR. I may put some changes in next week, and will do some testing as well.

Cheers! Sacha

if (cfg.mqtt_ha_use_legacy_discovery) {
md_root["platform"] = "mqtt_json";
} else {
md_root["platform"] = "mqtt";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is not required. ref: home-assistant/core#19420

@stelgenhof
Copy link
Owner

@donkawechico Just made changes that enable the new syntax for Home Assistant 0.84 (or later) and an option for users that are still on Home Assistant 0.84 (or older).

Let me close this PR as it isn't valid any longer. Thanks again for the work done!

Cheers! Sacha

@stelgenhof stelgenhof closed this Feb 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants