Aohzan / ipx800

IPX800 V4 integration for Home-Assistant
Apache License 2.0
20 stars 12 forks source link

WIP: rewrite #6

Closed Aohzan closed 3 years ago

guigeek38 commented 3 years ago

Hello, I have been able to test this new branch for a few days. It works well, and the code seems more efficient.

However, I think I am getting a lot more error from the IPX : {'product': 'IPX800_V4', 'status': 'Error'} It seems to me that these errors occur when the IPX receives too many requests at the same time, but even with the same configuration in version 1.8.1, I had less frequent errors.

Even if it's error come from the IPX, isn't there a solution to work around the problem? For example, automatically retrying the request a second time, if it fails the first time? This would make the plugin much more stable.

Thank you again for your work!

Aohzan commented 3 years ago

Good idea, I will add this feature

Aohzan commented 3 years ago

done, I'm waiting for a solution to: https://community.home-assistant.io/t/state-not-keeping-updated-on-change/270891 and maybe the component will be ready to be integrated in home assistant

guigeek38 commented 3 years ago

Good idea, I will add this feature

Thank you very much ! After some testing, I suggested some modifications in a PR: https://github.com/Aohzan/pypx800/pull/1

guigeek38 commented 3 years ago

Just a question out of curiosity: Why did you rollback the commit "async for climate" (https://github.com/Aohzan/ipx800/pull/6/commits/d8023709019bd44d2aaaf07de36f9e40db9dd5d7)?

guigeek38 commented 3 years ago

done, I'm waiting for a solution to: https://community.home-assistant.io/t/state-not-keeping-updated-on-change/270891 and maybe the component will be ready to be integrated in home assistant

I did some testing to avoid this problem, and I think it comes from the HA Debouncing system. By default, the 1st time, the coordinator sends the request immediately (REQUEST_REFRESH_DEFAULT_IMMEDIATE), but the second time, it waits by default 10 seconds (REQUEST_REFRESH_DEFAULT_COOLDOWN). Except that during these 10 seconds, the state is restored in HA. https://github.com/home-assistant/core/blob/6b340415b2b6b3585d835f6e17aec7914fa29cd2/homeassistant/helpers/update_coordinator.py#L19

One of the solutions could be to redefine the Debouncer of the IpxDataUpdateCoordinator with a smaller cooldown: https://github.com/Aohzan/ipx800/blob/9050f82559f4f00d86a1ed4d356d9bdbc222cf37/custom_components/ipx800/__init__.py#L316

    def __init__(self, hass, ipx, update_interval):
        """Initialize."""
        _LOGGER.debug(
            "Define the coordinator to poll the IPX800 every %s seconds.",
            update_interval.seconds,
        )
        self.ipx = ipx
        super().__init__(
            hass,
            _LOGGER,
            name=DOMAIN,
            update_interval=update_interval,
            request_refresh_debouncer= Debouncer(
                hass,
                _LOGGER,
                cooldown=2,
                immediate=True,
                function=self.async_refresh,
            ))

However I am not sure that this solution is ideal, since it depends on the time that the request will take on the IPX (if it takes less than 2 seconds it's ok, otherwise the problem will occur again).

Thanks again for the work done, and I hope this component could be integrated in home assistant!

Aohzan commented 3 years ago

Just a question out of curiosity: Why did you rollback the commit "async for climate" (d802370)?

I got problems on my side, I have to check this

I did some testing to avoid this problem, and I think it comes from the HA Debouncing system

Thanks, I will test it

Aohzan commented 3 years ago

@guigeek38 I rewrote the python package and the integration to use async http call and optimize some stuff, if you can test it :)

guigeek38 commented 3 years ago

Thank you for the work done for this new version!

I have a problem with the FP component, that will be quick to fix. You rewrote the names of presets modes using standard namming of Home assistant. However, the value returned next is that of the ipx, which does not have the same name as Home assistant. So, There is a discrepancy between the list of available presets and the presets returned by the ipx, which poses problems in the management of states in home assistant. (comfort / away / ... vs Confort / Hors Gel, ....)

In the development tools I can see:

preset_modes:
  - comfort
  - eco
  - away
  - none
  - comfort -1
  - comfort -2
preset_mode: Hors Gel

I'm continuing the tests to let you know if I see any problems.

Aohzan commented 3 years ago

So, There is a discrepancy between the list of available presets and the presets returned by the ipx, which poses problems in the management of states in home assistant. (comfort / away / ... vs Confort / Hors Gel, ....)

Oh sorry, I will fix it

Aohzan commented 3 years ago

@guigeek38 Normally fixed here : https://github.com/Aohzan/ipx800/pull/6/commits/7a188f74287ada72fb722f607ea26fc191b6f4a2