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

Implement more Comfoconnect sensors #28817

Merged

Conversation

@michaelarnauts
Copy link
Contributor

michaelarnauts commented Nov 16, 2019

Description:

  • Added schema validation.
  • Moved ATTR_* definitions from __init__.py to sensor.py since they were only used there.
  • Store unique_id var in ComfoConnectBridge() so I can use that to generate a unique_id for the fan and sensors.
  • Converted SENSOR_TYPES to a dict with keys instead of indexes.
  • Added device_class to sensors where it makes sense.
  • Moved multiplier to SENSOR_TYPES and apply it in _handle_update.
  • Added new sensors. I will still have to update the docs for this.
    • bypass_state
    • days_to_replace_filter
    • exhaust_fan_duty
    • exhaust_fan_speed
    • exhaust_humidity
    • exhaust_temperature
    • power_usage
    • supply_fan_duty
    • supply_fan_speed
    • supply_humidity
    • supply_temperature

Related issue (if applicable): fixes #27846

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#11216

Example entry for configuration.yaml (if applicable):

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

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. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
@project-bot project-bot bot added this to Needs review in Dev Nov 16, 2019
@project-bot project-bot bot moved this from Needs review to By Code Owner in Dev Nov 16, 2019
@michaelarnauts

This comment has been minimized.

Copy link
Contributor Author

michaelarnauts commented Nov 16, 2019

There is one thing that could be improved, but might be a bit strange.

All sensors now register for SIGNAL_COMFOCONNECT_UPDATE_RECEIVED and check the var to see if it is an update for them.

I could make them register on SIGNAL_COMFOCONNECT_UPDATE_RECEIVED + sensor_type, so I don't need to do that check. Is that something that is okay?

It would mean something like this:

diff --git a/homeassistant/components/comfoconnect/__init__.py b/homeassistant/components/comfoconnect/__init__.py
index 569965b3e..4964af5ed 100644
--- a/homeassistant/components/comfoconnect/__init__.py
+++ b/homeassistant/components/comfoconnect/__init__.py
@@ -115,4 +115,4 @@ class ComfoConnectBridge:
 
     def sensor_callback(self, var, value):
         """Notify listeners that we have received an update."""
-        dispatcher_send(self.hass, SIGNAL_COMFOCONNECT_UPDATE_RECEIVED, var, value)
+        dispatcher_send(self.hass, SIGNAL_COMFOCONNECT_UPDATE_RECEIVED + var, value)
diff --git a/homeassistant/components/comfoconnect/fan.py b/homeassistant/components/comfoconnect/fan.py
index 66b2ffc5c..60a5fdaaf 100644
--- a/homeassistant/components/comfoconnect/fan.py
+++ b/homeassistant/components/comfoconnect/fan.py
@@ -47,15 +47,13 @@ class ComfoConnectFan(FanEntity):
             self._ccb.comfoconnect.register_sensor, SENSOR_FAN_SPEED_MODE
         )
         async_dispatcher_connect(
-            self.hass, SIGNAL_COMFOCONNECT_UPDATE_RECEIVED, self._handle_update
+            self.hass, SIGNAL_COMFOCONNECT_UPDATE_RECEIVED + SENSOR_FAN_SPEED_MODE, self._handle_update
         )
 
-    def _handle_update(self, var, value):
+    def _handle_update(self, value):
         """Handle update callbacks."""
-        if var != SENSOR_FAN_SPEED_MODE:
-            return
-        _LOGGER.debug("Received update for %s: %s", var, value)
-        self._ccb.data[var] = value
+        _LOGGER.debug("Received update for %s: %s", SENSOR_FAN_SPEED_MODE, value)
+        self._ccb.data[SENSOR_FAN_SPEED_MODE] = value
         self.schedule_update_ha_state()
 
     @property
diff --git a/homeassistant/components/comfoconnect/sensor.py b/homeassistant/components/comfoconnect/sensor.py
index 23b7c0e2c..f1ad7b67f 100644
--- a/homeassistant/components/comfoconnect/sensor.py
+++ b/homeassistant/components/comfoconnect/sensor.py
@@ -232,15 +232,13 @@ class ComfoConnectSensor(Entity):
             self._ccb.comfoconnect.register_sensor, self._sensor_id
         )
         async_dispatcher_connect(
-            self.hass, SIGNAL_COMFOCONNECT_UPDATE_RECEIVED, self._handle_update
+            self.hass, SIGNAL_COMFOCONNECT_UPDATE_RECEIVED + self._sensor_id, self._handle_update
         )
 
-    def _handle_update(self, var, value):
+    def _handle_update(self, value):
         """Handle update callbacks."""
-        if var != self._sensor_id:
-            return
-        _LOGGER.debug("Received update for %s: %s", var, value)
-        self._ccb.data[var] = round(
+        _LOGGER.debug("Received update for %s: %s", self._sensor_id, value)
+        self._ccb.data[self._sensor_id] = round(
             value * SENSOR_TYPES[self._sensor_type].get(ATTR_MULTIPLIER, 1), 2
         )
         self.schedule_update_ha_state()
@michaelarnauts

This comment has been minimized.

Copy link
Contributor Author

michaelarnauts commented Nov 16, 2019

I just grepped trough the code, and this seems to be quite common. This is a nice cleanup IMO.

@michaelarnauts

This comment has been minimized.

Copy link
Contributor Author

michaelarnauts commented Nov 16, 2019

@MartinHjelmare I'm noticing a race condition now, but I think it's not related to these changes. I just notice it now since I have more sensors.

With this code, it seems to happen that I get a response back in the callback function, but the dispatcher isn't setup yet. Therefore, I miss that callback.

    async def async_added_to_hass(self):
        """Register for sensor updates."""
        _LOGGER.debug("Registering for fan speed")
        await self.hass.async_add_executor_job(
            self._ccb.comfoconnect.register_sensor, SENSOR_FAN_SPEED_MODE
        )
        async_dispatcher_connect(
            self.hass,
            SIGNAL_COMFOCONNECT_UPDATE_RECEIVED.format(SENSOR_FAN_SPEED_MODE),
            self._handle_update,
        )

Is there a way to connect the dispatcher first, and then do the register? I tried to swap the two statements, but that didn't help...

@michaelarnauts michaelarnauts changed the title Comfoconnect sensors WIP: Comfoconnect sensors Nov 16, 2019
@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Nov 16, 2019

Does it matter if we miss callbacks before the entity is ready? Before the entity is ready the entity can't react anyway.

@michaelarnauts

This comment has been minimized.

Copy link
Contributor Author

michaelarnauts commented Nov 16, 2019

The issue is that the bridge sends the update almost immediately achter the register, before the callback is registered. I did some more testing after switching the register and the dispatcher_connect, and I can't reproduce the issue anymore. I will do some more testing to be sure.

@michaelarnauts

This comment has been minimized.

Copy link
Contributor Author

michaelarnauts commented Nov 16, 2019

Some updates are only send once (since they don't change) so if we miss them, our entity state will stay Unknown.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Nov 16, 2019

async_dispatcher_connect, is a callback and it doesn't schedule anything so its function will be done when it returns.

@michaelarnauts

This comment has been minimized.

Copy link
Contributor Author

michaelarnauts commented Nov 16, 2019

Indeed, I think I made a mistake during my previous tests. It seems to work nicely now. The async_dispatcher_connect had to go first for it to work though.

For me, this PR is ready.

@michaelarnauts michaelarnauts changed the title WIP: Comfoconnect sensors Comfoconnect sensors Nov 16, 2019
@michaelarnauts michaelarnauts changed the title Comfoconnect sensors Implement more Comfoconnect sensors Nov 16, 2019
Dev automation moved this from By Code Owner to Review in progress Nov 17, 2019
@michaelarnauts michaelarnauts mentioned this pull request Nov 17, 2019
2 of 2 tasks complete
@michaelarnauts

This comment has been minimized.

Copy link
Contributor Author

michaelarnauts commented Nov 17, 2019

Documentation update is in home-assistant/home-assistant.io#11216

Dev automation moved this from Review in progress to Reviewer approved Nov 17, 2019
@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Nov 17, 2019

Good!

@MartinHjelmare MartinHjelmare merged commit 3d5b007 into home-assistant:dev Nov 17, 2019
11 checks passed
11 checks passed
CI Build #20191117.24 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch Coverage not affected when comparing d6e99db...8d8b8e8
Details
codecov/project 94.44% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Nov 17, 2019
@michaelarnauts

This comment has been minimized.

Copy link
Contributor Author

michaelarnauts commented Nov 18, 2019

Thanks @MartinHjelmare for the follow-up!

@michaelarnauts michaelarnauts deleted the michaelarnauts:comfoconnect-sensors branch Nov 18, 2019
@lock lock bot locked and limited conversation to collaborators Nov 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
3 participants
You can’t perform that action at this time.