openhab-addons
openhab-addons copied to clipboard
openhab
[smaenergymeter] Fix handling of broadcast frames
SMA Energy Meter improvements
There are two outstanding issues which are closely related to how UDP traffic is handled by SMA energy meter binding. This PR addresses both by separating several concerns into their own places, separating network communication from data parsing and port management. Starting point for fix were odd values reported by meter binding in #11497. Additionally it introduces support for handling of multiple meters requested in #3429 which is impossible prior provided fixes.
This work wouldn't be possible without amazing analysis made by Thomas: https://community.openhab.org/t/sma-energy-meter-binding-yields-unplausible-values/128180/4.
This pull request has been mentioned on openHAB Community. There might be relevant details there:
https://community.openhab.org/t/sma-energy-meter-binding-yields-unplausible-values/128180/6
I've updated PR as there are some troubles with amount of changes I introduced. We're clearing issues out in forum topic mentioned earlier.
@splatch : do you plan to finish that PR or should we close it ? In case you expect a review, please fix the conflicts and make the PR valid (it includes unexpected changes in many bindings).
@lolodomo I've updated branch, however not tested it yet as I have no 3.4.0 deployment with sma available. Lets see if others can confirm it working.
What about the flag "work in progress" ? I understand you did not test your PR, are you enough confident with your changes to remove the "WIP" flag ? @monnimeter: could you please have a look and confirm that the current PR code is still OK for you ?
@lolodomo I've fixed troubles you pointed out, other issues you reported I leave for further discussion.
Note - this PR is a breaking change as it introduces a required parameter to thing (serialNumber).
There were some notes on it in related forum thread. Beyond these code worked fine (when I had access to SMA equipment) and filtered out wrong packet kinds.
@splatch this PR has both awaiting feedback and WIP label, are they still valid or should a maintainer look at it?
This pull request has been mentioned on openHAB Community. There might be relevant details there:
https://community.openhab.org/t/sma-energy-meter-binding-yields-unplausible-values/128180/87
I've updated PR to latest main, please re-review it as it solves two major troubles for users and one which affects quality of data delivered by energy meter. This PR been open for almost two years during which I lost access to installation where I could test binding.
FYI #13437 from @lrep proposed some good improvements for protocol handling which would clarify a bit binding codebase.
Can you also run the i18n plugin?
If i understand this PR, the breaking change is only the mandatory configuration (no channels, or functional changes) so i think a update notice would be enough. Please create a PR here: https://github.com/openhab/openhab-distro/pulls
@splatch this PR has both awaiting feedback and WIP label, are they still valid or should a maintainer look at it?
I haven't added any of these labels, thus I am not a valid person to answer this question.
@splatch as you didn't add the WIP label and stated before this is ready for review, i removed the WIP label. But before a merge can be made, there are 3-4 comments from lolodomo that are not yet marked as resolved. Could you look at them (they are deeply hidden behind all the load more buttons)
Could you also add a breaking change notice here: https://github.com/openhab/openhab-distro/pulls
This pull request has been mentioned on openHAB Community. There might be relevant details there:
https://community.openhab.org/t/sma-energy-meter-binding-yields-unplausible-values/128180/116
@lsiepel I've updated copyright headers, i18n definitions and removed part of discovery service which was in question in unresolved comments from 2022. It should be good to go once CI finishes its job.
@kaikreuzer Sorry for distrubing you, but what is stopping the community from releasing the updated binding in 4.2.0 (snapshot)? I have a installation with 3 SMA energy meters, and the current Openhab binding is useless for more than 2 years, due the broadcast frames, and single meter restriction. Your help will be very much appreciated.
This pull request has been mentioned on openHAB Community. There might be relevant details there:
https://community.openhab.org/t/upgrade-or-move-to-home-assistant/152216/84
@cambronf Not sure why you are pinging me here. As far as I can read from above, the last updates were done due to @lolodomo's review comments, so he should check if they are addressed as wished. Then it is for any @openhab/add-ons-maintainers to merge the PR.
This pull request has been mentioned on openHAB Community. There might be relevant details there:
https://community.openhab.org/t/sma-energy-meter-binding-yields-unplausible-values/128180/130
I am asking myself why there is the tag "potentially not backward compatible" ? My feeling is that your code is backward compatible. Of course, after the change, all existing things will be OFFLINE until the serial number is filled.
This pull request has been mentioned on openHAB Community. There might be relevant details there:
https://community.openhab.org/t/sma-energy-meter-binding-yields-unplausible-values/128180/164
@lsiepel I've reflected my view on this here: https://community.openhab.org/t/sma-energy-meter-binding-yields-unplausible-values/128180/165
To phrase it again, I'm not going to waste more time on nit picks. I can fix the socket leak, but nothing else.
not going to waste more
Ah, got a bit late to the party. Sorry to hear this somewhat negative pov. I would not call them nit picks, but i can understand that it sometimes feels like it. A project like this has some guidelines and rules not to irritate you, but to assure quality, maintainability and so on. I also understand that a long standing PR like this is another factor in the equasion. Anyway, i would really appreciate it if you take the effort to fix the review comments. If not i can try to commit to your repository to get this accross the finish line. Please let me know.
@lsiepel I am not irritated, because I do not use this binding, hence I do not suffer from missed functionality. The most recent review contributed nothing (or at least very little) to quality of this addon, these are rather personal opinions of reviewer. More over these contradict purpose of Java GC and openHAB framework itself. If user controllable log output in one place or setting null values after dispose, empty method body, or way in which default values are set, are not nit picks, then what you consider a such?
Code I've submitted for review passed quality checks enforced by SAT. Neither of matters from recent review represent issue for SAT, if it should, then please update SAT to save not only reviewer but also other contributors time. Its not the first nor the latest PR, stuck by mysterious quality criteria combined with I do not like this construction™ review.
There is issue with socket leak reported for ipv6 already addressed in forums, which is hardly related to recent comments.
de I've submitted for review passed quality checks enforced by SAT. Neither of matters from recent review represent issue for SAT, if it should, then please update SAT to save not only reviewer but also other contributors time. Its not the first nor the latest PR, stuck by mysterious quality criteria combined with I do not like this construction™ review.
There is issue with socket leak reported for ipv6 already addressed in forums, which is hardly related to recent comments.
Unfortunately, SAT cannot enforce every case. For those cases we work with several resources to keep consistency. For example there is logging documentation explaining when to use each level. There is also a public available review checklist and i see some checks from that list in this PR review like "Use primitive over complex types (e.g. int vs. Integer)"
It is not to prove you are wrong, because i think you are right! There will always be different maintainers having different preferences, and as long as this is an opensource project full of volunteers, i guess this will not dramatically change.
That being said, my offer to help get this PR across the finish line still stands, let me know if i can be of any help.
@lsiepel None of review checklist mention empty method bodies, sets over lists. Use of primitives over complex types is subjective matter related to nullnes, which in above case when elements are mapped by config mapper, is not wrong. I probably could find dozen of such use cases in existing codebase, yet nobody fight with them.
I'll fix the socket leak if I can. I won't touch nit picks. You decide whether you do for users, continuous improvement or continuous waiting.
@lsiepel None of review checklist mention empty method bodies, sets over lists. Use of primitives over complex types is subjective matter related to nullnes, which in above case when elements are mapped by config mapper, is not wrong. I probably could find dozen of such use cases in existing codebase, yet nobody fight with them.
I'll fix the socket leak if I can. I won't touch nit picks. You decide whether you do for users, continuous improvement or continuous waiting.
Tried to fix all comments by a uploading a commit to your repository but permissions are lacking. Could you allow me to perform a commit?