Skip to content

add feature to connect through TCP to DSMR #7

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

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

Conversation

fboerman
Copy link

the underlying dsmr-parser package now also supports connecting through TCP socket. This PR enables the use of this in mqtt4dsmr. I also added python-dotenv for development purpose. I have it currently running at home for some days and works well.

I also cleaned up the requirements file and added some extra options such as disabling HA discovery when you don't use HA but still need MQTT like me.

Let me know what you think and if you want to change things!

enable disablement of HA discovery
upgrade requirements
make use of dotenv for development
@antonijn
Copy link
Owner

antonijn commented Apr 27, 2025

Thanks for your pull request! A TCP mode is definitely valuable and something we want to have. But I do see some issues with the PR.

  1. Consistent misspelling of DSMR as DMSR.
  2. Introduction of two new boolean options. Boolean options are to be kept to a minimum because a boolean is often the wrong data structure. Specifically, I would much prefer a DSMR_DEVICE_PROTOCOL which can be set to either serial (default) or tcp. Other options can be made required or optional based on this setting. This is also more future-proof. The option to disable HA can also be implemented by checking for an empty HA device ID, which I would prefer. This should also be a separate commit.
  3. Adding a dependency (dotenv) for development purposes only is not acceptable. If you want to keep this on your branch that's okay, but for development I would suggest a shell script to set the env vars.
  4. I must admit I do not know the best practices for requirements.txt, so I'm open to your input. The reason I have used pip freeze output is because of build reproducibility (similar to how a fixed python version is used in the dockerfile). Without another means of solving build reproducibility I cannot accept this requirements.txt upstream. If you wish to update dependencies, then run pip upgrade, test the application and pip freeze to requirements.txt on a separate "Add/update dependencies" commit.
  5. Why have you removed the fixed SERIAL_DEVICE setting from the dockerfile? This was obviously an intentional design decision on my side, so please clarify your reasoning here.

@fboerman
Copy link
Author

hi @antonijn ! Thanks for the quick review. some responses:

  1. oops, I am not very good in spelling, will fix that!
  2. personally I love to use booleans, but if you really object to this then the toggle as you meant is also possible. I can change that, it sounds nicer indeed
  3. The dockerfile you are using is quite bloated, I do not see why a new requirement would be such a big deal? dotenv works much much better then shellscripts in my opinion
  4. the use of pip freeze means there is all kinds of stuff in there that you dont need. Also you are using a quite "fat" dockerfile which is not pinned(python alpine is much better, but maybe that doesnt work with cryptography? did you test this perhaps already?) , so I would say reproducibility is not a thing already? I can try to make it optional in the code with some try except thats not really a big deal
  5. I think I did this because it interfered with my first tries on using TCP, but I will put that back.

@antonijn
Copy link
Owner

Booleans have their place of course, but not when they can be replaced with a "modal" approach. This simplifies the amount of undesired interactions you can have.

The objection to the additional dependency is not about bloat, but about keeping dependency management and complexity down to an absolute minimum. Even in javascript/python ecosystems I think people are starting to learn that additional dependencies for nice-to-haves has significant drawbacks. This project is to have three dependencies only, namely the required ones: Python, an MQTT library and a DSMR library.

I'm not sure if I follow what you say about pip freeze adding unneeded stuff. In the past I have always set up a clean venv, installed only paho-mqtt and dsmr-parser. Fixing their versions also fixes the versions of their dependencies, but surely this is not "unneeded stuff". You may be right about build reproducibility in the absolute sense in terms of the container image. Practically, however, compatibility of the base layer is guaranteed by fixing Python 3.12. The same is not true when removing the version tags from requirements.txt. I do not trust packages in the Python ecosystem to keep their APIs backwards-compatible.

I would be interested to hear back. Are you going to incorporate the changes yourself or should I do it?

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.

2 participants