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

Rewrite Osram Lightify component #22184

Merged
merged 5 commits into from Mar 31, 2019

Conversation

Projects
None yet
4 participants
@OleksandrBerchenko
Copy link
Contributor

commented Mar 19, 2019

Description:

Originally there was another PR that contained all my changes to Osram Lightify component: #21176 . Now I'm splitting it to several smaller ones. This particular PR is only about rewriting existing code, no new features are being added here. Once this PR is merged, I will create the next ones with scenes support, exposing additional attributes, etc.

Pull request in home-assistant.io with documentation: home-assistant/home-assistant.io#8621

Original code has multiple flaws: it is not performance efficient, uses throttling incorrectly, implements status of light groups in a confusing way and does not take into account actual supported features of each device.

Now the module is re-implemented almost from scratch. At the same time I did my best to keep the new code 100% compatible with the old version not to break existing installations.

Summary of changes:

  1. Upgrade underlying python-lightify library from 1.0.6.1 to 1.0.7.1.

  2. Improve performance:

    • Use different frequency for updates of light statuses (need to be near real-time) and group configurations (for most users may be updated once per hour or even less frequently).
    • Fix throttling. Existing throttling was simply broken: update function was always called with no_throttle=True. Now built-in throttling of the underlying library is used. It's more lightweight and also correctly handles errors: if some call failed, it doesn't throttle the next one.
    • Add additional configuration values to allow users control throttling even better.
    • Cache calculated values when possible to avoid unneeded recalculations.
  3. Correctly report statuses of light groups.

  4. Take into account supported features of each device (don't treat all devices as RGBW bulbs).

Please note that unique_id for Lightify groups is still calculated in a wrong way, but I decided not to fix that now not to break existing installations.

Example entry for configuration.yaml:

light:
  - platform: osramlightify
    scan_interval: 1
    host: 192.168.1.50

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
@amelchio
Copy link
Contributor

left a comment

This looks quite good, you just copied a lot of stuff that is not needed.

A light platform does not have to handle all aliases, that is done in the core Light component.

Show resolved Hide resolved homeassistant/components/osramlightify/light.py Outdated
Show resolved Hide resolved homeassistant/components/osramlightify/light.py Outdated
Show resolved Hide resolved homeassistant/components/osramlightify/light.py Outdated
Show resolved Hide resolved homeassistant/components/osramlightify/light.py Outdated
Show resolved Hide resolved homeassistant/components/osramlightify/light.py Outdated
Show resolved Hide resolved homeassistant/components/osramlightify/light.py Outdated
Show resolved Hide resolved homeassistant/components/osramlightify/light.py Outdated
Show resolved Hide resolved homeassistant/components/osramlightify/light.py Outdated
Show resolved Hide resolved homeassistant/components/osramlightify/light.py
Show resolved Hide resolved homeassistant/components/osramlightify/light.py Outdated
@amelchio
Copy link
Contributor

left a comment

Very nice, just a couple of minor comments now.

Show resolved Hide resolved homeassistant/components/osramlightify/light.py Outdated
Show resolved Hide resolved homeassistant/components/light/__init__.py Outdated

OleksandrBerchenko added some commits Mar 31, 2019

@OleksandrBerchenko

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2019

@amelchio

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2019

This looks good now. However, I now read the docs update and I actually got a bit confused by the multiple update timers, three in total. Is it really so expensive to get the groups that they need to run on a separate schedule?

@OleksandrBerchenko

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2019

@amelchio All I/O operations are expensive. And actually there are two timers not three:

  1. interval_lightify_status - how often we want to query light updates. There is one network request under the hood. We want to run that query as frequent as possible.

  2. interval_lightify_conf - how often we want to query groups/scenes configuration. There are two consequent network requests under the hood. If a user doesn't change Lightify configuration (last time I changed it about a year ago), there is no need to re-run that query at all. Even using the default value we save about 1,438 unneeded network requests per hour. Also if we run them together with light updates, we may introduce an additional 1-2 seconds delay for each update.

Both configuration values are optional: default values should work fine for most users, but allow a space for fine-tuning. Personally I set interval_lightify_status to 2 seconds and interval_lightify_conf to 86400 seconds.

scan_interval is a general HA parameter, and I just emphasized that it should be less or equal than interval_lightify_status (as common sense suggests).

Thanks!

@amelchio

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2019

Could we remove interval_lightify_status and use scan_interval for that timer?

@OleksandrBerchenko

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2019

Hm... Possibly yes. Let me think about use cases, when we might need different values.

@amelchio

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2019

Cool, thanks. My concern is that if we first introduce this configuration, taking it out again in a cleanup would be a breaking change. So I'd rather not introduce it unless it is really needed. A little inflexibility is okay, Home Assistant does not try to solve all problems for everybody :)

@OleksandrBerchenko

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2019

@amelchio Well, in theory they may be the same. You might want to make scan_interval less than interval_lightify_status (according to new documentation: "Shorter scan_interval may improve synchronization speed between individual lights and groups. For example, if you turn on a group, all its lights may be updated to on immediately, without querying the bridge."), but that should not be super critical.

On the other hand, reusing scan_interval seems a bit like a workaround. First, I'm a bit concerned about "These options are being phased out and are only available for single platform integrations" (https://www.home-assistant.io/docs/configuration/platform_options/#scan-interval ). Also I need int but scan_interval is actually datetime.timedelta, so I need to do an additional conversion. That's not a big deal by itself, but I'm concerned about a tight coupling to a global configuration value that I have no control of and that functionality may change at any time. Second, it is assumed to have 30 seconds by default (and it really works that way), but I can't access that default value from my module (if it's not set for osramlightify platform in configuration.yaml, I just see None). That means I still need my own default to be hardcoded somewhere.

To be honest, I don't see a big issue in having interval_lightify_status as a separate configuration value, not coupled to anything else and clearly described in the documentation. And I don't think we will ever need to clean it up in the future :)

What do you think?

@amelchio

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2019

Okay, I see your problem. To really do this properly, we should make an osramlightify component that handles the communication with the hub and pushes new state to each light entity. That would anyway cause a breaking change in the configuration, so I guess we can roll with this for now.

@amelchio amelchio merged commit 842a36d into home-assistant:dev Mar 31, 2019

3 of 6 checks passed

Build Error Workflow: Build Error
Details
ci/circleci: Build Error Your tests failed on CircleCI
Details
coverage/coveralls Coverage decreased (-0.7%) to 92.939%
Details
Hound No violations found. Woof!
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ghost ghost removed the in progress label Mar 31, 2019

@OleksandrBerchenko

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2019

@amelchio FYI, the second part: #22597

Thanks!

unibeck pushed a commit to unibeck/home-assistant that referenced this pull request Apr 7, 2019

Rewrite Osram Lightify component (home-assistant#22184)
* Rewrite Osram Lightify component

* Update python-lightify version to 1.0.7.2

* Remove unneeded code

* 1. Remove changes in light/__init__.py, 2. Set properties to None by default

* Fix typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.