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

Add samsungtv zeroconf discovery #35773

Closed
wants to merge 41 commits into from
Closed

Conversation

escoand
Copy link
Contributor

@escoand escoand commented May 18, 2020

Breaking change

Proposed change

Add samsungtv zeroconf discovery

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:

# Example configuration.yaml

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

@escoand
Copy link
Contributor Author

escoand commented May 20, 2020

@chemelli74 Are you brave enough to do a first test? I would expect that it's not working as custom_component, you need to checkout the complete HA code and run from there.

@chemelli74
Copy link
Contributor

@chemelli74 Are you brave enough to do a first test? I would expect that it's not working as custom_component, you need to checkout the complete HA code and run from there.

Why not, I have a docker test environment that is there exactly for those situations ;-)
Would be enough to git checkout overwriting current files ? or what ?

Simone

@escoand
Copy link
Contributor Author

escoand commented May 20, 2020

Not sure. Run it directly. But a docker build --tag HAtest . && docker run --rm HAtest should do it.

@escoand
Copy link
Contributor Author

escoand commented May 20, 2020

@chemelli74 I hope you had not too much work until now. There are many outstanding point. Please wait for my next message...

@escoand
Copy link
Contributor Author

escoand commented May 20, 2020

@chemelli74 as you can see there are some questions unclear, but now you could test it.

@chemelli74
Copy link
Contributor

@chemelli74 as you can see there are some questions unclear, but now you could test it.

I used files from previous commit:

2020-05-20 20:59:29 ERROR (MainThread) [homeassistant.core] Error doing job: Task exception was never retrieved
Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/data_entry_flow.py", line 129, in async_init
    flow, flow.init_step, data, init_done
  File "/usr/src/homeassistant/homeassistant/data_entry_flow.py", line 201, in _async_handle_step
    result: Dict = await getattr(flow, method)(user_input)
  File "/usr/src/homeassistant/homeassistant/components/samsungtv/config_flow.py", line 161, in async_step_zeroconf
    self._model = device_info.get("device", {}).get("modelName")
AttributeError: 'SamsungTVWSBridge' object has no attribute 'get'

Simone

@escoand
Copy link
Contributor Author

escoand commented May 20, 2020

Please use the most recently pushed code.

@chemelli74
Copy link
Contributor

Please use the most recently pushed code.

Updated, same result:

2020-05-21 00:10:29 ERROR (MainThread) [homeassistant.core] Error doing job: Task exception was never retrieved
Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/data_entry_flow.py", line 129, in async_init
    flow, flow.init_step, data, init_done
  File "/usr/src/homeassistant/homeassistant/data_entry_flow.py", line 201, in _async_handle_step
    result: Dict = await getattr(flow, method)(user_input)
  File "/usr/src/homeassistant/homeassistant/components/samsungtv/config_flow.py", line 161, in async_step_zeroconf
    self._model = device_info.get("device", {}).get("modelName")
AttributeError: 'SamsungTVWSBridge' object has no attribute 'get'

Simone

@pergolafabio
Copy link

Do you have an update for me ? @escoand ?

@chemelli74
Copy link
Contributor

Any chance to see this PR merged ?

Simone

@escoand
Copy link
Contributor Author

escoand commented Sep 3, 2020

It's still on my agenda, but with 3 kids and other things is also quiet busy. Family is definitely priority 1. Will work on it again when I have some minutes...

@pergolafabio
Copy link

But it's not finished yet to be merged since I have that "str" issue I posted earlier?

Take your time, I have 2 kids, couldn't imagine to have 3 ;)

@escoand
Copy link
Contributor Author

escoand commented Sep 3, 2020

@pergolafabio The difference from 0 to 1 is like a complete new life, but it's getting less and less diff with every new. In the hospital they said to us "3 is the new 2". ;-D

@chemelli74
Copy link
Contributor

But it's not finished yet to be merged since I have that "str" issue I posted earlier?

Take your time, I have 2 kids, couldn't imagine to have 3 ;)

Sorry, missed your comment.

Simone

@escoand
Copy link
Contributor Author

escoand commented Sep 8, 2020

@pergolafabio @chemelli74 Could you please retry with the last commit?

@pergolafabio
Copy link

ok, loaded patch 1
i had no samsung devices in HA
after restart, i also powered on my old non-tizen tv :

receive this error

2020-09-08 18:58:15 ERROR (MainThread) [homeassistant] Error doing job: Task exception was never retrieved
Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/data_entry_flow.py", line 128, in async_init
    result = await self._async_handle_step(
  File "/usr/src/homeassistant/homeassistant/data_entry_flow.py", line 201, in _async_handle_step
    result: Dict = await getattr(flow, method)(user_input)
  File "/config/custom_components/samsungtv/config_flow.py", line 189, in async_step_ssdp
    await self._abort_if_already_configured()
  File "/config/custom_components/samsungtv/config_flow.py", line 105, in _abort_if_already_configured
    self._host == entry.data[CONF_HOST]
KeyError: 'host'

also added my tizen TV, i got a popup on tv ... HA wants to access ... pressed allow ...
but again same error as above

@@ -612,6 +624,7 @@ async def test_autodetect_legacy(hass: HomeAssistantType, remote: Mock):
result = await hass.config_entries.flow.async_init(
DOMAIN, context={"source": "user"}, data=MOCK_USER_DATA
)
print(result)
Copy link
Member

Choose a reason for hiding this comment

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

Please remove debug print.


await self._abort_if_already_configured()

self.context["title_placeholders"] = {"model": self._model}
Copy link
Member

Choose a reason for hiding this comment

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

Is it ok to have model be None here?

self._name = f"Samsung {self._model}"
self._id = discovery_info.get(ATTR_UPNP_UDN)
self._title = self._model
async def async_step_ssdp(self, user_input=None):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
async def async_step_ssdp(self, user_input=None):
async def async_step_ssdp(self, discovery_info):

self.context["title_placeholders"] = {"model": self._model}
return await self.async_step_confirm()

async def async_step_zeroconf(self, user_input=None):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
async def async_step_zeroconf(self, user_input=None):
async def async_step_zeroconf(self, discovery_info):

self._mac = user_input[ATTR_PROPERTIES].get("deviceid")
self._manufacturer = user_input[ATTR_PROPERTIES].get("manufacturer")
if not self._model:
self._model = user_input[ATTR_PROPERTIES].get("model")
Copy link
Member

Choose a reason for hiding this comment

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

Don't use dict.get if we don't handle a None value returned.

@stale
Copy link

stale bot commented Oct 12, 2020

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale bot added the stale label Oct 12, 2020
@chemelli74
Copy link
Contributor

Prevent closing; will be a real pity with all the work done.

Simone

@stale stale bot removed the stale label Oct 12, 2020
@chemelli74
Copy link
Contributor

HI @escoand, I know your are very busy with real life and I don't want to sound rude.
I would just like to know if there is something I can do you help you getting this PR ready to merge.

Thank you again for your great work,

Simone

@escoand
Copy link
Contributor Author

escoand commented Nov 17, 2020

I'm also annoyed by the current situation. The code is quite matured but still not ready. The PR is still present in my head but I couldn't find the time and motivation to finalize it. When I have the time I will first work on this.

@balloob
Copy link
Member

balloob commented Jan 14, 2021

This PR seems to have gone stale. Closing it.

@chemelli74
Copy link
Contributor

@escoand, do you mind if I take over and try to finalize your work ? You did 99% is a pity to lost it.

Simone

@escoand
Copy link
Contributor Author

escoand commented Feb 22, 2021

@chemelli74 Yes sure, feel free to finalize it, thanks. For me it's currently really not possible specifically because I don't have a websocket device.

@chemelli74
Copy link
Contributor

@pergolafabio I would like to get in touch with you and complete the great work from @escoand.
Please look around for me on Discord

Simone

@pergolafabio
Copy link

ok, whats your ID?
i must say, i dont use the samsung integration, they are just added to HA as idle devices :-)
for now, i have put them to static IP with dhcp reservation, so i dont see the error at this moment

@chemelli74
Copy link
Contributor

ok, whats your ID?

chemelli74#2180

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants