DSMR sensor #4309

aequitas · 2016-11-08T10:05:18Z

This is my initial version of an sensor implementation for the DSMR (Dutch smart meter protocol). In its current state it is blocking IO which I intent to fix first and it is dependent on contributions to dsmr_parser which need to be released upstream.

I would like to share this as a WIP and get feedback about if I am on track as this is the first component implementation I've written.

Example entry for configuration.yaml (if applicable):

sensor:
  - name: dsmr
    platform: dsmr
    device: /dev/ttyUSB1

Checklist:

Todos:

Add test
Non-blocking IO implemented
dsmr_parser contribution merged upstream (DSMR 2.2 ndokter/dsmr_parser#2)

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

Documentation added/updated in home-assistant.github.io

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

Local tests with tox run successfully.
New dependencies have been added to the REQUIREMENTS variable.
New dependencies are only imported inside functions that use them.
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
~~New files were added to .coveragerc.~~
Tests have been added to verify that the new code works.
Entity icons
Error handling and recovery
Wait for asyncio protocol to be merged upstream in dsmr_parser, update REQUIREMENTS

lwis · 2016-11-09T12:28:24Z

homeassistant/components/sensor/dsmr.py

+
+        _LOGGER.info('got new telegram')
+
+        yield from asyncio.sleep(10, loop=self.hass.loop)


What's this sleep for?

Should have documented this, but this is a temporary poor-mans semi-non-blocking implementation: the smartmeter sends 'telegrams' every 10 seconds. So the update only has to check every 10 seconds after the first telegram is received. This will be removed if non-blocking is implemented.

@aequitas you'll be better off using @Throttle in that case 👍

lwis · 2016-11-09T12:31:27Z

homeassistant/components/sensor/dsmr.py

+        self._telegram = {}
+
+    @asyncio.coroutine
+    def async_update(self):


As far as I can tell, this isn't making use of any asyncio functionality - this probably shouldn't be async as the update func should be run in an executor by the core.

I'm quiet new to the whole asyncio stuff in python3 but I thought this was the new way to go with components according to (https://home-assistant.io/developers/asyncio/) and since the current library (dsmr-parser) is implemented blocking I figured I might implement it non-blocking using asyncio (see todo).

@aequitas if you're planning on making dsmr-parser then your current implementation will be appropriate.

…rottle.

fabaff · 2016-11-10T22:55:55Z

homeassistant/components/sensor/dsmr.py

+REQUIREMENTS = ['dsmr-parser==0.2']
+
+CONF_DSMR_VERSION = 'dsmr_version'
+DEFAULT_DEVICE = '/dev/ttyUSB0'


I wouldn't recommend to do that because this will most likely not work on macOS and Windows.

For Mac it uses a tty device name with a random number I believe, will verify that, for windows it would default to COM0 I guess? So for Mac/Windows you would have to set the device but for Linux it would work without config if its the only device. There should be no harm in keeping a default in that case I think.

On this topic, I have been pondering about a auto discovery implementation for serial devices. It is out of scope for this PR but I think it could be doable. I want to implement it for my own setup as I have some other devices that get assigned a device name in random order at boot.

The issue with that default is that it would be hard to debug for a no-Linux user.

fabaff · 2016-11-10T22:56:36Z

homeassistant/components/sensor/dsmr.py

+DEFAULT_DSMR_VERSION = '4'
+
+PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
+    vol.Optional(CONF_DEVICE, default=DEFAULT_DEVICE): cv.string,


For serial ports we use port: for the configuration.

Is this kind of stuff documented somewhere? Because I took the zigbee module as an example which uses 'device' so my guess was that was the convention https://github.com/home-assistant/home-assistant/blob/ece58ce78fe44b521122e3f41b79e670010bc905/homeassistant/components/zigbee.py#L30

and port seems to be generally used only in tcp/udp context.

No, it's not documented. I'm trying to establish a common base for the configuration and not use usb_path, filename, and others. It's a connection over a serial port. Even better than port would serial_port be, distinguish between the network port and the physical port.

Is there a reason it is not outlined in a document yet?

In my opinion if you want to work toward a convention for (for example) naming you outline it first in a document, this should make it easy for (new) contributors to learn about what the conventions are. Now I had to learn this convention through a pullrequest review, as the only other information I could go on is other components.

I rename the config to port, let me know if you want me to take initiative on documenting the conventions.

aequitas · 2016-11-12T00:44:41Z

I added the implementation for a non-blocking reader, however it needs upstream contribution (ndokter/dsmr_parser#3) before it can pass the tests. Feel free to comment on the new code.

Tests still need to be refactored.

lwis · 2016-11-12T10:31:32Z

.envrc

@@ -0,0 +1 @@
+layout python3


Should we keep this? Add it to .gitignore if needed.

Sorry, this got included by accident.

lwis · 2016-11-12T10:33:04Z

homeassistant/components/sensor/dsmr.py

+    hass.loop.create_task(dsmr.read_telegrams(queue))
+
+
+class DSMR:


Should this be an interface?

Can you explain the question a little further?

lwis · 2016-11-12T10:35:42Z

homeassistant/components/sensor/dsmr.py

+    @property
+    def state(self):
+        """Return the state of the sensor, if available."""
+        return getattr(self._interface.telegram.get(self._obis, {}),


This should be cached in an update method.

The call itself only reads the value of telegram and does not perform any IO or update sequence. What would be the reason to cache the value in this case?

lwis · 2016-11-12T10:36:06Z

tests/components/sensor/test_dsmr.py

+        ]),
+    }
+
+    # with patch('homeassistant.components.sensor.dsmr.DSMR.read_telegram',


Why is this commented?

I still need to refactor the test suite after de code has been refactored to non-blocking. This will be addresses (see todo).

aequitas · 2016-11-12T23:21:19Z

Refactored the tests for asyncio and added error handling. Also simplified/refactored some other parts of the code. I have all todos I intended for this feature completed. Could you please review again and let me know what needs to be changed before it can be accepted.

balloob · 2016-11-13T02:16:06Z

homeassistant/components/sensor/dsmr.py

+    devices += [DSMREntity(name, obis, dsmr) for name, obis in obis_mapping]
+
+    # setup devices
+    yield from hass.loop.create_task(async_add_devices(devices))


async_add_devices is a coroutine function, no need to wrap it in create_task

ofc, I still need to get a little accustomed to the framework and asyncio.

balloob · 2016-11-13T02:17:06Z

homeassistant/components/sensor/dsmr.py

+    reader.add_done_callback(handle_error)
+
+    # add task to receive telegrams and update entities
+    hass.loop.create_task(dsmr.read_telegrams(queue))


please use hass.async_add_job so that we can keep track of it (and be able to use hass.block_till_done())

Also, please cancel this coroutine when HOMEASSISTANT_STOP event is fired.

I will try to make that work.

Could you tell me if I am using the right pattern here? Hardware wise the Smartmeter is connection via a usb serial device, and the smartmeter sends a update every 10 seconds. I wanted to get this to work with some kind of async generator which would yield on waiting for new serial input. However my short research into asyncio indicated async generators are not supported until python 3.6. I was unable to find a different way to create something like a async generator for python3.4. So I resorted to a task with a output queue. But maybe there is a better (simpler) way to implement this which does away with the queue and the need to listen for HOMEASSISTANT_STOP.

balloob · 2016-11-13T02:19:17Z

homeassistant/components/sensor/dsmr.py

+        """Return the state of sensor, if available, translate if needed."""
+        from dsmr_parser import obis_references as obis
+
+        value = getattr(self._interface.telegram.get(self._obis, {}),


It's unclear to me if these calls are doing I/O. They shouldn't (as we're inside async land)

Someone else also commented on this. I will refactor/document this piece a bit to make it more clear it is not doing I/O.

balloob · 2016-11-13T02:21:00Z

tests/components/sensor/test_dsmr.py

+
+    # mock queue for injecting DSMR telegram
+    queue = asyncio.Queue(loop=hass.loop)
+    monkeypatch.setattr('asyncio.Queue', lambda: queue)


Nice. Didn't know about monkeypatch.

There is a lot of cool stuff in pytest. Thanks for going with that framework (and Tox). In my opinion it is the best one out there atm, but it doesn't force you into one style of writing tests as it also supports unittest.

btw, have you considered using https://github.com/klen/pylama for the project? It combines a lot of the linting tools used in this project into a single consistent interface.

- refactor asyncio reader task to make sure it stops with HA - document general principle of this component - refactor entity reading to be more clear - remove cruft from split entity implementation

aequitas · 2016-11-13T13:55:56Z

@balloob I refactored to address the issues you mentioned. Thanks for pointing out the required structures in the framework. I did not find good examples myself in the other components because there was just to much :). My assumption was hass/asyncio would take care of the stopping of the tasks automatically..

I added some additional overall documentation at the top of the module.

Let me know if there is anything else that needs to be done.

aequitas · 2016-11-13T21:07:08Z

Strange, this build failed for py34 but another travis build on the same commit succeeded (https://travis-ci.org/aequitas/home-assistant/builds/175541781). This one might succeed if retried again. Error/failure is not on my testsuite btw.

Is it common for these kind of test instabilities to pop up in this framework? Or could it be a bug/race-condition introduced by my code?

http://www.netbeheernederland.nl/themas/hotspot/hotspot-documenten/?dossierid=11010056&title=Slimme%20meter&onderdeel=Documenten > DSMR v2.2 Final P1 >> 6.1: table vs table note Meter Reading electricity delivered to client normal tariff) in 0,01 kWh - 1-0:1.8.1.255 Meter Reading electricity delivered to client (low tariff) in 0,01 kWh - 1-0:1.8.2.255 Note: Tariff code 1 is used for low tariff and tariff code 2 is used for normal tariff.

aequitas · 2016-11-16T08:26:25Z

Currently this components is being tested by a few persons in this thread: https://community.home-assistant.io/t/support-for-reading-dutch-smart-meter-electricity-gas-p1-port/1676

Some people have issues regarding serial port settings which can differ depending on the meter type and usb hardware used. I am working on finding a solution for this.

henkvdt · 2016-11-17T14:40:34Z

I might be wrong, but I think the kWh and kW units of measurement are switched: The current usage shows kW, but this should be kWh (usage per hour). The meter reading shows kWh, but this should be kW (absolute number)

aequitas · 2016-11-17T15:54:50Z

@henkvdt the kW and kWh units are taken directly from the DSMR telegram. So the telegram would be wrong in that case.

The current usage shows how much power is being used at this moment in kilowats, the meter reading is how much energy has been consumed.
http://www.energylens.com/articles/kw-and-kwh

aequitas · 2016-11-20T16:30:41Z

Users are reporting metrics stalling when a telegram failed to be parsed. Will be looking into this.

aequitas · 2016-11-21T15:44:27Z

I refactored the component a little bit to remove some complexity. I learned Protocols is the way to go in asyncio, so that is the implementation I went for this time.

lwis · 2016-11-21T15:46:39Z

homeassistant/components/sensor/dsmr.py

 from homeassistant.helpers.entity import Entity

 DOMAIN = 'dsmr'

-REQUIREMENTS = ['dsmr-parser==0.3']
+REQUIREMENTS = [
+    'https://github.com/aequitas/dsmr_parser/archive/async_protocol.zip'


Do you know if this will be merged upstream? Or are you planning on maintaining an async fork?

This also needs a version at the end ...zip#dsmr-parser==0.4 for example.

Don't know for sure yet, but the maintainer has been very keen to merge my changes so far so I don't foresee issue there. If not I will create my fork. I added a todo to this PR to ensure this gets taken care of.

Merged upstream, just waiting for new version to be deployed: ndokter/dsmr_parser#7

lwis · 2016-11-21T15:47:15Z

homeassistant/components/sensor/dsmr.py

+        for device in devices:
+            device.telegram = telegram
+            hass.async_add_job(device.async_update_ha_state)
+            # hass.loop.create_task(device.async_update_ha_state())


What's this for?

Nicely spot, left that in there by accident.

May I suggest adding: https://github.com/aequitas/pytest-eradicate to the project?

balloob · 2016-11-23T07:03:37Z

Great 🐬

aequitas · 2016-11-23T10:25:23Z

@balloob thanks.

I had some todo's still unchecked in the PR description. What is the recommended way to indicate a PR should not yet be merged? I though leaving todos unchecked would be a good indicator for that.

Merge should not have to be reverted I am pushing the update in a minute and the error handling seems to only affect one of the test users atm with which I am in debug session on the community forum.

lwis · 2016-11-23T10:29:55Z

@aequitas if you create a new PR and tag me I'll review and merge if it all looks good.

If you don't want something to be merged and you're just looking for feedback, it's best to put "WIP" in the title.

aequitas · 2016-11-23T10:35:37Z

@lwis ok, this one was a little in between because at first I considered it ready for merge but then some users had feedback already. I will keep with the WIP convention next time.

balloob · 2016-11-24T18:15:05Z

Sorry, missed the to do checkbox. WIP is the best indicator.

Johan Bloemberg added 7 commits November 8, 2016 10:38

Initial implemenation of DSMR component.

3c1fd15

Fix linting

5b3f0ba

Remove protocol V2.2 support until merged upstream.

aed7627

Generate requirements using script.

4544685

Use updated dsmr-parser with protocol 2.2 support.

ce70497

Add tests.

fc6e732

Isort and input validation.

9c86d0a

lwis reviewed Nov 9, 2016

View reviewed changes

Add entities for gas and actual meter reading. Error handling. Use Th…

e4594f1

…rottle.

fabaff changed the title ~~Dsmr~~ DSMR sensor Nov 10, 2016

fabaff reviewed Nov 10, 2016

View reviewed changes

Johan Bloemberg added 2 commits November 12, 2016 01:42

Implement non-blocking serial reader.

27a4e01

Improve logging.

1cf1341

lwis reviewed Nov 12, 2016

View reviewed changes

aequitas mentioned this pull request Nov 12, 2016

Dmsr home-assistant/home-assistant.io#1432

Merged

Merge entities into one, add icons, fix tests for asyncio.

aabbe6e

aequitas force-pushed the dsmr branch from 1a01617 to aabbe6e Compare November 12, 2016 23:02

Add error logging for serial reader.

c5de690

balloob reviewed Nov 13, 2016

View reviewed changes

balloob self-assigned this Nov 13, 2016

Refactoring and documentation.

8f30b51

- refactor asyncio reader task to make sure it stops with HA - document general principle of this component - refactor entity reading to be more clear - remove cruft from split entity implementation

Use port configuration key.

42d7e23

Refactor to use asyncio.Protocol instead of loop+queue.

873cbd4

lwis reviewed Nov 21, 2016

View reviewed changes

Johan Bloemberg and others added 6 commits November 21, 2016 16:50

Fix requirements

749de01

Close transport when HA stops.

30137d4

Cleanup.

95e5a83

Include as dependency for testing (until merged upstream.)

eb6c853

Fix style.

f5e9ccc

Update setup.cfg

0762fca

balloob merged commit 64cfc4f into home-assistant:dev Nov 23, 2016

home-assistant locked and limited conversation to collaborators Mar 17, 2017


		_LOGGER.info('got new telegram')

		yield from asyncio.sleep(10, loop=self.hass.loop)

		hass.loop.create_task(dsmr.read_telegrams(queue))


		class DSMR:

DSMR sensor #4309

DSMR sensor #4309

Conversation

aequitas commented Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aequitas commented Nov 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aequitas commented Nov 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aequitas commented Nov 13, 2016

aequitas commented Nov 13, 2016

aequitas commented Nov 16, 2016

henkvdt commented Nov 17, 2016

aequitas commented Nov 17, 2016

aequitas commented Nov 20, 2016

aequitas commented Nov 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balloob commented Nov 23, 2016

aequitas commented Nov 23, 2016

lwis commented Nov 23, 2016

aequitas commented Nov 23, 2016

balloob commented Nov 24, 2016

aequitas commented Nov 8, 2016 •

edited

Loading