bushvin / hass-integrations

My custom Home Assistant integrations
Apache License 2.0
63 stars 11 forks source link

#4 #5 fix warnings and zeroconf on docker #6

Closed guix77 closed 3 years ago

guix77 commented 3 years ago

Hi,

First thanks a lot for this integration !

Here is a proposal to fix #4 and #5.

For #4, actually Mopidy publishes the "_mopidy-http._tcp.local." zeroconf so it's a good way to detect a Mopidy instance without trying to connect. I also removed the warnings in the logger which should not have any more sense now.

For #5, the problem has been discussed a lot of times in Home Assistant integrations. Either the zeroconf service publishes an IP or a FQDN other than .local, in which case it can be used, either we just get the IP. Even better, just after getting the IP, we reverse resolve it into the FQDN so the configuration remains stable even with an IP change of the device.

Thanks!

bushvin commented 3 years ago

Hey,

Thanks for figuring this out and submitting this.

I wasn't aware of the '_mopidy-http._tcp.local.' zeroconf endpoint, but that would mean it needs to go into manifest.json to make it clean, and auto-available for HA.

One issue I can see with the reverse lookup is when you do not have a dedicated DNS server, and solely rely on zeroconf. Granted, if you have a docker instance you're probably skilled enough to have one. But I think there should be a fallback just in case, what do you think?

guix77 commented 3 years ago

Hi!

Good catch, in fact I already had _mopidy-http._tcp.local. in manifest.json but I forgot to commit it, since I'm developing outside of the repository.

You're right too about the reverse lookup. My internet box auto provides a .home network and I didn't think about it. In a Docker scenario, we could try to reverse lookup and just use the IP if there is no resolution, which is not ideal but still better than nothing. In a non-Docker scenario, it would however be best to keep the .local one. However I can't see how to easily combine both scenari, except with a configuration which would kind of defeat the zeroconf stuff. What do yo think ?

guix77 commented 3 years ago

In fact we could probably do something like this:

  1. try to resolve the .local address (no Docker scenario)
  2. if it fails, try to reverse resolve the IP to a FQDN (Docker scenario with DNS)
  3. fallback on IP in last resort

I'm gonna try this!

bushvin commented 3 years ago

In fact we could probably do something like this:

1. try to resolve the .local address (no Docker scenario)

2. if it fails, try to reverse resolve the IP to a FQDN (Docker scenario with DNS)

3. fallback on IP in last resort

This is the way! (pun intended)

guix77 commented 3 years ago

Done, however I can't test the cases 1 and 3. In which case are you ?

bushvin commented 3 years ago

Thank you for bearing with me! I really appreciate it.

Just noticed the displayname for devices are a bit awkward due to zeroconf. Mine are named like this: figrindan_mopidy_http_tcp_local. See what I can find

bushvin commented 3 years ago

You changed self._name = discovery_info["properties"].get("name", self._host) to self._name = discovery_info[CONF_NAME]

Is there any particular reason for that?

I'm considering changing it to

self._name = discovery_info["properties"].get("name", self._host) + "@" + str(self._port)

Would that break things on your end?

guix77 commented 3 years ago

Thank you for bearing with me! I really appreciate it.

No worries, the more we are to look at the code, the better it is, thank you!

Just noticed the displayname for devices are a bit awkward due to zeroconf. Mine are named like this: figrindan_mopidy_http_tcp_local. See what I can find

For me too.

guix77 commented 3 years ago

You changed self._name = discovery_info["properties"].get("name", self._host) to self._name = discovery_info[CONF_NAME]

Is there any particular reason for that?

I thought there was nothing in discovery_info["properties"].get("name") and I wanted to take advantage of the Mopidy server name as defined in the zeroconf key in the http section of mopidy.conf. But it's appending the *_mopidy_http_tcp_local anyways, which is not great. Also, I'm not sure why the entity_id does not generate from self._uuid but from self._name, but it must be a HASS thing.

I'm considering changing it to

self._name = discovery_info["properties"].get("name", self._host) + "@" + str(self._port)

Would that break things on your end?

I think it's great :)

bushvin commented 3 years ago

How about self._name = discovery_info[CONF_NAME][:-22] to strip off _mopidy_http_tcp_local ? That'smaybe even better!

guix77 commented 3 years ago

Sure!

bushvin commented 3 years ago

There, I've updated the code so the name is supposed to be the real zeroconf name with port number and I've cleaned up the code so it's conform with the HA style guide.

Merci pour votre contribution!

guix77 commented 3 years ago

I've pulled the latest code and it's really great like this.

Avec plaisir !