Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Iskra IE.5 (Belgium) #153

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ljehu
Copy link

@ljehu ljehu commented Apr 11, 2024

Modified regex in obis_references.py to recognize multiple belgian devices.

@ndokter
Copy link
Owner

ndokter commented Apr 23, 2024

Thanks for the PR! There are some tests which need to be fixed first

@anthares
Copy link

anthares commented May 2, 2024

Hi @ljehu,

I just got the same smart meter installed but from another grid provider in Belgium (AIEG).
I'd like to see this merged, but I don't know how I could help on this MR.

Feel free to let me know :)

Kind regards,

Nicolas

@anthares
Copy link

anthares commented May 23, 2024

Hi @ndokter,

I made some test of this change locally to try to understand why tests are failing.
I'm not a seasoned python dev (coming from Java world), but here is what I found.

Current change in version information regex makes parser fail to retrieve the right line in the telegram data.
Instead of finding a single string, it finds a tuple of other strings, due to the addition on regex groups in the regex.

My feeling is, after some debug, is that just changing the regex won't work with the way the parser works right now. Making the parser compatible with those regex groups is way above my current skills.
So I think the right way to go, and the safest, is to introduce another value for dsmr_version in protocol.py with corresponding specs in telegram_specifications.py and proper testing.

So, sorry to bother you on this, but I want your advice before going further.
I'm ready to create another PR for that, I just want to make sure this is how it should be done :)
I'm also open to suggestion for the dsmr_version value to introduce.

Best regards,

Nicolas

@ndokter
Copy link
Owner

ndokter commented Jun 7, 2024

Hi @anthares, sorry for the late response. I will try to get back to you

@ndokter
Copy link
Owner

ndokter commented Jun 16, 2024

@anthares you are right that the main problem is the tuple. Would something like this work? It fixes the tuple issue, but i'm not sure about the resulting values. The tests break in a different way:
BELGIUM_VERSION_INFORMATION = r'^\d-\d:96\.(?:14\.0|1\.4).+?\r\n'

@rmelotte
Copy link

rmelotte commented Nov 8, 2024

@anthares Would you be able to share a full Telegram for your meter so that we can compare it to the one we already have (from REW) in #152 (comment) ?

I'd like to understand if we need to support two new version information (and equipment identifiers?), or if the ones from AIEG and REW are the same (but different from Fluvius apparently).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants