JurajNyiri / HomeAssistant-Atrea

Custom component allowing control of Atrea ventilation units
Apache License 2.0
23 stars 12 forks source link

Duplex 390 support #3

Closed omikr0n closed 3 years ago

omikr0n commented 3 years ago

Fully tested Duplex 390 support. Requires changes from https://github.com/JurajNyiri/pyatrea/pull/3 I will only merge this PR after bumping pyatrea version.

JurajNyiri commented 3 years ago

I also started seeing this:

Traceback (most recent call last):
  File "/usr/local/lib/python3.8/site-packages/homeassistant/helpers/entity_platform.py", line 315, in async_add_entities
    await asyncio.gather(*tasks)
  File "/usr/local/lib/python3.8/site-packages/homeassistant/helpers/entity_platform.py", line 506, in _async_add_entity
    await entity.add_to_platform_finish()
  File "/usr/local/lib/python3.8/site-packages/homeassistant/helpers/entity.py", line 531, in add_to_platform_finish
    self.async_write_ha_state()
  File "/usr/local/lib/python3.8/site-packages/homeassistant/helpers/entity.py", line 296, in async_write_ha_state
    self._async_write_ha_state()
  File "/usr/local/lib/python3.8/site-packages/homeassistant/helpers/entity.py", line 322, in _async_write_ha_state
    attr.update(self.state_attributes or {})
  File "/usr/local/lib/python3.8/site-packages/homeassistant/components/climate/__init__.py", line 221, in state_attributes
    self.current_temperature,
  File "/Users/jurajnyiri/.homeassistant/custom_components/atrea/climate.py", line 261, in current_temperature
    return float(self._inside_temp)
ValueError: could not convert string to float: ''
2021-02-09 20:57:55 ERROR (MainThread) [homeassistant.components.climate] Error while setting up atrea platform for climate
Traceback (most recent call last):
  File "/usr/local/lib/python3.8/site-packages/homeassistant/helpers/entity_platform.py", line 206, in _async_setup_platform
    await asyncio.gather(*pending)
  File "/usr/local/lib/python3.8/site-packages/homeassistant/helpers/entity_platform.py", line 315, in async_add_entities
    await asyncio.gather(*tasks)
  File "/usr/local/lib/python3.8/site-packages/homeassistant/helpers/entity_platform.py", line 506, in _async_add_entity
    await entity.add_to_platform_finish()
  File "/usr/local/lib/python3.8/site-packages/homeassistant/helpers/entity.py", line 531, in add_to_platform_finish
    self.async_write_ha_state()
  File "/usr/local/lib/python3.8/site-packages/homeassistant/helpers/entity.py", line 296, in async_write_ha_state
    self._async_write_ha_state()
  File "/usr/local/lib/python3.8/site-packages/homeassistant/helpers/entity.py", line 322, in _async_write_ha_state
    attr.update(self.state_attributes or {})
  File "/usr/local/lib/python3.8/site-packages/homeassistant/components/climate/__init__.py", line 221, in state_attributes
    self.current_temperature,
  File "/Users/jurajnyiri/.homeassistant/custom_components/atrea/climate.py", line 261, in current_temperature
    return float(self._inside_temp)
ValueError: could not convert string to float: ''

Currently working on other integration but this popped up with this code, just noting it here until I get to it.

JurajNyiri commented 3 years ago

I was not able to replicate above again.

Can we please set defaults as follow just in case?

        self._outside_temp = 0
        self._inside_temp = 0
        self._supply_air_temp = 0
        self._requested_temp = 0
omikr0n commented 3 years ago

I've set default numeric values for temperatures and also set self._requested_power to None which makes more sense to me than an empty string.

omikr0n commented 3 years ago

Regarding using status['...'] versus self.atrea.getValue('...') - that was intentional on my part to preserve backwards compatibility with previously supported units as much as possible. Case in point is the logic for setting _outside_temp in manualUpdate():

if('I10202' in status):
  if(float(status['I10202']) > 6550):
      self._outside_temp = (float(status['I10202'])-6550)/10 * -1
  else:
      self._outside_temp = float(status['I10202'])/10

Because status[] gets a raw value, but getValue() applies multiplicative and additive constants (coefs and offsets from params), there is a risk that by changing this particular code to use getValue(), coefs and offsets would be applied twice (once in getValue() and once in manualUpdate()), if the unit provided correct descriptions in params. I know you mentioned that your unit does not declare some variables in params correctly, so this would probably need to be tackled on case-by-case basis.

I'm more than happy to change everything to getValue() for code consistency, but you'd need to let me know which variables does your unit declare in params with correct coefs and offsets and which it doesn't. There is also a slight chance that we would break other people with slightly different units (with regards to their params) who might already use this code.

JurajNyiri commented 3 years ago

Looking good! I will test this as soon as it stops freezing as my unit refuses to operate with "2nd Frost protection" warning (I will make it unavailable when this warning is present in next release)

JurajNyiri commented 3 years ago

Tested, works, great work!!

omikr0n commented 3 years ago

Perfect, thank you!