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.

Sign up for GitHub

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

Jump to bottom

Add KEF speakers integration #28959

Merged
merged 14 commits into from Jan 3, 2020
Merged

Add KEF speakers integration #28959

merged 14 commits into from Jan 3, 2020

Conversation

basnijholt
Copy link
Contributor

@basnijholt basnijholt commented Nov 22, 2019 •
edited

Description:

This PR will add an integration to integrate KEF speakers.
This will work with the KEF LS50 Wireless and KEF LSX speakers.
The development of this code happened on basnijholt/media_player.kef and it uses the aiokef package.

Since these are reasonably expensive speakers (~$2300) there might not be many beta testers. However, on the community forum several people have tried to use HA with these speakers. Having it as a component inside HA (vs custom_component) will bring it to a bigger audience.

Pull request with documentation for home-assistant.io: home-assistant/home-assistant.io#11274

Example entry for configuration.yaml (if applicable):

media_player:
  - platform: kef
    host: 192.168.x.x  # the IP of your speaker
    type: LS50
    name: MyAudiophileSpeaker  # optional, the name in Home Assistant
    maximum_volume: 0.5  # optional, to avoid extremely loud volumes
    volume_step: 0.05  # optional

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.
  • I have followed the development checklist

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. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@project-bot project-bot bot added this to Needs review in Dev Nov 22, 2019
@basnijholt basnijholt force-pushed the kef_speakers branch 4 times, most recently from c04edc5 to b8812c3 Compare November 22, 2019 19:21
@basnijholt
Copy link
Contributor Author

basnijholt commented Nov 22, 2019 •
edited

The tests are failing right now because aiokef is Python 3.7 only. I thought that I heard that HA was Python 3.7 only now on the podcast?

Edit: I've made aiokef Python 3.6.

@basnijholt basnijholt closed this Nov 22, 2019
Dev automation moved this from Needs review to Cancelled Nov 22, 2019
@basnijholt basnijholt reopened this Nov 22, 2019
Dev automation moved this from Cancelled to Incoming Nov 22, 2019
@basnijholt basnijholt force-pushed the kef_speakers branch 2 times, most recently from d010dba to 56754d3 Compare November 23, 2019 00:52
@MartinHjelmare MartinHjelmare changed the title add KEF speakers platform for the integration Add KEF speakers integration Nov 23, 2019
@basnijholt basnijholt mentioned this pull request Nov 24, 2019
Merged
2 tasks
@basnijholt
Copy link
Contributor Author

basnijholt commented Nov 24, 2019 •
edited

@frenck, I see you added the 'docs-needed' label, however, I've just made a PR in home-assistant.io now 😄

@springstan
Copy link
Member

springstan commented Nov 24, 2019 •
edited

@basnijholt I think you should also tick the box with 'Documentation added/updated' then 😁

@basnijholt basnijholt force-pushed the kef_speakers branch 2 times, most recently from 87a49bc to 92b10a2 Compare November 24, 2019 15:18
Dev automation moved this from Incoming to Review in progress Nov 24, 2019
@basnijholt
Copy link
Contributor Author

basnijholt commented Nov 24, 2019 •
edited

@springstan, thanks, I fixed that.

🎉 all the tests are passing now!

@MartinHjelmare MartinHjelmare moved this from Incoming to Review in progress in Dev Dec 1, 2019
@arigit
Copy link
Contributor

arigit commented Dec 11, 2019

tested (via HACS) - works. Could this be merged so we can test in a pre-release

@basnijholt
Copy link
Contributor Author

@arigit, great to hear! Let me add (for the core devs) that the version in the HACS store has the same code as in this PR.

@frenck frenck removed their request for review December 12, 2019 14:10
MartinHjelmare
MartinHjelmare requested changes Dec 26, 2019
View reviewed changes
homeassistant/components/kef/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/kef/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/kef/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/kef/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/kef/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/kef/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/kef/media_player.py Outdated Show resolved Hide resolved
MartinHjelmare
MartinHjelmare approved these changes Jan 3, 2020
View reviewed changes
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Looks great!

@MartinHjelmare MartinHjelmare merged commit 0d5486f into home-assistant:dev Jan 3, 2020
Dev automation moved this from Review in progress to Done Jan 3, 2020
@basnijholt
Copy link
Contributor Author

@MartinHjelmare, thanks for the review and merging! 🎉

@lock lock bot locked and limited conversation to collaborators Jan 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@frenck frenck frenck left review comments

@springstan springstan springstan left review comments

@MartinHjelmare MartinHjelmare MartinHjelmare approved these changes

@pvizeli pvizeli Awaiting requested review from pvizeli

Assignees
No one assigned
Labels
cla-signed integration: kef new-integration
Projects
Dev
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@basnijholt @springstan @arigit @frenck @MartinHjelmare @pvizeli @homeassistant