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

Philips TV ambilight support #44867

Merged
merged 9 commits into from
May 31, 2021
Merged

Conversation

elupus
Copy link
Contributor

@elupus elupus commented Jan 6, 2021

Proposed change

  • Add support for ambilight control

Bump of dependency: danielperna84/ha-philipsjs@2.7.3...2.7.4

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

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:

@elupus
Copy link
Contributor Author

elupus commented Jan 9, 2021

Should be done for start of review. I still need to update docs.

homeassistant/components/philips_js/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/philips_js/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/philips_js/device_trigger.py Outdated Show resolved Hide resolved
homeassistant/components/philips_js/device_trigger.py Outdated Show resolved Hide resolved
homeassistant/components/philips_js/light.py Outdated Show resolved Hide resolved
homeassistant/components/philips_js/light.py Outdated Show resolved Hide resolved
homeassistant/components/philips_js/light.py Outdated Show resolved Hide resolved
homeassistant/components/philips_js/media_player.py Outdated Show resolved Hide resolved
tests/components/philips_js/test_device_trigger.py Outdated Show resolved Hide resolved
tests/components/philips_js/test_device_trigger.py Outdated Show resolved Hide resolved
Copy link
Contributor

@joncar joncar left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@Clooos
Copy link

Clooos commented Feb 28, 2021

I've just tried it and it's working great for now! Like others Ambilight component it just can't turn on the light when the TV is in deep sleep (or am I missing something?) but with Wakelock app it's working. I have tried transitions but it's not working too, the color changing is instant. Anyway it's great to see an official component for our Ambilight TV! It's just missing a switch for the Ambilight+Hue, I've made one myself but I'm not really a dev. Sorry for my approximate english and thank you again for your work!

@chpego
Copy link
Contributor

chpego commented Feb 28, 2021

@elupus thank you very much for your work. i've tried and for me it's works for only color, but i can't see the Lounge Light HOT_LAVA, DEEP_WATER and FRESH_NATURE effects on the list and, i don't know why. Do you need something to debug ? I'm using a Philips 55oled804/12 with api 6 version

@elupus
Copy link
Contributor Author

elupus commented Mar 1, 2021

At the moment it is using the old ambilight control api that is available for v1. Annoyingly they had to go and invent new stuff for v6.

I will likely add support for those styles in a later pull.

@chpego
Copy link
Contributor

chpego commented Mar 1, 2021

Thank you @elupus ... i mentionned an issue on the other github project, i've made some modification, maybe you can (re)use to include on ha-philipsjs ?

one more time, nice job 👌👌👌

@elupus
Copy link
Contributor Author

elupus commented Mar 1, 2021

ha-philipsjs already have support for getting styles. Just not used here yet. It need to fully replace the old logic if the system support it.

Ps. I don't know which other project you mean.

@Clooos
Copy link

Clooos commented Mar 1, 2021

I have found another "issue", I don't know if it's a normal behavior but when I change the color to pure red (255, 0, 0), the color on the Ambilight is orange, I can live with it but I prefer to share this with you 🙂

@elupus
Copy link
Contributor Author

elupus commented Mar 1, 2021

I have found another "issue", I don't know if it's a normal behavior but when I change the color to pure red (255, 0, 0), the color on the Ambilight is orange, I can live with it but I prefer to share this with you 🙂

That is really odd. Does it snap to orange? That is, something really close to pure orange is still red:er?

@elupus
Copy link
Contributor Author

elupus commented May 8, 2021

I finally managed to understand why i wasn't able to restore ambilight mode to follow video properly. To restore, one need to turn it to off state, wait for a transition effect to off, then turn it to follow video again.

If you are too quick. The off transition gets aborted half way, and get stuck.

So.. it should now turn off properly and switch to follow video. I will try to implement a restoring off mode later.

Also, I've reduced TV hammering of ambilight data, that seemed to crash it. I will backport that.

@aegjoyce
Copy link

Sorry, switch off still not working on OLED804. Fades off, then after a second does a little pulse, then a few seconds later comes on again at dim brightness...

@aegjoyce
Copy link

This looks good! Turns on and off correctly, and the other tweaks seem to make it really responsive. I'm impressed.

Any particular things you want testing? I think the only thing I would look to add would be some sort of lookup dictionary so that the colour modes are labelled in a friendlier way (matching up with the built-in names on the TV) and without some of the duplicate modes.

Functionally though it's excellent!

Copy link
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

Looks good! Left a couple of comments.

@aegjoyce
Copy link

These latest changes have broken something. Re-installed the component from scratch using the new commits, it can't add the light entity:

Logger: homeassistant.components.light
Source: custom_components/philips_js/light.py:62 
Integration: Light (documentation, issues) 
First occurred: 12:25:17 (2 occurrences) 
Last logged: 12:25:17

Error adding entities for domain light with platform philips_js
Error while setting up philips_js platform for light
Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/helpers/entity_platform.py", line 364, in async_add_entities
    await asyncio.gather(*tasks)
  File "/usr/src/homeassistant/homeassistant/helpers/entity_platform.py", line 558, in _async_add_entity
    await entity.add_to_platform_finish()
  File "/usr/src/homeassistant/homeassistant/helpers/entity.py", line 581, in add_to_platform_finish
    self.async_write_ha_state()
  File "/usr/src/homeassistant/homeassistant/helpers/entity.py", line 334, in async_write_ha_state
    self._async_write_ha_state()
  File "/usr/src/homeassistant/homeassistant/helpers/entity.py", line 368, in _async_write_ha_state
    state = self._stringify_state()
  File "/usr/src/homeassistant/homeassistant/helpers/entity.py", line 340, in _stringify_state
    state = self.state
  File "/usr/src/homeassistant/homeassistant/helpers/entity.py", line 738, in state
    return STATE_ON if self.is_on else STATE_OFF
  File "/config/custom_components/philips_js/light.py", line 232, in is_on
    mode, style, _ = _parse_effect(self.effect)
  File "/config/custom_components/philips_js/light.py", line 62, in _parse_effect
    style, _, algorithm = effect.partition(EFFECT_PARTITION)
AttributeError: 'NoneType' object has no attribute 'partition'

@aegjoyce
Copy link

Can confirm that reverting back to 31c71d2 fixes the issue.

@elupus
Copy link
Contributor Author

elupus commented May 31, 2021

I missed something in the upgrade to attributes. Will fix later.

@elupus
Copy link
Contributor Author

elupus commented May 31, 2021

Oh right. you need lasted dev branch for those things to work properly. It's not fault in the pull request. Just won't work with older home assistant versions.

@balloob balloob merged commit 6631a4e into home-assistant:dev May 31, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Jun 1, 2021
@elupus elupus deleted the philips_js_light branch June 4, 2021 22:16
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.

None yet

8 participants