-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[sunspec] Modbus: SunSpec Binding initial contribution #4220
Conversation
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/modbus-2-x-tcp-errors-on-startup/61707/4 |
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/modbus-and-system-air/6212/88 |
e215b97
to
99816ae
Compare
This seems to be necessary to get the children notified about the change Signed-off-by: Nagy Attila Gabor <mrbig@sneaker.hu>
Created discovery service based on the implementation in the bluetooth bundle Signed-off-by: Nagy Attila Gabor <mrbig@sneaker.hu>
SunSpec is a Modbus based specification for solar inverters, electricity maters, batteries and other connected appliance. This protocol allows monitoring of these devices and retrieving live data. This new binding extends the Modbus binding with the SunSpec specific things. Currently two new type of things are added: inverters (single, split, three-phase) and meters (single, split, wye/delta connected) Items are automatically discovered at an endpoint. Signed-off-by: Nagy Attila Gabor <mrbig@sneaker.hu>
Signed-off-by: Nagy Attila Gabor <mrbig@sneaker.hu>
56fdd56
to
64d658c
Compare
Signed-off-by: Nagy Attila Gabor <mrbig@sneaker.hu>
Signed-off-by: Nagy Attila Gabor <mrbig@sneaker.hu>
64d658c
to
0d0f297
Compare
Just a quick update: I've fixed a reported null pointer issue, rebased the PR to the current master and updated the licence texts according the up to date standards |
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/openhab1-2-nilan-heatpump/23538/72 |
</config-description> | ||
|
||
|
||
</config-description:config-descriptions> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no newline
</channels> | ||
</channel-group-type> | ||
|
||
</thing:thing-descriptions> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no newline
</channel-type> | ||
|
||
</thing:thing-descriptions> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whitespace in last line
</channels> | ||
</channel-group-type> | ||
|
||
</thing:thing-descriptions> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no newline
xsi:schemaLocation="http://eclipse.org/smarthome/schemas/thing-description/v1.0.0 http://eclipse.org/smarthome/schemas/thing-description-1.0.0.xsd"> | ||
|
||
<channel-group-type id="meter-ac-general"> | ||
<label>Summary</label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why no description? :)
* | ||
* @param manager the modbus manager from org.openhab.io.transport.modbus package | ||
*/ | ||
@Reference(service = ModbusManager.class, cardinality = ReferenceCardinality.MANDATORY, policy = ReferencePolicy.STATIC, unbind = "unsetManager") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove unbind (its implicit) and policy. The default is reasonable. Also MANDATORY is default.
*/ | ||
public class CommonModelBlock extends AbstractSunSpecMessageBlock { | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible, can you use immutable classes (everything set in the constructor, everything public final, no getters, no setters)? That helps for thread safety but also for reviewing, because less lines of boilerplate code.
try { | ||
new SunspecDiscoveryProcess(handler, listener).detectModel(); | ||
} catch (EndpointNotInitializedException ex) { | ||
logger.debug("Could not start discovery process"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Raise to "warn". Its an actual error that should be highlighted in the logs don't you think.
And also: Add the exception as last argument. That adds a stacktrace and the exception message to the log.
} | ||
|
||
/** | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm?
*/ | ||
private InverterModelParser parser = new InverterModelParser(); | ||
/** | ||
* Logger instance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commented the obvious
A binding of more than 5000 LOC is hard to review. I have noticed that you are doing two things in here as well. I know this is a bit more work (although not really), but will help to merge things faster. Can you make a separate PR for extending modbus first and then reduce this PR? I have left some inline comments. You sometimes comment the obvious which increases the code size. And you are using getters/setters although they could be avoided with immutable classes and other means. Please try to reduce the code size if you find more places. |
Dear @davidgraeff I understand that reviewing this much of change at once can be hard. I tried to help this work by organizing my work into commits by the goal of the change. So modbus changes are commits f86056 and ed1ac0 and the rest is the new binding. So if you like that better, I can quickly split this into two separate pull requests.
I'm in a bit trouble with this request. SunSpec model block can have 10-50 properties at least. Changing these to immutable classes as you requested would result in classes with a constructor with that many parameters, which is very hard to read and work with. Also that would need the controller plane to do first everything in local variables before instantiating the data object. I hardly believe this would reduce code size. On the other side if our goal is thread safety and real immutable classes, then we could use chainable setters that return a copy of the object with the selected property altered. That would work something like the BigDecimal. This would make the user side a bit more compact, so instead of this: MeterModelBlock block = new MeterModelBlock();
block.setSunspecDID(...);
block.setLength(...);
block.setAcCurrentTotal(...);
block.setAcCurrentPhaseA(...);
block.setAcCurrentPhaseB(...);
block.setAcCurrentPhaseC(...);
block.setAcCurrentSF(...);
block.setAcVoltageLinetoNAverage(...); one could write this: MeterModelBlock block = new MeterModelBlock()
.setSunspecDID(...)
.setLength(...)
.setAcCurrentTotal(...)
.setAcCurrentPhaseA(...)
.setAcCurrentPhaseB(...)
.setAcCurrentPhaseC(...)
.setAcCurrentSF(...)
.setAcVoltageLinetoNAverage(...)
[...] But apart of the removed new lines this does not save code either. One more design goal was to leave room for implementation divergences. It's very likely that there will be manufacturers that diverge from the standard either by purpose or by mistake. With the current architecture my goal was to have a clear separation of concerns: The most code could be eliminated by removing the parser/block layer, but this would result in overcrowded handler classes. Do you have any ideas which way to go from here? |
That would be awesome. The thing is: I can only accept the entire PR or nothing.
If the code is indeed modularized, then multiple PRs are very doable, I guess? Like in:
Can you have those in a "model" or "dto" package? That has been established as an accepted pattern
The architecture is quite good, it is just the pure size of the PR. |
Note that #5498 will break this PR since ModbusBitUtilities.extractStateFromRegisters will now return State, not DecimalType. Since this PR uses that method with integer valuetypes, it is safe to cast to DecimalType. EDIT: API is changing still a bit (not settling on |
@mrbig the description of you bundle sounds promising and I am looking forward to try it. Do you already have plans to get this finally merged? |
Thank you @ramack for your follow up on this. I started to reorganize the code to match up the new bundle organization, and to be able to split up into smaller PR-s. |
@mrbig FYI in case you didn't see it. There is a tutorial on the forum to guide you with the migration to the new build system: |
SunSpec is an open standard for solar inverters and other related devices to share data about their internal state. The standard is implemented by several vendors like ABB, Fronius, SMA, Schneider Electric, Solaredge, etc. The goal of this work is to add user friendly support of these devices to openHAB. The standard is built on the Modbus protocol, so this work is heavily based on the Modbus binding. Also the Bluetooth binding and several of it's solutions were taken as an example how to extend an already existing binding with a new bundle. Related issue: openhab#3216 This is a stripped down version of a SunSpec bundle aimed to ease reviewing. Original PW with all the features can be found here: https://github.com/openhab/openhab2-addons/pull/4220 This version contains only a limited support for single phase inverters, but no auto discovery or any other fancy features are included. Other changes from my original PR: - migrated to the new build system - using NonNull values wherever possible - modell classes were moved to a dto package and highly simplified - other minor code changes Signed-off-by: Nagy Attila Gabor <mrbig@sneaker.hu>
I am also interested in this binding. The PR is 16 days old now. |
SunSpec is an open standard for solar inverters and other related devices to share data about their internal state. The standard is implemented by several vendors like ABB, Fronius, SMA, Schneider Electric, Solaredge, etc. The goal of this work is to add user friendly support of these devices to openHAB. The standard is built on the Modbus protocol, so this work is heavily based on the Modbus binding. Also the Bluetooth binding and several of it's solutions were taken as an example how to extend an already existing binding with a new bundle. Related issue: openhab#3216 This is a stripped down version of a SunSpec bundle aimed to ease reviewing. Original PW with all the features can be found here: https://github.com/openhab/openhab2-addons/pull/4220 This version contains only a limited support for single phase inverters, but no auto discovery or any other fancy features are included. Other changes from my original PR: - migrated to the new build system - using NonNull values wherever possible - modell classes were moved to a dto package and highly simplified - other minor code changes Signed-off-by: Nagy Attila Gabor <mrbig@sneaker.hu>
SunSpec is an open standard for solar inverters and other related devices to share data about their internal state. The standard is implemented by several vendors like ABB, Fronius, SMA, Schneider Electric, Solaredge, etc. The goal of this work is to add user friendly support of these devices to openHAB. The standard is built on the Modbus protocol, so this work is heavily based on the Modbus binding. Also the Bluetooth binding and several of it's solutions were taken as an example how to extend an already existing binding with a new bundle. Related issue: openhab#3216 This is a stripped down version of a SunSpec bundle aimed to ease reviewing. Original PW with all the features can be found here: https://github.com/openhab/openhab2-addons/pull/4220 This version contains only a limited support for single phase inverters, but no auto discovery or any other fancy features are included. Other changes from my original PR: - migrated to the new build system - using NonNull values wherever possible - modell classes were moved to a dto package and highly simplified - other minor code changes Signed-off-by: Nagy Attila Gabor <mrbig@sneaker.hu>
SunSpec is an open standard for solar inverters and other related devices to share data about their internal state. The standard is implemented by several vendors like ABB, Fronius, SMA, Schneider Electric, Solaredge, etc. The goal of this work is to add user friendly support of these devices to openHAB. The standard is built on the Modbus protocol, so this work is heavily based on the Modbus binding. Also the Bluetooth binding and several of it's solutions were taken as an example how to extend an already existing binding with a new bundle. Related issue: openhab#3216 This is a stripped down version of a SunSpec bundle aimed to ease reviewing. Original PW with all the features can be found here: https://github.com/openhab/openhab2-addons/pull/4220 This version contains only a limited support for single phase inverters, but no auto discovery or any other fancy features are included. Other changes from my original PR: - migrated to the new build system - using NonNull values wherever possible - modell classes were moved to a dto package and highly simplified - other minor code changes Signed-off-by: Nagy Attila Gabor <mrbig@sneaker.hu>
* [sunspec] Modbus: SunSpec bundle basic version SunSpec is an open standard for solar inverters and other related devices to share data about their internal state. The standard is implemented by several vendors like ABB, Fronius, SMA, Schneider Electric, Solaredge, etc. The goal of this work is to add user friendly support of these devices to openHAB. The standard is built on the Modbus protocol, so this work is heavily based on the Modbus binding. Also the Bluetooth binding and several of it's solutions were taken as an example how to extend an already existing binding with a new bundle. Related issue: #3216 This is a stripped down version of a SunSpec bundle aimed to ease reviewing. Original PW with all the features can be found here: https://github.com/openhab/openhab2-addons/pull/4220 This version contains only a limited support for single phase inverters, but no auto discovery or any other fancy features are included. Other changes from my original PR: - migrated to the new build system - using NonNull values wherever possible - modell classes were moved to a dto package and highly simplified - other minor code changes Signed-off-by: Nagy Attila Gabor <mrbig@sneaker.hu>
* [sunspec] Modbus: SunSpec bundle basic version SunSpec is an open standard for solar inverters and other related devices to share data about their internal state. The standard is implemented by several vendors like ABB, Fronius, SMA, Schneider Electric, Solaredge, etc. The goal of this work is to add user friendly support of these devices to openHAB. The standard is built on the Modbus protocol, so this work is heavily based on the Modbus binding. Also the Bluetooth binding and several of it's solutions were taken as an example how to extend an already existing binding with a new bundle. Related issue: openhab#3216 This is a stripped down version of a SunSpec bundle aimed to ease reviewing. Original PW with all the features can be found here: https://github.com/openhab/openhab2-addons/pull/4220 This version contains only a limited support for single phase inverters, but no auto discovery or any other fancy features are included. Other changes from my original PR: - migrated to the new build system - using NonNull values wherever possible - modell classes were moved to a dto package and highly simplified - other minor code changes Signed-off-by: Nagy Attila Gabor <mrbig@sneaker.hu> Signed-off-by: Eugen Freiter <freiter@gmx.de>
* [sunspec] Modbus: SunSpec bundle basic version SunSpec is an open standard for solar inverters and other related devices to share data about their internal state. The standard is implemented by several vendors like ABB, Fronius, SMA, Schneider Electric, Solaredge, etc. The goal of this work is to add user friendly support of these devices to openHAB. The standard is built on the Modbus protocol, so this work is heavily based on the Modbus binding. Also the Bluetooth binding and several of it's solutions were taken as an example how to extend an already existing binding with a new bundle. Related issue: openhab#3216 This is a stripped down version of a SunSpec bundle aimed to ease reviewing. Original PW with all the features can be found here: https://github.com/openhab/openhab2-addons/pull/4220 This version contains only a limited support for single phase inverters, but no auto discovery or any other fancy features are included. Other changes from my original PR: - migrated to the new build system - using NonNull values wherever possible - modell classes were moved to a dto package and highly simplified - other minor code changes Signed-off-by: Nagy Attila Gabor <mrbig@sneaker.hu>
* [sunspec] Modbus: SunSpec bundle basic version SunSpec is an open standard for solar inverters and other related devices to share data about their internal state. The standard is implemented by several vendors like ABB, Fronius, SMA, Schneider Electric, Solaredge, etc. The goal of this work is to add user friendly support of these devices to openHAB. The standard is built on the Modbus protocol, so this work is heavily based on the Modbus binding. Also the Bluetooth binding and several of it's solutions were taken as an example how to extend an already existing binding with a new bundle. Related issue: openhab#3216 This is a stripped down version of a SunSpec bundle aimed to ease reviewing. Original PW with all the features can be found here: https://github.com/openhab/openhab2-addons/pull/4220 This version contains only a limited support for single phase inverters, but no auto discovery or any other fancy features are included. Other changes from my original PR: - migrated to the new build system - using NonNull values wherever possible - modell classes were moved to a dto package and highly simplified - other minor code changes Signed-off-by: Nagy Attila Gabor <mrbig@sneaker.hu>
* [sunspec] Modbus: SunSpec bundle basic version SunSpec is an open standard for solar inverters and other related devices to share data about their internal state. The standard is implemented by several vendors like ABB, Fronius, SMA, Schneider Electric, Solaredge, etc. The goal of this work is to add user friendly support of these devices to openHAB. The standard is built on the Modbus protocol, so this work is heavily based on the Modbus binding. Also the Bluetooth binding and several of it's solutions were taken as an example how to extend an already existing binding with a new bundle. Related issue: openhab#3216 This is a stripped down version of a SunSpec bundle aimed to ease reviewing. Original PW with all the features can be found here: https://github.com/openhab/openhab2-addons/pull/4220 This version contains only a limited support for single phase inverters, but no auto discovery or any other fancy features are included. Other changes from my original PR: - migrated to the new build system - using NonNull values wherever possible - modell classes were moved to a dto package and highly simplified - other minor code changes Signed-off-by: Nagy Attila Gabor <mrbig@sneaker.hu>
* [sunspec] Modbus: SunSpec bundle basic version SunSpec is an open standard for solar inverters and other related devices to share data about their internal state. The standard is implemented by several vendors like ABB, Fronius, SMA, Schneider Electric, Solaredge, etc. The goal of this work is to add user friendly support of these devices to openHAB. The standard is built on the Modbus protocol, so this work is heavily based on the Modbus binding. Also the Bluetooth binding and several of it's solutions were taken as an example how to extend an already existing binding with a new bundle. Related issue: openhab#3216 This is a stripped down version of a SunSpec bundle aimed to ease reviewing. Original PW with all the features can be found here: https://github.com/openhab/openhab2-addons/pull/4220 This version contains only a limited support for single phase inverters, but no auto discovery or any other fancy features are included. Other changes from my original PR: - migrated to the new build system - using NonNull values wherever possible - modell classes were moved to a dto package and highly simplified - other minor code changes Signed-off-by: Nagy Attila Gabor <mrbig@sneaker.hu> Signed-off-by: CSchlipp <christian@schlipp.de>
* [sunspec] Modbus: SunSpec bundle basic version SunSpec is an open standard for solar inverters and other related devices to share data about their internal state. The standard is implemented by several vendors like ABB, Fronius, SMA, Schneider Electric, Solaredge, etc. The goal of this work is to add user friendly support of these devices to openHAB. The standard is built on the Modbus protocol, so this work is heavily based on the Modbus binding. Also the Bluetooth binding and several of it's solutions were taken as an example how to extend an already existing binding with a new bundle. Related issue: openhab#3216 This is a stripped down version of a SunSpec bundle aimed to ease reviewing. Original PW with all the features can be found here: https://github.com/openhab/openhab2-addons/pull/4220 This version contains only a limited support for single phase inverters, but no auto discovery or any other fancy features are included. Other changes from my original PR: - migrated to the new build system - using NonNull values wherever possible - modell classes were moved to a dto package and highly simplified - other minor code changes Signed-off-by: Nagy Attila Gabor <mrbig@sneaker.hu>
* [sunspec] Modbus: SunSpec bundle basic version SunSpec is an open standard for solar inverters and other related devices to share data about their internal state. The standard is implemented by several vendors like ABB, Fronius, SMA, Schneider Electric, Solaredge, etc. The goal of this work is to add user friendly support of these devices to openHAB. The standard is built on the Modbus protocol, so this work is heavily based on the Modbus binding. Also the Bluetooth binding and several of it's solutions were taken as an example how to extend an already existing binding with a new bundle. Related issue: openhab#3216 This is a stripped down version of a SunSpec bundle aimed to ease reviewing. Original PW with all the features can be found here: https://github.com/openhab/openhab2-addons/pull/4220 This version contains only a limited support for single phase inverters, but no auto discovery or any other fancy features are included. Other changes from my original PR: - migrated to the new build system - using NonNull values wherever possible - modell classes were moved to a dto package and highly simplified - other minor code changes Signed-off-by: Nagy Attila Gabor <mrbig@sneaker.hu>
* [sunspec] Modbus: SunSpec bundle basic version SunSpec is an open standard for solar inverters and other related devices to share data about their internal state. The standard is implemented by several vendors like ABB, Fronius, SMA, Schneider Electric, Solaredge, etc. The goal of this work is to add user friendly support of these devices to openHAB. The standard is built on the Modbus protocol, so this work is heavily based on the Modbus binding. Also the Bluetooth binding and several of it's solutions were taken as an example how to extend an already existing binding with a new bundle. Related issue: openhab#3216 This is a stripped down version of a SunSpec bundle aimed to ease reviewing. Original PW with all the features can be found here: https://github.com/openhab/openhab2-addons/pull/4220 This version contains only a limited support for single phase inverters, but no auto discovery or any other fancy features are included. Other changes from my original PR: - migrated to the new build system - using NonNull values wherever possible - modell classes were moved to a dto package and highly simplified - other minor code changes Signed-off-by: Nagy Attila Gabor <mrbig@sneaker.hu>
* [sunspec] Modbus: SunSpec bundle basic version SunSpec is an open standard for solar inverters and other related devices to share data about their internal state. The standard is implemented by several vendors like ABB, Fronius, SMA, Schneider Electric, Solaredge, etc. The goal of this work is to add user friendly support of these devices to openHAB. The standard is built on the Modbus protocol, so this work is heavily based on the Modbus binding. Also the Bluetooth binding and several of it's solutions were taken as an example how to extend an already existing binding with a new bundle. Related issue: openhab#3216 This is a stripped down version of a SunSpec bundle aimed to ease reviewing. Original PW with all the features can be found here: https://github.com/openhab/openhab2-addons/pull/4220 This version contains only a limited support for single phase inverters, but no auto discovery or any other fancy features are included. Other changes from my original PR: - migrated to the new build system - using NonNull values wherever possible - modell classes were moved to a dto package and highly simplified - other minor code changes Signed-off-by: Nagy Attila Gabor <mrbig@sneaker.hu>
* [sunspec] Modbus: SunSpec bundle basic version SunSpec is an open standard for solar inverters and other related devices to share data about their internal state. The standard is implemented by several vendors like ABB, Fronius, SMA, Schneider Electric, Solaredge, etc. The goal of this work is to add user friendly support of these devices to openHAB. The standard is built on the Modbus protocol, so this work is heavily based on the Modbus binding. Also the Bluetooth binding and several of it's solutions were taken as an example how to extend an already existing binding with a new bundle. Related issue: openhab#3216 This is a stripped down version of a SunSpec bundle aimed to ease reviewing. Original PW with all the features can be found here: https://github.com/openhab/openhab2-addons/pull/4220 This version contains only a limited support for single phase inverters, but no auto discovery or any other fancy features are included. Other changes from my original PR: - migrated to the new build system - using NonNull values wherever possible - modell classes were moved to a dto package and highly simplified - other minor code changes Signed-off-by: Nagy Attila Gabor <mrbig@sneaker.hu> Signed-off-by: Daan Meijer <daan@studioseptember.nl>
* [sunspec] Modbus: SunSpec bundle basic version SunSpec is an open standard for solar inverters and other related devices to share data about their internal state. The standard is implemented by several vendors like ABB, Fronius, SMA, Schneider Electric, Solaredge, etc. The goal of this work is to add user friendly support of these devices to openHAB. The standard is built on the Modbus protocol, so this work is heavily based on the Modbus binding. Also the Bluetooth binding and several of it's solutions were taken as an example how to extend an already existing binding with a new bundle. Related issue: openhab#3216 This is a stripped down version of a SunSpec bundle aimed to ease reviewing. Original PW with all the features can be found here: https://github.com/openhab/openhab2-addons/pull/4220 This version contains only a limited support for single phase inverters, but no auto discovery or any other fancy features are included. Other changes from my original PR: - migrated to the new build system - using NonNull values wherever possible - modell classes were moved to a dto package and highly simplified - other minor code changes Signed-off-by: Nagy Attila Gabor <mrbig@sneaker.hu>
[modbus] SunSpec bundle - add support for SunSpec compatible inverters and meters
SunSpec is an open standard for solar inverters and other related devices to share data about their internal state. The standard is implemented by several vendors like ABB, Fronius, SMA, Schneider Electric, Solaredge, etc.
The goal of this work is to add user friendly support of these devices to openHAB.
The standard is built on the Modbus protocol, so this work is heavily based on the Modbus binding. Also the Bluetooth binding and several of it's solutions were taken as an example how to extend an already existing binding with a new bundle.
Related issue: #3216
In this PR two model types are supported: inverters and meters. The structure allows for easy extension with other model types.
This PR contains two more commits to the Modbus binding: one adds the framework for auto discovery, and the other handles a minor issue where connected items to the Modbus bridge did not get notified about bridge changes.
I'm submitting my work for initial review and to get your feedback. Thank you for reviewing my work.