Skip to content

Add adaptive_lighting component#40626

Closed
basnijholt wants to merge 165 commits into
home-assistant:devfrom
basnijholt:adaptive-lighting
Closed

Add adaptive_lighting component#40626
basnijholt wants to merge 165 commits into
home-assistant:devfrom
basnijholt:adaptive-lighting

Conversation

@basnijholt
Copy link
Copy Markdown
Contributor

@basnijholt basnijholt commented Sep 26, 2020

Note: anyone that wants to try this out, add https://github.com/basnijholt/adaptive-lighting to your custom repos in HACS and install it! Please report here if you find any bugs 🐛 Read the docs here

This PR adds a new adaptive_lighting component which is based on the circadian_lighting custom component by @claytonjn, currently the most popular(!) component in HACS, however 100% rewritten and with a lot of new functionallity.

I decided to not overwrite the Flux component because this integration works very differently, I believe it would be both a massive breaking change and most of all, I think that the flux integration is poorly named (so harder to find). Considering Apple is bringing out "Adaptive lighting", I am sure that this integration will have a much higher discoverability factor.

@balloob suggested to me (in a private Discord chat) that besides the YAML configuration it would be great to have some UI options. So this component is both fully configurable via the UI and via YAML.

ui
yaml

I chose to keep YAML support due to the rich options one can set. See for example this 100+ lines settings of how I use this in my house, which would be awful to test and setup using the UI.

This component can also be configured without any options or lights set because it offers a service adaptive_lighting.apply which can apply the settings of an adaptive_lighting switch to any lights that are supplied.

An example of a service call to adaptive_lighting.apply is

service: adaptive_lighting.apply
entity_id: switch.adaptive_lighting_default
data:
  lights:
    - light.living_room_lights
    - light.bedroom_lights
  transition: 5  # optional
  turn_on_lights: false  # optional
  adapt_brightness: true  # optional
  adapt_color_temp: true  # optional
  adapt_rgb_color: true  # optional

Otherwise, the options/features are all similar to claytonjn/hass-circadian_lighting up to the following differences:

These graphs illustrate how this component works (are taken from the original repo's README)

Sun Position:
cl_percent|690x131

Color Temperature:
cl_colortemp|690x129

Brightness:
cl_brightness|690x130

I think I have found a nice solution to using validation on both the YAML config and ConfigFlow options with basically the same code. The UI options are more restrictive with indicating times for example, it only allows the "HH:MM:SS" format, while YAML allows any cv.time.

See the documentation in home-assistant/home-assistant.io#14877.

Most of the development of this code happened in the original repo.
For the past months, I have spent many hours in rewriting possibly every single line of the original component.

Where I did work in the following PRs:

Breaking change

Proposed change

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

See a full example of how I use the component here

# Example configuration.yaml with single entry
adaptive_lighting:
  lights:
    - light.living_room_lights
# Example configuration.yaml with two entries
adaptive_lighting:
  - name: living room
    lights:
      - light.living_room_lights
    min_brightness: 70
    take_over_control: true

  # has a sleeping mode
  - name: default
    lights:
      - light.philips_go
      - light.bamboo
      - light.ceiling_bedroom
      - light.lampan
      - light.bed_reading_up
      - light.bed_reading_down
      - light.ceiling_bathroom
      - light.toilet
      - light.ceiling_kitchen
      - light.stairs_up
      - light.stairs_down
      - light.hall_1
      - light.hall_2
      - light.hall_3
    min_brightness: 70
    take_over_control: true
# Example configuration.yaml all settings
adaptive_lighting:
- name: "default"
  lights: []
  prefer_rgb_color: false
  transition: 45
  initial_transition: 1
  interval: 90
  max_brightness: 100
  max_color_temp: 5500
  min_brightness: 1
  min_color_temp: 2000
  sleep_brightness: 1
  sleep_color_temp: 1000
  sunrise_offset: 0
  sunrise_time: "08:00:00"
  sunset_offset: 0
  sunset_time: null  # use actual sunset time
  take_over_control: true
  detect_non_ha_changes: false
  only_once: false

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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

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

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@frenck
Copy link
Copy Markdown
Member

frenck commented Sep 26, 2020

I use this component a lot, however, this is a real issue:

https://github.com/claytonjn/hass-circadian_lighting/wiki/Frequently-Asked-Questions#q-why-cant-i-turn-off-my-lights-after-configuring-them-with-circadian-lighting

Has that been solved with the integration? If not, I would not vote for having it in the core.

Comment thread homeassistant/components/adaptive_lighting/switch.py Outdated
@drinfernoo
Copy link
Copy Markdown
Contributor

drinfernoo commented Sep 26, 2020

@basnijholt Is there no longer a way to specify which lights get controlled by which attributes? I have color bulbs that I used with Circadian Lighting in the lights_ct mode, but they come out as a funky pink color here... presumably they're all being handled by lights_rgb or one of the other schemes?

@basnijholt
Copy link
Copy Markdown
Contributor Author

basnijholt commented Sep 26, 2020

@frenck, both those issues should be fixed.

@drinfernoo, that might be a bug, it should detect that it can change the colors by itself. Could you tell me what number you see in your light's attribute supported_features?

@drinfernoo
Copy link
Copy Markdown
Contributor

All of my colored bulbs (Teckin bulbs integrated with tuya-custom) have Supported features: 19, while my three Waze bulbs have Supported features: 3, and they work fine.

@basnijholt basnijholt force-pushed the adaptive-lighting branch 2 times, most recently from 6bbfe18 to 375d920 Compare September 26, 2020 21:57
@KevinCathcart
Copy link
Copy Markdown
Contributor

@drinfernoo The issue is that this new version prefers to use RGB colors if available, only using color temperature if it is not. Which seems backwards to me. On many bulbs color temperature produces better results than the rgb color can.

The new disable_color_adjust feature is designed to force it to use color temperature instead of rgb. Just make sure not to set it if you include any rgb only bulbs. Instead create one configuration for rgb only ones, and one for CT supporting ones.

Comment thread homeassistant/components/adaptive_lighting/__init__.py Outdated
@drinfernoo
Copy link
Copy Markdown
Contributor

drinfernoo commented Sep 27, 2020

In the newest version (regardless of whether I use disable_color_adjust: true on my color Teckin bulbs or not), they now turn on to their odd pink color, then flash to a more appropriate temperature-based color, then back to pink. These lights are in only_once: true switches.

My white Wyze bulbs still seem to working fine.

EDIT: If I leave disable_color_adjust: true and change to only_once: false, I still get odd coloring, and no flicker to the correct color in the middle.

Comment thread homeassistant/components/adaptive_lighting/switch.py Outdated
@disrupted
Copy link
Copy Markdown

I played around with configuring the integration from the UI, then deleted it, but unfortunately it doesn't remove the switch entity.

@basnijholt
Copy link
Copy Markdown
Contributor Author

@disrupted, it does (should) when you restart Home Assistant.

@disrupted
Copy link
Copy Markdown

@disrupted, it does (should) when you restart Home Assistant.

can confirm, I was just under the impression it would be instant when configured through the UI.

@drinfernoo
Copy link
Copy Markdown
Contributor

I haven't been able to get the UI integration to work 🤔 It won't let me because of stuff in my configuration.yaml I guess?

@mouth4war
Copy link
Copy Markdown

mouth4war commented Sep 27, 2020

  1. How about changing precedence in the code from "RGB then color temperature" to "colour temperature then RGB", instead of adding the disable colour adjust flag? This way users don't have to create separate instances of the component for RGB-only lights. @KevinCathcart

  2. Currently, if the component applies colour temperature outside of the light's range, nothing happens. It could instead set the lowest or highest supported colour temperature of the light.

Copy link
Copy Markdown
Contributor

@KevinCathcart KevinCathcart left a comment

Choose a reason for hiding this comment

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

I do like @mouth4war's suggestions.

I'm a bit confused as to the report of the switch not going away after removing the config entry. If you support async_unload_entry, then it really should be cleaning everything up. At a quick glance I'm not seeing what you are missing/doing wrong, but I'm not familiar with the platform unloading process.

Comment thread homeassistant/components/adaptive_lighting/config_flow.py Outdated
@elupus
Copy link
Copy Markdown
Contributor

elupus commented Mar 3, 2021

I think there might be benefits to restart this pull if it still valid. It now has 160 ish commits. So it has gone through many changes since it was opened. It would be good to get something small in to start with.

@bdraco
Copy link
Copy Markdown
Member

bdraco commented May 8, 2021

I think there might be benefits to restart this pull if it still valid. It now has 160 ish commits. So it has gone through many changes since it was opened. It would be good to get something small in to start with.

As this has grown organically over time, the ~1500 lines that is the base of this integration is tightly coupled into the switch platform entities.

This current design makes it difficult to provide constructive feedback. The base functionality should be moved outside of the switch platform and into a new module or modules.

The switch platform should know very little about the implementation details and only turn on/off functionality.

@home-assistant home-assistant unlocked this conversation May 9, 2021
@frenck
Copy link
Copy Markdown
Member

frenck commented May 9, 2021

Unlocked the PR conversation as it blocks the author from joining the conversation.

⚠️ If you are planning to respond on this PR at this point

Please be sure your comment attributes to the review of the code or any architectural contents of the code. In this PR.

We are trying to avoid this becoming a forum or issue tracker.

Thanks! ❤️

@frenck
Copy link
Copy Markdown
Member

frenck commented May 9, 2021

From an architectural perspective, I'm not really sure if this should even be an integration level. In general, the functionality offered by this integration (and the Circadian Lighting / Flux integrations as well) are really useful and used by the community in all kinds of forms.

I might even dare to say, that functionality like this should be more embedded in Home Assistant Core itself.

Besides the comments from @bdraco and @elupus (I agree with both of them); We currently do not allow for integrations, that don't connect to a device or service, to use a config flow. (Which reminds me to finish the write-up of the architectural discussion for that). It is the same reason you see things like: Sun, Moon, Season not being available via the UI at this point.

@basnijholt
Copy link
Copy Markdown
Contributor Author

Thanks a lot for your replies and for allowing me to reply here again (the PR was locked).

I have thought about it and decided to not pursue getting this PR merged, for a few reasons

  • lack of momentum, it took about 6 months for someone from core to take a look at the code
  • I get the impression that the current design with UI + YAML is not acceptable (even though @balloob first suggested to me to add a config flow 😄 )
  • I am quite busy with other things at the moment (work+moving) and given the above two points, I imagine it will take many more months to get this into a mergeable state (also considering the 55 issues here (not all bugs but some have to be addressed)).

For now, I recommend everyone to just use this as a custom_component via https://github.com/basnijholt/adaptive-lighting, I plan to keep fixing all urgent bugs quickly. I and many others are already successfully using this component to control a lot of lights for many months now! 💡:tada:

I might make another attempt to add this feature to core at some later point.

I would like to thank everyone for the feedback (this is one of the most commented PR in this repo ever) and for trying out this component! 🙌

@basnijholt basnijholt closed this May 28, 2021
@agilob
Copy link
Copy Markdown

agilob commented May 28, 2021

@basnijholt thanks for your work on this as a custom component. it's really shame it won't target core integration.

@claytonjn
Copy link
Copy Markdown
Contributor

@basnijholt It sounds like you basically ran into the same issues as I did with Circadian Lighting; I had come to the conclusion a couple years ago that it would need to more heavily integrated into the core of Home Assistant than simply an integration. I was actually pursuing that pre-Lovelace and pre-Integrations but knew it would need custom UI elements for control and custom configuration flow for it to really be an optimal experience. I was hopeful that the benefits of Lovelace and the Integration configuration flow would make the process easier for you with Adaptive Lighting but it seems that many of the same challenges still exist.

I've also been busy with personal things (new puppy, new job) so I'm only today finally able to work on updating Circadian Lighting for 2015.5.x, but maybe some time in the future when you and are aren't so busy and there is more interest from the Home Assistant team we could all work together to design and implement a native solution that adds the functionality so many users are looking for while also fitting into the Home Assistant UX ethos.

@RubenKelevra
Copy link
Copy Markdown

Lets continue the discussion in the forum:

https://community.home-assistant.io/t/adaptive-lighting-as-a-core-component/313016

@JeffPixelSplash
Copy link
Copy Markdown

Some feedback? I haven't played around with any of the existing similar options. I was interested in this one. but honestly, I couldn't figure out what it does nor how it might be used. From your examples, it creates a switch.... which does what? Right now I work in node-red and for the lights that I want to adapt to the time of day, I have manually programmed that. It seems like something like this ought to simplify my flows so I'm interested. But I simply don't understand what it does and how it's supposed to fit into existing automation/flows.

Somewhere near the top of the docs a simple example might be really helpful. Of course, the other option is that I'm just not getting it.

@danielbrunt57
Copy link
Copy Markdown

danielbrunt57 commented Dec 28, 2021

@JeffPixelSplash

Simply put: "The adaptive_lighting platform changes the settings of your lights throughout the day. It uses the position of the sun to calculate the color temperature and brightness that is most fitting for that time of the day."

I started with Circadian Lighting then changed to Adaptive Lighting but I am now using a custom Circadian routine in Node Red to automatically adjust light brightness and color... Circadian/Adaptive Lighting NodeRed

@JeffPixelSplash
Copy link
Copy Markdown

@danielbrunt57

Thank you. So I gather then this operates in the background and acts on any lights which are on when the switch is set? If that is correct then the configuration must be all about setting ranges and values for each light as needed?

As it sits right now, I'm handling each light event individually in Node Red. There's a time check in most of them and the appropriate brightness and color are set in flow variables and passed on to the service call to turn the lights on.

@broyuken
Copy link
Copy Markdown

Yes this changes the lights even if they are on and adjusts them every few minutes, both brightness and color temp (or only one) based on sunrise/sunset. There are a lot of options and makes things work nice and smoothly. It even has options to stop adjusting then if you have manually changed it to something else. For example if you make a light blue this will not adjust it the next time it goes to because it is set as manually adjusted.

@bcutter
Copy link
Copy Markdown

bcutter commented Dec 28, 2021

@danielbrunt57 Nice Bro. And what we're gonna do with that now - here, talking bout AL?!?

@danielbrunt57
Copy link
Copy Markdown

Just trying to help someone out.
Post deleted...

@vbaros
Copy link
Copy Markdown

vbaros commented Dec 28, 2021

Please do not mind bad comments, you will always have those. Please post it again. I just wanted to copy it from you.

@JeffPixelSplash
Copy link
Copy Markdown

Yes this changes the lights even if they are on and adjusts them every few minutes, both brightness and color temp (or only one) based on sunrise/sunset. There are a lot of options and makes things work nice and smoothly. It even has options to stop adjusting then if you have manually changed it to something else. For example if you make a light blue this will not adjust it the next time it goes to because it is set as manually adjusted.

Awesome. Thank you. NOW I understand what this does and yes, I'm super-excited to play with it. My current node-red flows don't adjust the temp and brightness on the fly. And, in general, it'd be nice to have the config for that in one centralized location rather than scattered in each flow for each light.

@bcutter
Copy link
Copy Markdown

bcutter commented Dec 28, 2021

Please do not mind bad comments, you will always have those. Please post it again. I just wanted to copy it from you.

Use the community forum for this:

https://community.home-assistant.io

@frenck
Copy link
Copy Markdown
Member

frenck commented Dec 28, 2021

I'm locking down this PR.

Please note, this is a PR! A closed one as well. PRs are where code reviews happen; none of today's comments are related to reviewing code.

If you want to discuss; please use our community forum or Discord chat.

Thanks 👍

@home-assistant home-assistant locked as off-topic and limited conversation to collaborators Dec 28, 2021
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.