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

Broadlink service name #16345

Merged
merged 2 commits into from
Sep 24, 2018
Merged

Broadlink service name #16345

merged 2 commits into from
Sep 24, 2018

Conversation

Danielhiversen
Copy link
Member

Description:

Should fix #15067, but is not tested.
And no response from the issue opner

@elupus
Copy link
Contributor

elupus commented Sep 1, 2018

Cant you switch it to an entity name instead? IP addresses is often not fixed.

@Danielhiversen
Copy link
Member Author

The correct ip address/hostname needs to be specified for the connection to work

@elupus
Copy link
Contributor

elupus commented Sep 1, 2018

Absolutly, but the service name doesn't need to include this.

@Danielhiversen
Copy link
Member Author

That is correct. But I do not think it is worth a breaking change.

@elupus
Copy link
Contributor

elupus commented Sep 1, 2018

Accepting dns name would mitigate it without a breaking change I suppose.

Copy link
Member

@pvizeli pvizeli left a comment

Choose a reason for hiding this comment

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

We should not make a service per device. The device or entity_id should be a option and internal inside the handler should be call the correct device. @balloob ?

@pvizeli pvizeli requested a review from balloob September 3, 2018 10:35
@Danielhiversen
Copy link
Member Author

Good point.
Might be worth changing to that, even if it will be a breaking change?
(I currently do not have any Broadlink components available, so I do not want to do too much changes that I can not test. And it seems to be hard to find other users willing to test changes here.)

@elupus
Copy link
Contributor

elupus commented Sep 3, 2018 via email

@balloob
Copy link
Member

balloob commented Sep 24, 2018

I agree that this should be moved to a single service, but I think that this fix for now is fine.

@balloob balloob merged commit 1f74ada into dev Sep 24, 2018
@balloob balloob deleted the broadlink branch September 24, 2018 09:10
@ghost ghost removed the in progress label Sep 24, 2018
@balloob balloob mentioned this pull request Sep 28, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Feb 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

switch.broadlink component creates invalid services when configured using hostname
6 participants