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

[e3dc] E3DC Home Power Plant Binding contribution #8172

Merged
merged 67 commits into from
Aug 27, 2020
Merged

[e3dc] E3DC Home Power Plant Binding contribution #8172

merged 67 commits into from
Aug 27, 2020

Conversation

weymann
Copy link
Contributor

@weymann weymann commented Jul 22, 2020

Binding for the E3DC Home Power Plant (https://www.e3dc.com/). It provides all information regarding Photovoltaic Production, Household Electric Power Consumption, detailed Photovoltaic String values, Emergency Power Settings and attached Wallboxes. Some Wallbox Settings can be changed and give you even more Control. It’s based on org.openhab.io.transport.modbus

I didn’t choose a pure Modbus Binding for the following reasons:

  • Many Modbus registers shall interpreted as Strings. It’s not convenient to allocate 16 registers and concatenate the Bytes to a String
  • Some Modbus registers shall be interpreted as Bits. The registers are declared as uint16 and can be written as uint16. But you have to do the math e.g. in rules. In my eyes also not very convenient.
  • Some values are negative. Battery and Grid can be either Power Suppliers or Consumers. This is somehow inconsistent because Household Consumption is a positive value but also Photovoltaic Production is positive.

The overall design looks like this: org.openhab.io.transport.modus bundle polls Modbus registers and provides byte arrays. The Data Transfer Objects are translating bytes into Strings, Numbers and Bits. Handlers takes care to distribute the data to the correct Channels.

Test bundle plus configuration is available here https://github.com/weymann/openhab-addons/tree/e3dc-test/bundles/org.openhab.binding.e3dc/test

@weymann weymann requested a review from a team as a code owner July 22, 2020 17:25
@fwolter
Copy link
Member

fwolter commented Jul 22, 2020

Please stick with this PR. If you messed up rebasing, it can be fixed in this PR without creating a new one.

@fwolter fwolter added the new binding If someone has started to work on a binding. For a new binding PR. label Jul 22, 2020
@weymann
Copy link
Contributor Author

weymann commented Jul 23, 2020

Please stick with this PR. If you messed up rebasing, it can be fixed in this PR without creating a new one.
@fwolter Sorry for the confusion. I messed up my fork so the previous PR went wrong. Also missed to check the changes in beforehand - in my fork changes looked good.
Now everything is clean and I made an update from upstream. Also adapted the code to the new io.transport.modbus API

@TravisBuddy
Copy link

Travis tests have failed

Hey @weymann,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

@weymann
Copy link
Contributor Author

weymann commented Jul 24, 2020

I get errors on CI regarding openjdk11 for org.openhab.io.transport.modbus
image

Is there a todo on my side to resolve this error?

@fwolter
Copy link
Member

fwolter commented Jul 26, 2020

It's a general issue. Nothing has to be done from your side.

@TravisBuddy
Copy link

Travis tests have failed

Hey @weymann,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution, again! Here is my feedback.

Did you take a look into the modbus binding? Maybe you could've saved some code by utilizing it like #8163.

@ssalonen
Copy link
Contributor

ssalonen commented Jul 27, 2020

Hi,

maintainer of the modbus binding/transport here. Happy to see new binding popping again!

I hope you would reconsider that you would "extend" on top of the modbus binding with a device specific modbus binding, basing on binding.modbus -- did you consider this option?

I can see how you want to avoid complex rules etc., this is exactly the use case for device specific bindings!

Instead of directly interfacing with the transport, you would reuse tcp/serial endpoints from the modbus binding as parent thing for the actual device thing. You would not need (and probably shouldn't) use the poller and data things.

Many (all?) other modbus-based bindings follow the same approach,

This allows user to configure the connection details (e.g. retry interval, how long to keep the connection open, connection retry interval etc.).

A comment on thing hierarchy:

Some bindings have opted for single thing (representing the physical device?) with multiple channel groups, making the configuration even more simple. See e.g. sunspec docs .

If you always poll the same registers and there's no variation (e.g. different model variations represented by "Blocks"), this could make sense here as well -- reducing the number "things" user needs to configure and define? All data would be readily available, one just needs to link it to items.

@weymann
Copy link
Contributor Author

weymann commented Jul 27, 2020

Hi @ssalonen, hi @fwolter,

thanks for giving feedback to my Binding contribution. Let me explain my design approach and the reason why I started this way:

I got my E3DC system in April 2020 and I prepared my openHAB setup based on this Community post: https://community.openhab.org/t/e3dc-with-the-new-modbus-binding/51345
This worked very smooth and I got the necessary data for power measuring.

In June a new Modbus Spec (only available in German) was released by E3DC which I placed temporarily here https://github.com/weymann/openhab-addons/blob/e3dc-test/bundles/org.openhab.binding.e3dc/test/spec/ModBus_E3DC_Speichersysteme_V1.70_2020-06-18.pdf. With this some of my problems began:

First aspect
The first 67 registers shall be interpreted as Strings. This information is quite handy raising tickets towards the manufacturer: Serial Number, Firmware Release, supported Modbus version. These values are not really static and will change from “time to time”. I applied a poll time of 1 hour.
Side note: I measured the cycling times of Modbus polling inclusive update channels. Times on my Raspi 3+ are roughly ~40ms without String decoding and ~60ms with String decoding with polling time of 2 seconds

Second aspect
Registers for EMS and Wallbox are uint16 but shall be interpreted as Bits. It’s no problem for reading as shown in the Community post. But for writing I have to convert the bits into a number and write it back. It wasn’t very convenient implementing rules for this.

Third aspect
Grid and Battery are acting as Power Producer and Power Consumer. Registers like Household consumption are positive while Photovoltaic producer is also positive. Also some rules are necessary to apply positive values for all.

My conclusion
Having this 3 aspects in mind I decided to go the middle approach reusing io.transport.modbus which is really comfortable for programming. I’m really interested in your view either to have a Modbus Binding plus additional rules are needed to apply these conversions or to go with io.transport.modbus providing a more “user friendly interface” but of course with slightly lesser control.

@ssalonen
Copy link
Contributor

Hi @weymann thanks for the reply. Yeah I noticed I as well replied to the original thread at the time (https://community.openhab.org/t/e3dc-with-the-new-modbus-binding/51345/2) :)

I think you are misinterpreting my suggestion, let me try to re-explain.

I'm not advicing to come up with complex rules to process the data and force yourself to work with generic modbus binding. The whole setup is designed to accomodate device specific bindings, that's completely OK.

Instead, I'm advicing you to follow the advice set in binding.modbus/DEVELOPERS.MD, a pattern established by modbus.sunspec and now followed up by many upcoming modbus based bindings (open PRs linked above). In your case discovery is not wanted, please ignore it.

In essence: you would still interact with the transport bundle, that's OK. You would just reuse the tcp and serial things (from the regular modbus binding), and make them parent thing of your thing representing the e3dc device.

Thing hierarchy could look like (keeping blocks as separate thing types)

  • tcp/serial thing
    • e3dc-info
    • e3dc-power
    • ...

User would need no additional rules or processing in place -- that's the job of the binding as now. You would still interact with ModbusCommunicationInterface to read/write data from the slave, you can get access to it using tcp/serial ThingHandler's getCommunicationInterface() method.

You might ask why do it this way?

  1. by reusing tcp and serial thing, you make the binding easily applicable to both serial and tcp connections (*). This is perhaps more relevant with natively serial slaves that might be accessed by openHAB using tcp if one is using separate hardware serial/tcp modbus gateway converter. If you decide, you can decide to support only tcp things however, it's up to you. In the future you would get benefits of modbus binding itself developing, e.g. bringing support to RTU-over-TCP. In the old times we had custom bindings flying around in the forums, but only applicable to tcp and/or serial. The separation of modbus type
  2. tcp thing has several parameters to tune connection details, e.g. timeBetweenTransactionsMillis (throttling requests), reconnectAfterMillis (how long to keep the connection open), retry for connections. In practice this helps users to tune to system to avoid unnecessary errors (e.g. due to slave being too busy to respond) and maximize performance.
  3. it's consistent with other modbus bindings, keeping the user experience the same and follow the same logic even if you are hooking to several different modbus based devices.

(*) Not sure how relevant this argument is with this particular device. If it is only available as TCP, I have hard time seeing anyone converting modbus TCP to Modbus RTU serial.

Obvious downside is that you end up having one more thing representing the connection type to the device.


My other comments was related to channel / thing setup. This is a separate discussion.

A completely separate discussion is whether you should combine all the different e3dc thing types into one thing type. Instead of having e3dc-info, e3dc-power, etc., you would only have one thing type e3dc. The same data would be available, but just using channel groups, similar to how sunspec has deviceInformation, acGeneral, etc. groups.

In general I have understood this is the intention with the thing system (docs), trying to keep openHAB things represent physical things if possible, and channels exposing thing's functionality.

@ssalonen
Copy link
Contributor

ssalonen commented Jul 28, 2020

To make my point more clear, see this quick & dirty conversion of having single thing, using tcp as parent thing. All information is stored as channel groups of that one thing

ssalonen@8f5b53f

Hopefully this made my message more clear. Let me know what you think

@weymann
Copy link
Contributor Author

weymann commented Jul 29, 2020

Thanks @ssalonen for the code example, I think I got the idea. I'll put the "work in progress" label onto this Pull Request and evaluate your example in my enrionment. Give me some time to figure it out and I'll come back with an updated version.

@weymann weymann changed the title [e3dc] E3DC Home Power Plant Binding contribution [wip] [e3dc] E3DC Home Power Plant Binding contribution Jul 29, 2020
@fwolter fwolter added the work in progress A PR that is not yet ready to be merged label Jul 29, 2020
@ssalonen
Copy link
Contributor

@weymann glad to hear it! Let me know if I can help out in any way

@weymann
Copy link
Contributor Author

weymann commented Jul 30, 2020

Hey @ssalonen ! Time for a short pre-review?
I adapted my binding towards modbus. To be honest I like the result pretty much. I added E3DC Device with 4 different channel groups. These groups are reflecting my previous "Blocks" pretty well. Additionally the Wallbox is an extra Thing with config "wallboxId". This avoids the allocation of 8 channels for each of the possible 8 Wallboxes => 64 channels. The E3DC Device acts as a Bridge and this represents quite well the "real installation" - each Wallbox is connected towards the right device.

Perhaps you can give me a helping hand on modbus error handling. I don't know the appropriate actions if an error occurs. Do I have to stop the Poller? Is it automatically stopped and if yes shall I introduce a timer for recovery?

Test bundle is updated here https://github.com/weymann/openhab-addons/tree/e3dc-test/bundles/org.openhab.binding.e3dc/test

@ssalonen
Copy link
Contributor

ssalonen commented Aug 1, 2020

I had a quick look and looks like good stuff to me!

With errors you probably want to update thing statuses to offline with COMMUNICATION_ERROR as reason. You can also provide textual description of what failed (was it eg the write or reading of data).

I would keep the polling going, essentially retrying again after a while. Thing status should be updated back to ONLINE once read / write is successful.

@ssalonen
Copy link
Contributor

ssalonen commented Aug 1, 2020

Btw, a small thing

If you want to remove some code you could listen for children things with childHandlerInitialized and childHandlerDisposed, and tracking them in a collection. I don't think you really need DataListener, you could just deal with the concrete wall box class in e3dc thing.

In fact, are the wall box data listeners deregistered in the current implementation?

Check out poller implementation for inspiration https://github.com/openhab/openhab-addons/blob/2.5.x/bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/handler/ModbusPollerThingHandler.java

@ssalonen
Copy link
Contributor

ssalonen commented Aug 1, 2020

Now with #8218 in, please also handle the EndpointaNotInitialized exception

Signed-off-by: Bernd <bernd.weymann@gmail.com>
Signed-off-by: Bernd <bernd.weymann@gmail.com>
Signed-off-by: Bernd <bernd.weymann@gmail.com>
Signed-off-by: Bernd <bernd.weymann@gmail.com>
Signed-off-by: Bernd <bernd.weymann@gmail.com>
Signed-off-by: Bernd <bernd.weymann@gmail.com>
Signed-off-by: Bernd <bernd.weymann@gmail.com>
Signed-off-by: Bernd <bernd.weymann@gmail.com>
@TravisBuddy
Copy link

Travis tests were successful

Hey @weymann,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Signed-off-by: Bernd <bernd.weymann@gmail.com>
@TravisBuddy
Copy link

Travis tests were successful

Hey @weymann,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@weymann weymann requested a review from fwolter August 9, 2020 17:58
Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your changes look good! I found a few minor things I overlooked during the first review. Sorry for that.

Signed-off-by: Bernd <bernd.weymann@gmail.com>
@TravisBuddy
Copy link

Travis tests were successful

Hey @weymann,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Signed-off-by: Bernd <bernd.weymann@gmail.com>
@TravisBuddy
Copy link

Travis tests were successful

Hey @weymann,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@weymann weymann requested a review from fwolter August 14, 2020 11:42
Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fwolter fwolter added cre Coordinated Review Effort and removed work in progress A PR that is not yet ready to be merged labels Aug 14, 2020
Copy link
Member

@Hilbrand Hilbrand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for you contribution. I've not really something to comment, just a question (and a informative suggestion).

xmlns:thing="https://openhab.org/schemas/thing-description/v1.0.0"
xsi:schemaLocation="https://openhab.org/schemas/thing-description/v1.0.0 https://openhab.org/schemas/thing-description-1.0.0.xsd">
<channel-group-type id="info-values">
<label>Information</label>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These channels look more like properties of a thing/bridge? Maybe this was discussed before, but is there a reason to create channels instead of properties?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was one remark regarding properties and as result the implementation contains a "low speed" update of these registers while Data is updated frequently.
Several enrties will change due to remote updates like Modbus Firmware Version, Supported Registers and Firmware. After the Device Installation the Serial Number wasn't set and was updated remotely. Also the Modbus-ID can change because you can operate the device either in Sunspec Mode or in the Manufacturers own Mode.

In addition I would like to have these values in my UI. It's quite handy to check currently installed Software Versions for Support. With Properties I have to change to PaperUI, search the device and check the properties. On Smartphone I didn't find a possibility to check properties yet.

Signed-off-by: Bernd <bernd.weymann@gmail.com>
@TravisBuddy
Copy link

Travis tests were successful

Hey @weymann,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@Hilbrand Hilbrand merged commit 6cd0cb0 into openhab:2.5.x Aug 27, 2020
@Hilbrand Hilbrand added this to the 2.5.9 milestone Aug 27, 2020
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
Signed-off-by: Bernd <bernd.weymann@gmail.com>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
Signed-off-by: Bernd <bernd.weymann@gmail.com>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
Signed-off-by: Bernd <bernd.weymann@gmail.com>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
Signed-off-by: Bernd <bernd.weymann@gmail.com>
DaanMeijer pushed a commit to DaanMeijer/openhab-addons that referenced this pull request Sep 1, 2020
Signed-off-by: Bernd <bernd.weymann@gmail.com>
Signed-off-by: Daan Meijer <daan@studioseptember.nl>
CSchlipp pushed a commit to CSchlipp/openhab-addons that referenced this pull request Sep 12, 2020
Signed-off-by: Bernd <bernd.weymann@gmail.com>
markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Sep 19, 2020
Signed-off-by: Bernd <bernd.weymann@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cre Coordinated Review Effort new binding If someone has started to work on a binding. For a new binding PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants