Skip to content

Add polling interval service and setting available through zwave node entity panel#9056

Merged
turbokongen merged 8 commits into
home-assistant:devfrom
turbokongen:zwave-config
Sep 11, 2017
Merged

Add polling interval service and setting available through zwave node entity panel#9056
turbokongen merged 8 commits into
home-assistant:devfrom
turbokongen:zwave-config

Conversation

@turbokongen
Copy link
Copy Markdown
Contributor

@turbokongen turbokongen commented Aug 20, 2017

Description:

Let's user set polling interval during runtime.
Moves polling and ignore into node entity card.
This will also save to the zwave_device_config.yaml file.
Tied with: home-assistant/frontend#402
image

@mention-bot
Copy link
Copy Markdown

@turbokongen, thanks for your PR! By analyzing the history of the files in this pull request, we identified @armills, @balloob and @andrey-git to be potential reviewers.

@andrey-git
Copy link
Copy Markdown
Contributor

On HA restart the poll value will get back to the config setting

@turbokongen
Copy link
Copy Markdown
Contributor Author

Yes, we can ofcourse remove everything all together, and make every config that has polling_intensity set fail, or we can leave both, and let users migrate. I can add a warning to be printed for each config line set.
Is that a usable solution?

@andrey-git
Copy link
Copy Markdown
Contributor

I don't see a migration path here.
Things shouldn't change on restart and creating a start-up automation that will call a service is too cumbersome.

Why have this runtime-controlled anyway?

@turbokongen
Copy link
Copy Markdown
Contributor Author

Isn't it wanted to be able to set/remove polling without a restart of the whole server?

@andrey-git
Copy link
Copy Markdown
Contributor

It is definetly a plus, but if the value is not retained across a restart, then it's a step back.

@turbokongen
Copy link
Copy Markdown
Contributor Author

You are completely right @andrey-git
I forgot about the hard setting that was already present in the code.
It's now removed.

turbokongen added a commit to home-assistant/home-assistant.io that referenced this pull request Aug 20, 2017
@andrey-git
Copy link
Copy Markdown
Contributor

OK, this is better now.
This makes backing up zwcfg even more important, as another piece of info is now stored there.

@rofrantz
Copy link
Copy Markdown
Contributor

@turbokongen @andrey-git please consider the following possible scenario: a system crashes for whatever reasons. With automatic deployments u redeploy the app very simple and have everything working again instantly; by moving configuration things to UI (like u did with the entire zwave node id namings) which indeed need to be setup only once in the UI) u're asking that user to reconfigure manually from UI everything.

You're moving from hardcoded controllable configuration deployed files used in automated deployments to manual labor configurations from UI.

Is this the wanted behavior and/or direction ?

@turbokongen
Copy link
Copy Markdown
Contributor Author

@rofrantz All the information you mention is stored in zwcfg[home_id].xml
So a backup of that file will restore the data. Just like copying back the configuration.yaml restores the settings.

@rofrantz
Copy link
Copy Markdown
Contributor

Thanks @turbokongen that's indeed a solution too.
I prefered to avoid it since I didn't want get dirty with the content of that file...u know and I know how it looks and what it contains compared to the nice looking dedicated yaml config file which is more human readable, not to mention it contained only the data I care about and not 10000 zwave params and configs.

But I guess that will work.
Thanks for your feedback

@andrey-git
Copy link
Copy Markdown
Contributor

Another point is that configuration.yaml doesn't get corrupted easily as it is written only on user action.
If you change a value in the UI (that should go into zwcfg) you basically need to restart anyway in order to write the data to zwcfg, otherwise if HA doesn't go down gracefully (blackout?) the change will be lost.

That's also the reason I disliked the IDs change - I don't want to depend on zwcfg.

@rofrantz
Copy link
Copy Markdown
Contributor

@andrey-git I didn't wanna raise the dead, but as @turbokongen once said "it is decided, and it is done" :)

@andrey-git
Copy link
Copy Markdown
Contributor

Since zwave devices have only a handful config attributes and two of them (ignore and poll) are already in UI config editor, I think a better solution would be to implement the rest of them in the UI.

That said, changing poll interval should also call the new service - updating the device live, but in addition saving this into the config, thus not relying on zwcfg.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

block comment should start with '# '

@turbokongen
Copy link
Copy Markdown
Contributor Author

turbokongen commented Aug 26, 2017

I'm torn in how to implement this. To be able to do both the live and the config file updates, we need the entity_id together with the value_id. I can't seem to find one place where both are present, so this first draft is accessing a protected member to generate the entity_id in the value-view api. Please comment and suggest, as this is the best I could come up with at this moment. 😢

@turbokongen turbokongen changed the title Add polling interval service and setting available through zwave value panel Add polling interval service and setting available through zwave node entity panel Sep 3, 2017
@turbokongen
Copy link
Copy Markdown
Contributor Author

Ping?

Copy link
Copy Markdown
Contributor

@emlove emlove left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. Just one question.

if polling_intensity:
self.primary.enable_poll(polling_intensity)
else:
self.primary.disable_poll()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Question: Is polling disabled by default, and that's why this was removed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Polling is always set to 0 by OZW unless it is set to something different by user to the value_id.
So if there is no entry for polling, in either the homeassistant config or the zwcfg, no polling is set.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍 Just checking.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Appreciate it 🥇

@turbokongen turbokongen merged commit cc19796 into home-assistant:dev Sep 11, 2017
@andrey-git
Copy link
Copy Markdown
Contributor

So if I understand correctly:

For users who have poll intensity defined in the config: Using the UI will take effect but on restart it will fall back to the config value.

For users without poll intensity in the config: Poll intensity is stored in zwcfg

@balloob
Copy link
Copy Markdown
Member

balloob commented Sep 12, 2017

Why can't we do both, we store it both in the config and make it take effect right away? And in the future we add a checkbox to do it temporarily only (and not write it to config).

@turbokongen
Copy link
Copy Markdown
Contributor Author

When using the Frontend editor, it sets both the live value and saves to the configuration. If only using the service, that is temporary.

@balloob balloob mentioned this pull request Sep 22, 2017
fabaff pushed a commit to home-assistant/home-assistant.io that referenced this pull request Sep 26, 2017
* Update z-wave.markdown

Changes for:
home-assistant/core#9056

* Update z-wave.markdown

* Update z-wave.markdown

* Update z-wave.markdown
@home-assistant home-assistant locked and limited conversation to collaborators Dec 11, 2017
@ghost ghost removed the platform: config.zwave label Mar 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants