Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discovery Platform #387

Merged
merged 77 commits into from
Apr 28, 2021
Merged

Discovery Platform #387

merged 77 commits into from
Apr 28, 2021

Conversation

krkeegan
Copy link
Collaborator

@krkeegan krkeegan commented Apr 23, 2021

Proposed change

This adds the Discovery Platform.

In theory all of the code is complete and has been unit tested to confirm it works as intended.

I can't really test the interaction with HomeAssistant without migrating my entire installation. I have tested small parts and it seems to work. I will attempt to do the migration this weekend. I will note any issues along the way and add to the documentation.

If anyone else wants to start testing this, that would be helpful. These changes have very little interaction with the existing code, so it shouldn't break anything. But, I would like to work out all of the issues within Discovery before merging even into dev, because changes to the config file can't be pushed to users once they set everything up.

Additional information

Checklist

  • The code change is tested and works locally. (Works locally, but interaction with HA has not been tested). Tested
  • Dog food - install and run on my own HA installation
  • Local tests pass.
  • Tests have been added to verify that the new code works.
  • Code documentation was added where necessary
  • Documentation added/updated
  • Add documentation about migrating to Discovery
  • Add documentation about any gotchas discovered while testing

To be used for setting the Name in the discovery payload.
Store Name in Case as Entered by User
Adds the basic functionality for reading discovery settings from
the config.yaml file.  Lightly implemented in Dimmer.

Still does not publish anything yet, need to monitor the
homeassistant/status topic
Monitors a defined topic looking for a message indicating that
HomeAssistant was restarted and triggers a re-publish of
discovery entities if that happens.

Publish discovery entities on startup.
Device information is likely to be the same structure across all
devices.  Or at least most devices.  This adds a variable that
is defined in yaml and useable in the jinja templates.

Add basic dimmer discovery entities.
Add additional information about the device model and such.
This is all primarily cosmetic, but it might aid with support
requests if a user can see their device info in a nice GUI.
This way it is only processed once, seems more streamlined.
Skeleton of Discovery Implementation
Discovery Documentation; Additional Variables and Tweaks
Can't use default discovery class name for selecting the config
section, it will cause the wrong inheritance with things like
fan_linc inheritting dimmer settings.

Rename some variables and add better descriptions to try and make
them more clear.
Rather than extending a method, I think it is more concise and
makes more sense to load this data into the rendered topics
map on load.
Primarily to support KPL.  But other devices may need this as
well.
@krkeegan krkeegan linked an issue Apr 23, 2021 that may be closed by this pull request
@krkeegan
Copy link
Collaborator Author

Progress:
image

Necessary because the modem only uses a single template with
an indeterminate, until run time, number of topics generated
The HA implementation of the discovery protocol only allows a
very small subset of characters for use in the node_id and
the object_id.  This replaces all un-allowed characters with an
underscore.

Also removed the ability to set the topic via yaml.  This never
really worked anyways and would create potential problems.

Reverted changing the periods in the address to underscores,
periods are still fine in the unique_id, just not in the topic
string.
Discovery Tweaks After Initial Usage
@krkeegan
Copy link
Collaborator Author

I am mostly up and running. I ran into some bugs I had to fix. But it generally seems to be working.

There are some quirks:

  1. HA does not save the state between restarts, if you had turned off retain in IM, all devices will start out as off after any restart. I am going to have to re-enable retain.
  2. It isn't clear that this is the best form of configuration. But it does seem to be the direction that HA is going, and it does add some new features (mostly cosmetic).
  3. Transitioning from yaml entities to mqtt discovered entities is not easy. At least not for me, particularly if you created a lot of unique entities to solve various issues and limitations.

There is a small issue with modem scenes. Because of the order in which IM loads, when the discovery topics are published, we have not yet loaded the scenes file and so do not have the names of scenes. It isn't a big deal, but users would end up with scene numbers which may be confusing. I may have to add a signal to cause the discovery topics to publish only after we are fully loaded.

Anyways, I still have more testing and tinkering to do. I only just got things vaguely functioning again with all of my automations.

@krkeegan
Copy link
Collaborator Author

Oh, also, it does cause a lot of mqtt traffic on startup. I am publishing/subscribing to about 431 topics for 87 devices. It takes about 5-10 seconds for all of the mqtt activity to die down after an IM restart and that is with an intel i5 processor. Raspberry pi devices will likely struggle a bit more.

@krkeegan
Copy link
Collaborator Author

OK, I am satisfied that this works as expected. I am going to merge this into dev tonight along with the other waiting PRs in preparation for the next release. I am a bit busy this weekend, so the release will likely be the middle of next week. So please send any PRs if you see something wrong.

I am also going to recommend that people NOT go through the process of migrating to the discovery platform unless they really want to. Until we do the fixes with the config files, migrating is a tedious process. Even once we improve the config design, migrating will still be a good amount of work for not much benefit.

The real benefit is for new users, for them this is an unconditional improvement.

@krkeegan krkeegan merged commit adea282 into dev Apr 28, 2021
@roopesh
Copy link

roopesh commented Apr 29, 2021

I'm super duper lucky that I just installed insteon-mqtt yesterday and started looking for auto discovery... how fortunate it's coming soon!

If I'm using the HA add-on, can I somehow install this branch?

@krkeegan
Copy link
Collaborator Author

If you are comfortable with git, these bare instructions should get you what you need. You can just checkout the dev branch.

If not, this should be pushed to release next Tuesday/Wednesday

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MQTT discovery automatic configuration
2 participants