Support DSMR data read via RFXtrx with integrated P1 reader

[rhpijnacker]

Proposed change

ndokter/dsmr_parser#98 adds a new set of API functions to dsmr_parser that allows reading DSMR data from a RFXtrx device with an integrated P1 reader.

The new API functions expect the DSMR telegram to be wrapped up as RF packages and strip away the RF packet headers to get to the actual telegram data. They also discard any non telegram related packets.

This PR makes these new functions available to the DSMR integration by auto-detecting whether using a RFXtrxDSMRProtocol is needed.
It introduces an additional boolean config property, that can be enabled in the config flow, when a RFXtrx with P1 device is used. In this case the code will use the rfxtrx variation of the functions to read the telegram from the DSMR device.

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

The dependency update to version 0.32 of dsmr_parser that holds the new API functions is already handled in a previous PR.

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. See https://github.com/home-assistant/home-assistant.io/pull/21060/files.

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]

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]

I would also suggest to make a documentation PR and link it here. It could use a few words in the documentation.

[RobBie1221]

Some suggestions from my side.

Also, it would be good to add a smoke test to test_sensor.py, that would also fix failing CodeCov.

[rhpijnacker]

I would also suggest to make a documentation PR and link it here. It could use a few words in the documentation.

I can't think of much more than just mentioning that the combination of RFXtrx and P1 cable is also supported.
I guess there is little point is explaining that they can use a checkbox to select that, is there?

[RobBie1221]

Under chapter "CONNECTING TO THE METER" there are options to make connections. I'd add here that the RFXtrx device is supported. It seems to me useful for users that want to know if the integration supports the device.

I think the checkbox is quite self explanatory with above note.

[frenck]

First question: We have an RFXtrx integration... That seems a better fix than putting rfxtrx stuff into the DSMR integration?

[rhpijnacker]

First question: We have an RFXtrx integration... That seems a better fix than putting rfxtrx stuff into the DSMR integration?

That was exactly my first thought, which is why I started working on Danielhiversen/pyRFXtrx#125.

The RFXtrx integration is based on the assumption that one RF packet equals one or more sensor updates in HA.
However, when the RFXtrx sends DSMR telegram data to the receiver, the telegram is split up over multiple RF packets. This is something that does not fit very well with the pyRFXtrx design.

A second problem is that we need to "put DSMR stuff into the rfxtrx integration".
Initially, I tried to keep the pyRFXtrx library free of DSMR knowledge, passing the uninterpreted telegram lines to the rfxtrx component. This fits even less with the design assumptions of rfxtrx, because the rfxtrx integration expects the packets to contain interpreted measurement data, every time an event comes in.
Then I tried doing the DSMR collection and parsing on the level of pyRFXtrx, which means that some sensor events will not contain any values if the telegram is not yet complete. Because of this I got all kind of "not available" sensors in HA.

After trying for days to "put DSMR stuf into RFXtrx", I started looking at the DSMR integration. I found out that it was trivial to filter and skip the RF related data on the transport level. So "putting RFXtrx into DSMR" turned out to be a much better fit. Extending the API of the dsmr_parser.clients with a rfxtrx_protocol that closely resembles the existing protocol was a matter of a few hours work and very little code changes.
With a one-line hack everything worked in Home Assistant flawlessly too (replacing the call to create_tcp_dsmr_reader with create_rfxtrx_tcp_dsmr_reader was sufficient to make it work).

In the DSMR integration all it needs to know is whether it reads directly from a P1 port, or via the RFXtrx.
Everything else is identical.

[RobBie1221]

Would it work when using rfxtrx and dsmr integration in parallel on the same serial port?

[frenck]

Would it work when using rfxtrx and dsmr integration in parallel on the same serial port?

No, serial ports can't be shared. Which is the main reason for my comment.

[MartinHjelmare]

Please test a create entry result for the new protocol in the config flow.

[rhpijnacker]

Please test a create entry result for the new protocol in the config flow.

Added for both serial and network.
Not sure how I can cover the lines currently not covered, though... (one of the remaining warnings is about a function signature ?!?)

[rhpijnacker]

Is there anything more I need to do to finish this PR?
I see that "some checks were not successful", however running the tests yields coverage:

homeassistant/components/dsmr/config_flow.py     168      5    97%   87-88, 126-129

which does not match the codecov/patch results. I did not touch the not covered lines at all.

I do not really understand the lines reported by codecov/patch. It looks to me like the tool is confused.

[RobBie1221]

See #63669 (comment)

Config flow requires 100% coverage but since some protocols were tested with import and import code has been completely removed, coverage in the repo is below 100%.
You could simply fix it by copying a config test for protocol 5L and change it to 5S to also test Swedish protocol.

[rhpijnacker]

See #63669 (comment)

Config flow requires 100% coverage but since some protocols were tested with import and import code has been completely removed, coverage in the repo is below 100%. You could simply fix it by copying a config test for protocol 5L and change it to 5S to also test Swedish protocol.

So, I'm just the unlucky guy that changed something right after this happened?
So far, I'm not enjoying my first-time-committer experience as a Home Assistant developer very much :( . This certainly does not make it any better.

See #65238.

[frenck]

This looks good to me 👍