ndokter / dsmr_parser

Library to parse Dutch Smart Meter Requirements (DSMR) telegrams.
MIT License
110 stars 64 forks source link

Support DSMR data read via RFXtrx with integrated P1 reader #97

Closed rhpijnacker closed 2 years ago

rhpijnacker commented 2 years ago

Recently I purchased a RFXtrxLAN with both a RFXtrx443XL and a DSMR P1 cable installed.

This device reads the telegram data from the P1 port and wraps it line-by-line into packets that fit the RF packet protocol that is normally pushed onto its USB or network connection. One such RF packet looks like this:

Over the last week(s) I've been trying to get this data into Home Assistant by feeding it through the RFXtrx integration. See e.g. https://github.com/Danielhiversen/pyRFXtrx/issues/125. However, the RFXtrx integration expects one RF packet to equal a complete sensor update, including identifier. Fitting in the case where multiple "RF" packets form a single telegram that includes the serial identifiers is turning out to get complicated.

I considered starting a new Home Assistant integration that handles this particular combination of RFXtrx and DSMR and started looking at the DSMR integration to see how things are done there. I realised that it would be trivial if we could simply add a variation where the RF packets are interpreted, skipping RF packets that aren't DMSR data and collecting the telegram data for ones that are.

If I would implement this variation, would this make any chance of getting accepted into this library?

rhpijnacker commented 2 years ago

Implemented a proof-of-concept, which works on my RFXtrx setup (with an additional single-line hack on the protocol level: changing DSMRProtocol into RFXtrxDSMRProtocol). I can add the DSMR integration in Home Assistant without any problems after loading this change in my Home Assistant test environment, and the measured values are getting updated.

To finish the PR we would need to decide on what the external API should be:

ndokter commented 2 years ago

Would it be possible to put the RFX specific part into a (new?) client module? It could inherit from the DSMRProtocol if needed i think. This way the RFX specific code is contained in it's own module and the default library is unaffected.

rhpijnacker commented 2 years ago

Yes, I think that could be possible. Do you have any suggestions for the file structure and API you'd like to see? E.g. something like rfxtrx_protocol.py containing create_rfxtrx_dsmr_reader, create_rfxtrx_tcp_dsmr_reader and create_rfxtrx_dsmr_protocol would make sense to me. This would probably give a small bit of code duplication that could be avoided by using an additional parameter. Maybe we could keep the external API the same and create some private functions with that extra parameter to keep the duplication minimal?

ndokter commented 2 years ago

Those names sound good! What part would need to be duplicated?

rhpijnacker commented 2 years ago

In the current implementation only line 48 needs to be changed from DSMRProtocol into RFXtrxDSMRProtocol to make it work. Creating a private create_dsmr_protocol passing this as parameter would avoid duplication there. The other two functions would need to call that with the appropriate parameter.

ndokter commented 2 years ago

Maybe you can add a 'protocol' parameter? def create_dsmr_protocol(dsmr_version, telegram_callback, loop=None, protocol=DSMRProtocol , **kwargs):

Which then is overrided only in the create_rfx.. function? Then the other methods won't work with the RFXtrxDSMRProtocol object which might be confusing?

rhpijnacker commented 2 years ago

I updated the (draft) Pull Request. Slightly differently than what you suggested. If you prefer, I can easily update it.

ndokter commented 2 years ago

Nice! Same line of thought :)

rhpijnacker commented 2 years ago

I tried following your suggestion, but somehow Python will not let me use protocol=DSMRProtocol in the function declaration. I do not know enough Python to understand why.

Is there anything I should change/add before you would accept the PR?

rhpijnacker commented 2 years ago

Great to see the Pull Request getting merged. Thanks! Do you plan to release this as a new version too?

ndokter commented 2 years ago

Hey, yes i have to manually make a new release. Will do it soon. Thanks for the contribution!