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.

Sign up for GitHub

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

Jump to bottom

[viessmann] Initial contribution of Viessmann Binding #212

Merged
merged 12 commits into from
Nov 17, 2021
Merged

[viessmann] Initial contribution of Viessmann Binding #212

merged 12 commits into from
Nov 17, 2021

Conversation

rogrun
Copy link
Contributor

@rogrun rogrun commented Nov 3, 2021

This binding connects openHAB with the new Viessmann API

@rogrun rogrun requested a review from J-N-K as a code owner November 3, 2021 13:34
@J-N-K
Copy link
Member

J-N-K commented Nov 3, 2021

Thanks, I'll try to review as soon as possible. 5500 lines may take a while though.

@rogrun
Copy link
Contributor Author

rogrun commented Nov 3, 2021

Please wait to merge. I need to make some fixes. I'll give you feedback when it's done

@rogrun
Copy link
Contributor Author

rogrun commented Nov 6, 2021

All fixes are done. Ready to merge.

J-N-K
J-N-K requested changes Nov 8, 2021
View reviewed changes
Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

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

Thanks, in general looks good. I have left some comments:

  • The comments regarding .equals and @SerializedName also apply to the other DTO classes.
  • Why do you need the applet?

bundles/org.smarthomej.binding.viessmann/README.md Outdated Show resolved Hide resolved
bundles/org.smarthomej.binding.viessmann/README.md Outdated Show resolved Hide resolved
bundles/org.smarthomej.binding.viessmann/README.md Outdated Show resolved Hide resolved
bundles/org.smarthomej.binding.viessmann/README.md Outdated Show resolved Hide resolved
bundles/org.smarthomej.binding.viessmann/README.md Outdated Show resolved Hide resolved
.../src/main/java/org/smarthomej/binding/viessmann/internal/handler/ViessmannBridgeHandler.java Outdated Show resolved Hide resolved
.../src/main/java/org/smarthomej/binding/viessmann/internal/handler/ViessmannBridgeHandler.java Outdated Show resolved Hide resolved
.../src/main/java/org/smarthomej/binding/viessmann/internal/handler/ViessmannBridgeHandler.java Outdated Show resolved Hide resolved
bundles/org.smarthomej.binding.viessmann/src/main/resources/OH-INF/thing/thing-types.xml Outdated Show resolved Hide resolved
bundles/org.smarthomej.binding.viessmann/src/main/resources/OH-INF/thing/thing-types.xml Outdated Show resolved Hide resolved
@rogrun
Copy link
Contributor Author

rogrun commented Nov 8, 2021

Thx for review. I need some time to make these changes.

@rogrun
Copy link
Contributor Author

rogrun commented Nov 9, 2021

Why do you need the applet?

I need the applet to parse the JSON response. I don't know if there might be a better way.

Here is an example response: https://lu-media.de/viessmann-api/example.json

@J-N-K
Copy link
Member

J-N-K commented Nov 9, 2021

Servlet: I didn't see that you need it for the redirect. I'm fine with that. But please use viessmann or viessmann/api instead of viessmann-api. In general servants are registered under their binding name to avoid collisions between different bindings.

rogrun added a commit to rogrun/addons that referenced this pull request Nov 9, 2021
@rogrun
[viessmann] requested changes for smarthomej#212
861501d 
Signed-off-by: Ronny Grun <ronny.grun@t-online.de>
@rogrun
Copy link
Contributor Author

rogrun commented Nov 10, 2021

I've made all the changes. But why does the build fail on Travis CI?

@J-N-K
Copy link
Member

J-N-K commented Nov 10, 2021

Now you moved the same issue to the other CI :-)

You need to rebase on the upstream 3.2.x branch. The command depends on your setup. If you don't know how to do that, please send me the .git/config of your system via private mail and I'll assist you.

@rogrun
Copy link
Contributor Author

rogrun commented Nov 10, 2021

Oh. Yes I see. I wrote you an email

@rogrun
[viessmann] Initial contribution
00a55cb 
This binding connects openHAB with the new Viessmann API

Signed-off-by: Ronny Grun <ronny.grun@t-online.de>
@rogrun
[viessmann] add pollingInterval to bridgeconfig
9fd53cf 
it allows to set manual the interval by user

Signed-off-by: Ronny Grun <ronny.grun@t-online.de>
@rogrun
[viessmann] fix entries for statistic feature
fc3fb32 
Signed-off-by: Ronny Grun <ronny.grun@t-online.de>
@rogrun
[viessmann] requested changes for smarthomej#212
bcef588 
Signed-off-by: Ronny Grun <ronny.grun@t-online.de>
@rogrun
[viessmann] increase version
887de03 
Signed-off-by: Ronny Grun <ronny.grun@t-online.de>
@rogrun
[viessmann] add new features
7295e8c 
Signed-off-by: Ronny Grun <ronny.grun@t-online.de>
@rogrun
[viessmann] fix version
bf2d816 
Signed-off-by: Ronny Grun <ronny.grun@t-online.de>
@rogrun
[viessmann] fix version
bb7bb74 
Signed-off-by: Ronny Grun <ronny.grun@t-online.de>
@rogrun
[viessmann] fix MemberNameCheck
dc2f84e 
Signed-off-by: Ronny Grun <ronny.grun@t-online.de>
@rogrun
Copy link
Contributor Author

rogrun commented Nov 11, 2021

Now everything is ready to be merged.

@rogrun rogrun requested a review from J-N-K November 13, 2021 09:55
J-N-K
J-N-K requested changes Nov 13, 2021
View reviewed changes
Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

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

Thanks, already looks quite good. I have left some more comments, but they should be easy to address.

bundles/org.smarthomej.binding.viessmann/README.md Outdated Show resolved Hide resolved
bundles/org.smarthomej.binding.viessmann/README.md Outdated Show resolved Hide resolved
bundles/org.smarthomej.binding.viessmann/README.md Outdated Show resolved Hide resolved
...inding.viessmann/src/main/java/org/smarthomej/binding/viessmann/internal/BindingServlet.java Outdated Show resolved Hide resolved
...smann/src/main/java/org/smarthomej/binding/viessmann/internal/ViessmannBindingConstants.java Outdated Show resolved Hide resolved
.../src/main/java/org/smarthomej/binding/viessmann/internal/handler/ViessmannBridgeHandler.java Outdated Show resolved Hide resolved
.../src/main/java/org/smarthomej/binding/viessmann/internal/handler/ViessmannBridgeHandler.java Outdated Show resolved Hide resolved
bundles/org.smarthomej.binding.viessmann/src/main/resources/OH-INF/thing/thing-types.xml Outdated Show resolved Hide resolved
bundles/org.smarthomej.binding.viessmann/src/main/resources/OH-INF/thing/thing-types.xml Outdated Show resolved Hide resolved
...g.viessmann/src/main/java/org/smarthomej/binding/viessmann/internal/dto/ThingMessageDTO.java Outdated Show resolved Hide resolved
@rogrun
[viessmann] removed empty lines in dto classes
e30bc99 
Signed-off-by: Ronny Grun <ronny.grun@t-online.de>
@rogrun
[viessann] requested changes
e054dd6 
Signed-off-by: Ronny Grun <ronny.grun@t-online.de>
@rogrun
[viessmann] removed unused formatter:off/on
0fb7094 
Signed-off-by: Ronny Grun <ronny.grun@t-online.de>
@rogrun
Copy link
Contributor Author

rogrun commented Nov 17, 2021

All requested changes are done

@rogrun rogrun requested a review from J-N-K November 17, 2021 16:48
@J-N-K J-N-K merged commit 4dae89f into smarthomej:3.2.x Nov 17, 2021
@J-N-K J-N-K added this to the 3.1.8 milestone Nov 17, 2021
J-N-K added a commit that referenced this pull request Nov 17, 2021
@J-N-K
[viessmann] Initial contribution of Viessmann Binding (#212)
50044e0 
* [viessmann] Initial contribution
This binding connects openHAB with the new Viessmann API

Signed-off-by: Ronny Grun <ronny.grun@t-online.de>
Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
@rogrun rogrun deleted the 3.2.x-viessmann branch November 18, 2021 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@J-N-K J-N-K J-N-K approved these changes

Assignees
No one assigned
Labels
newbinding
Projects
None yet
Milestone
3.1.8
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@rogrun @J-N-K