Add Elk-M1 sensor platform by gwww · Pull Request #17342 · home-assistant/core

gwww · 2018-10-11T23:32:55Z

Description:

See component PR for full details: #16952

Related issue (if applicable): fixes #

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

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

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.io

If the code communicates with devices, web services, or third-party tools:

New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

If the code does not interact with devices:

Tests have been added to verify that the new code works.

MartinHjelmare · 2018-10-16T19:23:14Z

homeassistant/components/sensor/elkm1.py

+For more details about this platform, please refer to the documentation at
+https://home-assistant.io/components/sensor.elkm1/
+"""
+


Remove blank line.

This is still left.

MartinHjelmare · 2018-10-16T19:29:01Z

homeassistant/components/sensor/elkm1.py

+        args = (service.data.get('number'))
+        _dispatch(SIGNAL_SPEAK_PHRASE, entity_ids, *args)
+
+    hass.services.async_register(DOMAIN, 'elkm1_sensor_speak_word',


Sensors are up until now read only, so we have never implemented any services for these platforms. Probably move the services to the elkm1 component. This is an architecture question.

I don't really see why these services belong to the sensor platform in the first place. "speak word" and "speak phrase" doesn't sound like something a sensor would do. More like a speaker. But I don't know this integration.

I can move the services. They seemed to be a better fit against the alarm panel rather than the entire config but serious don't know or care much as long as they can be easily invoked.

I suggest moving them to the component.

When you say move to the component, is that elkm1/__init__.py async_setup? Would I register the service as elkm1.speak_word, for example?

MartinHjelmare · 2018-10-16T19:40:40Z

homeassistant/components/sensor/elkm1.py

+    def device_state_attributes(self):
+        """Attributes of the sensor."""
+        attrs = self.initial_attrs()
+        attrs['elkm1_version'] = self._element.elkm1_version


Firmware version and similar info should go in device info, not state attributes. State attributes should hold data connected to the state of the device. device_info property requires config entries though so it's not super easy to implement. I suggest removing the version attributes for now.

https://developers.home-assistant.io/docs/en/device_registry_index.html

I've read those docs and was trying to figure out how it could be used with ElkM1. Not very clear at all how to do that integration. Some examples would be helpful in the docs. config_entries not very clear at all either. Does that mean everything that's in the elk section of configuration.yaml goes away?

So what to do? I'd like to keep the versions, implementing something else way over the top (to use your words "not super easy to implement").

What about the state associated with this entity? It tracks the state of the connection from hass to the actual platform. Is that in device_info?

We make a distinction between dynamic info and static info. Dynamic info belongs in state and state attributes as long as it pertains to the state of a device. Static info is... static so doesn't have to do with state of the device.

Static info like version has to be removed from state attributes.

If the connection has multiple states it can be in, then it fits as sensor state.

MartinHjelmare · 2018-10-16T19:42:13Z

homeassistant/components/sensor/elkm1.py

+        attrs = self.initial_attrs()
+        attrs['elkm1_version'] = self._element.elkm1_version
+        attrs['system_trouble_status'] = self._element.system_trouble_status
+        attrs['xep_version'] = self._element.xep_version


What is xep_version?

It is the version of the Ethernet controller.

Ok, then it has to be removed too.

MartinHjelmare · 2018-10-16T19:44:19Z

homeassistant/components/sensor/elkm1.py

+        return attrs
+
+
+class ElkThermostat(ElkSensor):


Is this a pure read only integration, no temperature control? It should be a sensor and not a climate entity?

I'm sure I read, but cannot find right now, that each of the sensors of a device should be represented as separate entities. Thus this thermostat entity. Perhaps @BioSehnsucht can comment as I'm guessing he read the same thing from which code followed.

The next PR is for climate in which case the temperature will be available from that. Delete?

If this thermostat reading is the same device feature that will go in the climate entity, yes then remove this sensor type.

TL;DR : I think it's fine to remove the ElkThermostat sensors for now. When we get Omni protocol re-implemented to support the additional sensors, we can add those additional sensors back in.

At one time I had implemented the necessary protocol-within-a-protocol to be able to gather additional temperature sensors from Elk supported climate devices (specifically HAI Omnistat devices) which could have up to 4 temperature sensors in total (one of which was the thermostat itself). However at the moment we haven't re-implemented that again after switching from my library to @gwww 's so thermostat sensors are redundant, and once we re-implement the protocol inception necessary to support it, we can revisit and add just the 2nd-4th temp sensors as individual sensors. (In actuality, at the time, the thermostat sensor that was part of the climate entity was duplicated as a sensor because reasons ... so I always had duplicated sensors for climate)

homeassistant/components/sensor/elkm1.py

MartinHjelmare · 2018-10-16T19:49:06Z

homeassistant/components/sensor/elkm1.py

+"""
+Support for control of ElkM1 sensors.
+
+On the ElkM1 there are 6 types of sensors:


Move this to the docs.

gwww · 2018-10-16T20:42:14Z

Thank you for the review! Appreciate it.

tjorim · 2018-10-17T17:58:03Z

homeassistant/components/elkm1/__init__.py

+                     'switch']
+
+SPEAK_SERVICE_SCHEMA = vol.Schema({
+    vol.Required('number'): vol.Range(min=0, max=999),


Do not validate only the range, but also the type (use float if necessary).

vol.Required('number'): vol.All(vol.Coerce(int), vol.Range(min=0, max=999)),

MartinHjelmare · 2018-10-17T20:38:55Z

homeassistant/components/services.yaml

        example: trip_back_home
+
+elkm1:
+  sensor_speak_word:


Move these to a new file services.yaml in the elkm1 package. See other components that are packages for example.

Will do. Don't understand why though. What is different about elkm1 compared to the other services in where they were put?

We don't want to add more service descriptions to the base component package file.

The descriptions in the base component file are always loaded and cached the first time we need all the service descriptions of registered services, eg in the frontend, so the longer that file is the longer time it will take to do that, and the more information, potentially not needed info, will be stored in memory.

That's why we want to split this file up and put the descriptions in the other child components so that we only load the necessary information.

Thanks, that helps. Sounds like that the services will be eventually moved. Makes a lot of sense. Would it make sense to leave a "trail" in that services.yaml that says what you said in your comment? It could help point people in the right direction.

Make the suggestion in a PR. It should be documented in our dev docs at minimum.

MartinHjelmare · 2018-10-18T05:56:09Z

homeassistant/components/elkm1/services.yaml

@@ -0,0 +1,13 @@
+elkm1:


Remove this key and unindent the following lines.

MartinHjelmare · 2018-10-18T05:57:45Z

homeassistant/components/elkm1/services.yaml

@@ -0,0 +1,13 @@
+elkm1:
+  sensor_speak_word:


This is not the correct service name. Remove the sensor prefix.

gwww · 2018-10-19T12:16:16Z

Is this PR ready to merge? Thanks!

Initial sensor version.

3730e09

homeassistant added cla-signed integration: elkm1 new-platform platform: sensor.elkm1 labels Oct 11, 2018

ghost added the in progress label Oct 11, 2018

gwww mentioned this pull request Oct 11, 2018

Add Elk-M1 sensor platform documentation home-assistant/home-assistant.io#6705

Merged

2 tasks

Add Elk-M1 sensor services. Initial version of services.yaml.

de02584

bachya added the Hacktoberfest label Oct 12, 2018

Fix lint errors.

265c10e

MartinHjelmare reviewed Oct 16, 2018

View reviewed changes

Fix PR comments.

7b2a4ad

tjorim suggested changes Oct 17, 2018

View reviewed changes

fix PR comment

c4edb88

MartinHjelmare reviewed Oct 17, 2018

View reviewed changes

Fix PR comment

209da24

MartinHjelmare reviewed Oct 18, 2018

View reviewed changes

Fix PR comments

1083cfb

fix

76722e9

MartinHjelmare approved these changes Oct 19, 2018

View reviewed changes

MartinHjelmare merged commit 3655fef into home-assistant:dev Oct 19, 2018

ghost removed the in progress label Oct 19, 2018

balloob mentioned this pull request Oct 26, 2018

0.81 #17809

Merged

gwww deleted the elkm1-sensor branch November 12, 2018 15:15

ghost removed the platform: sensor.elkm1 label Mar 21, 2019

Uh oh!

Conversation

gwww commented Oct 11, 2018

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gwww commented Oct 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gwww commented Oct 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Example entry for `configuration.yaml` (if applicable):