Add DSMR config options for EasyMeter/Q3D by Aeroid · Pull Request #63669 · home-assistant/core

Aeroid · 2022-01-08T11:46:13Z

Proposed change

Adding support dsmr EasyMeter Q3D
(re-submitting expired PR #60088, apologies)

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request: add documentation note for Q3D support in dsmr nxt home-assistant.io#21113

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

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

Documentation added/updated for www.home-assistant.io

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

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

probot-home-assistant · 2022-01-08T11:46:19Z

Hey there @RobBie1221, @frenck, mind taking a look at this pull request as it has been labeled with an integration (dsmr) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

RobBie1221 · 2022-01-08T11:50:55Z

As mentioned in the comment of #60088 you need to add tests for the config flow because it requires 100% coverage. It would also be good to add a smoke test to test_sensor.py to test setup with the new option.

frenck

As per comment above, please add tests 👍

RobBie1221 · 2022-01-08T11:52:29Z

#63529 contains a good example of how to extend tests if you need some guidance.

Aeroid · 2022-01-08T15:59:42Z

Added a test for a Q3D telegram. Given the very minimal changes expanding the protocol version just by one entry, the test is not really improving coverage. But this was my first test ever written. So I hope it's sufficient. Otherwise I would appreciate details of how I may improve the PR to an acceptable level.

RobBie1221 · 2022-01-08T17:01:25Z

The test you added is fine for the smoke test. It doesn't improve coverage that much, but it ensures setting up the integration with the new option will work (and will keep working).

The config flow actually needs 100% coverage, lines 49 and 50 you added in config_flow.py are untested.

To test these, you can reuse a lot. So add in below file/function a statement to add for Q3D the equipment identifier :

core/tests/components/dsmr/conftest.py

Lines 51 to 67 in 8860549

    
           async def connection_factory(*args, **kwargs): 
        
               """Return mocked out Asyncio classes.""" 
        
               if args[1] == "5L": 
        
                   protocol.telegram = { 
        
                       LUXEMBOURG_EQUIPMENT_IDENTIFIER: CosemObject( 
        
                           [{"value": "12345678", "unit": ""}] 
        
                       ), 
        
                       EQUIPMENT_IDENTIFIER_GAS: CosemObject( 
        
                           [{"value": "123456789", "unit": ""}] 
        
                       ), 
        
                   } 
        
               if args[1] == "5S": 
        
                   protocol.telegram = { 
        
                       P1_MESSAGE_TIMESTAMP: CosemObject([{"value": "12345678", "unit": ""}]), 
        
                   } 
        
               return (transport, protocol)

Then copy below function, name it something like test_setup_q3d and change dsmr_version to Q3D, that will test the the missing lines above.

core/tests/components/dsmr/test_config_flow.py

Line 66 in 8860549

    
           async def test_setup_serial(com_mock, hass, dsmr_connection_send_validate_fixture):

RobBie1221 · 2022-01-08T17:23:19Z

@frenck I noticed coverage of config_flow.py dropped below 100% here: #61008. The import test was actually testing also connection validation for Luxembourgh and Swedish meters. We probably need to fix that in another pull?
Anyway, just with adding tests for the added code here, we won't reach 100% coverage.

Aeroid · 2022-01-09T09:32:38Z

The test you added is fine for the smoke test. It doesn't improve coverage that much, but it ensures setting up the integration with the new option will work (and will keep working).

The config flow actually needs 100% coverage, lines 49 and 50 you added in config_flow.py are untested.

To test these, you can reuse a lot. So add in below file/function a statement to add for Q3D the equipment identifier :

core/tests/components/dsmr/conftest.py

Lines 51 to 67 in 8860549

async def connection_factory(*args, **kwargs):

"""Return mocked out Asyncio classes."""

if args[1] == "5L":

protocol.telegram = {

LUXEMBOURG_EQUIPMENT_IDENTIFIER: CosemObject(

[{"value": "12345678", "unit": ""}]

),

EQUIPMENT_IDENTIFIER_GAS: CosemObject(

[{"value": "123456789", "unit": ""}]

),

}

if args[1] == "5S":

protocol.telegram = {

P1_MESSAGE_TIMESTAMP: CosemObject([{"value": "12345678", "unit": ""}]),

}

return (transport, protocol)

Then copy below function, name it something like test_setup_q3d and change dsmr_version to Q3D, that will test the the missing lines above.

core/tests/components/dsmr/test_config_flow.py

Line 66 in 8860549

async def test_setup_serial(com_mock, hass, dsmr_connection_send_validate_fixture):

I actually tried something like that yesterday, playing around with the version in the existing test_setup functions. Following your tipps I'm running into the same problem where I gave up yesterday: As soon as config_flow lines 48 (for 5L) or 50 (for Q3D) get coverage I get AssertionErrors: assert 'form' == 'create_entry' in the final assertation. Gone when I change back to "5", "5S", or "2.2".

Couldn't get to the bottom of it. Should I commit the half baked code for visibility?

RobBie1221 · 2022-01-09T09:42:49Z

Couldn't get to the bottom of it. Should I commit the half baked code for visibility?

Yes, I'll check what's wrong.

RobBie1221

This should fix it. Don't forget to add import in conftest.py.

Just a general question looking at the obis references, what is the difference between Q3_EQUIPMENT_IDENTIFIER and Q3_EQUIPMENT_SERIALNUMBER? Is the equipment identifier unique for your device? Or should we actually use here serial number?

Furthermore, you need to add a documentation pull request. I saw you had it in your initial PR. You can reopen that one or make a new one (maybe easier because the original needs to be rebased on next).

RobBie1221 · 2022-01-09T11:38:59Z

+    entry_data = {
+        "port": port.device,
+        "dsmr_version": "Q3D",
+    }


Suggested change

entry_data = {

"port": port.device,

"dsmr_version": "Q3D",

}

entry_data = {

"port": port.device,

"dsmr_version": "Q3D",

"serial_id": "12345678",

"serial_id_gas": None,

}

Aeroid · 2022-01-09T12:08:41Z

They are more or less the same. S/N has a prefix.

Q3D_EQUIPMENT_IDENTIFIER = r'\d-\d:0.0.0.+?\r\n' # Logical device name
Q3D_EQUIPMENT_SERIALNUMBER = r'\d-\d:96.1.255.+?\r\n' # Device Serialnumber

telegram: /ESY5Q3DA1004 V3.04

1-0:0.0.0255(1336001560)
1-0:1.8.0255(00032653.9579389kWh)
1-0:21.7.0255(000050.56W)
1-0:41.7.0255(000000.00W)
1-0:61.7.0255(000000.00W)
1-0:1.7.0255(000050.56W)
1-0:96.5.5255(80)
0-0:96.1.255*255(1ESY1336001560)
!

Co-authored-by: Rob Bierbooms <mail@robbierbooms.nl>

Aeroid · 2022-01-09T12:34:31Z

Happy to add doc.

The test should be working now and also included a 5L test to bump up converage by another %.

Aeroid · 2022-01-09T12:57:38Z

Doc PR is home-assistant/home-assistant.io#21113

Please let me know if I can do anything else.

RobBie1221 · 2022-01-09T13:08:48Z

The link points to wrong repo, you should copy the link from home-assistant.io github repo. Also, add it to the PR description in the template.

frenck

Nice! Thanks, @Aeroid 👍

Adding DSMR config options for EasyMeter/Q3D

27bddda

Aeroid requested a review from frenck as a code owner January 8, 2022 11:46

homeassistant added the cla-signed label Jan 8, 2022

probot-home-assistant Bot added integration: dsmr new-feature small-pr PRs with less than 30 lines. labels Jan 8, 2022

probot-home-assistant Bot assigned frenck Jan 8, 2022

frenck requested changes Jan 8, 2022

View reviewed changes

MartinHjelmare changed the title ~~Adding DSMR config options for EasyMeter/Q3D~~ Add DSMR config options for EasyMeter/Q3D Jan 8, 2022

Testing dsmr Q3D telegram

332cfbc

Failing tests for Q3D code coverage

42de101

RobBie1221 suggested changes Jan 9, 2022

View reviewed changes

Aeroid and others added 3 commits January 9, 2022 13:11

Update tests/components/dsmr/conftest.py

8be1f1b

Co-authored-by: Rob Bierbooms <mail@robbierbooms.nl>

Update tests/components/dsmr/test_config_flow.py

7f017f5

Co-authored-by: Rob Bierbooms <mail@robbierbooms.nl>

working tests for Q3D and 5L

f749b92

This was referenced Jan 9, 2022

add documentation note for Q3D support in dsmr home-assistant/home-assistant.io#21111

Closed

add documentation note for Q3D support in dsmr nxt home-assistant/home-assistant.io#21113

Merged

frenck approved these changes Jan 10, 2022

View reviewed changes

frenck merged commit 0f2e2ae into home-assistant:dev Jan 10, 2022

github-actions Bot locked and limited conversation to collaborators Jan 11, 2022

Uh oh!

Conversation

Aeroid commented Jan 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed change

Type of change

Additional information

Checklist

Uh oh!

probot-home-assistant Bot commented Jan 8, 2022

Uh oh!

RobBie1221 commented Jan 8, 2022

Uh oh!

frenck left a comment

Choose a reason for hiding this comment

Uh oh!

RobBie1221 commented Jan 8, 2022

Uh oh!

Aeroid commented Jan 8, 2022

Uh oh!

RobBie1221 commented Jan 8, 2022

Uh oh!

RobBie1221 commented Jan 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Aeroid commented Jan 9, 2022

Uh oh!

RobBie1221 commented Jan 9, 2022

Uh oh!

RobBie1221 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RobBie1221 Jan 9, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Aeroid commented Jan 9, 2022

Uh oh!

Aeroid commented Jan 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Aeroid commented Jan 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RobBie1221 commented Jan 9, 2022

Uh oh!

frenck left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Aeroid commented Jan 8, 2022 •

edited

Loading

RobBie1221 commented Jan 8, 2022 •

edited

Loading

Aeroid commented Jan 9, 2022 •

edited

Loading

Aeroid commented Jan 9, 2022 •

edited

Loading