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

[awattar] Initial contribution #11976

Merged
merged 21 commits into from
May 2, 2022
Merged

[awattar] Initial contribution #11976

merged 21 commits into from
May 2, 2022

Conversation

Wolfgang1966
Copy link

Around one year ago I started developing a binding to retrieve energy prices for the German/Austrian electricity provider aWATTar. Find the discussion at
https://community.openhab.org/t/awattar-binding-beta-and-discussion/110497

The goal of the binding is to enable aWATTar customers to make use of hours with lower energy prices for equipment that consumes a lot of energy but does not need to run all the time, e.g. wallboxes.

A few people are using the binding already in their openHAB installation. They found a few bugs which were already corrected since then.

Before opening this PR I rebased my branch to the current main branch to avoid merge conflicts.

The latest version of the binding is available for download at
https://home.klimt.de/owncloud/index.php/s/zKAsTWkxjG6RLyS

I did my best to follow the requirements for new bindings. However, as this is my first one, I may have overlooked things, If I did, I appreciate any comments.

@Wolfgang1966 Wolfgang1966 requested a review from a team as a code owner January 6, 2022 16:14
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/awattar-binding-beta-and-discussion/110497/39

@fwolter fwolter added the new binding If someone has started to work on a binding. For a new binding PR. label Jan 6, 2022
@lolodomo
Copy link
Contributor

@Wolfgang1966 : I mark this PR with label "work in progress" in relation to your PR tile. Let us know when your work is finished and ready for a review.

@lolodomo lolodomo added the work in progress A PR that is not yet ready to be merged label Jan 15, 2022
@Wolfgang1966 Wolfgang1966 changed the title [awattar] WIP Initial contribution [awattar] Initial contribution Jan 15, 2022
@Wolfgang1966
Copy link
Author

Hi @lolodomo , thanks for bringing this up. I removed the "WIP" from the title, as I now managed to get rid of all warnings (at least when building the code locally). So I would appreciate a first review to get some feedback. As I wrote, this is the first binding I wrote from scratch and there may be fundamental things I failed to implement the way it should be. I assume the first review brings up those things and will require deeper rework which will take some time. Afterwards, there may be smaller detail work necessary, but I think this only makes sense as soon as the overall structure is finalized and stable.

@lolodomo lolodomo removed the work in progress A PR that is not yet ready to be merged label Jan 15, 2022
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! I reviewed your code and here is my feedback.

You could use mvn license:format in the root directory of this repo to update the copyright year.

There are some checkstyle warnings left. You could take a look at target/code-analysis/report.html after building it with mvn clean install.

Please add your binding to bom\openhab-addons\pom.xml.

bundles/org.openhab.binding.awattar/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.awattar/README.md Outdated Show resolved Hide resolved
@@ -0,0 +1,166 @@
binding.awattar.name = aWATTar Binding
Copy link
Member

Choose a reason for hiding this comment

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

@lolodomo Shall this be part of the binding?

Copy link
Contributor

@lolodomo lolodomo Apr 5, 2022

Choose a reason for hiding this comment

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

No, it is better to translate in Crowdin.
But if the translation was fully done that way, as it is a new binding, an exception could be done : just after the merge, someone should take care to load the German transitions in Crowdin.

Copy link
Author

Choose a reason for hiding this comment

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

I removed the name from the properties file

Copy link
Member

Choose a reason for hiding this comment

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

It was about the entire file. As you already did all translations in the properties file, I suggest you to add the name property again and after this has been merged, you ask a maintainer to load your whole translation into Crowdin.

Copy link
Member

Choose a reason for hiding this comment

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

@lolodomo @jlaur can you load the German translations into Crowdin?

@lolodomo lolodomo added the awaiting feedback Awaiting feedback from the pull request author label Apr 10, 2022
@Wolfgang1966
Copy link
Author

@fwolter @lolodomo Finally found some time to work on your comments. Hope to be able to push an update within the next days. Sorry for the delay.

@Wolfgang1966
Copy link
Author

Hi @fwolter and @lolodomo , I now did the requested changes and also the checkstyle report, with the following exception:

At two places I need the ScheduleAtFixedRate because the actions need to happen at minute boundaries. Otherwise I would have to schedule much more often than once per minute, which would most probably cause much more load than the more efficient scheduler might save.

Is there any way to avoid the warning in such cases?

@Wolfgang1966
Copy link
Author

I just saw that the build fails because of the year in the header line. However, this is the line I got with

mvn license:format

and with "2022" the checkstyle report gives errors. Do I need to rebase the whole PR to a newer openHAB version to get rid of this problem?

@Wolfgang1966 Wolfgang1966 requested a review from fwolter April 17, 2022 16:10
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.

To make the binding compile, you need to rebase your branch and set the version to the current snapshot: 3.3.0-SNAPSHOT.

Here are the commands for rebasing your branch:

If not already done, add the upstream openHAB addon repo as a remote to your local repo and fetch it:

git remote add upstream https://github.com/openhab/openhab-addons.git
git fetch upstream

The remotes (git remote --verbose) should look like this, now:

origin  https://github.com/[your name]/openhab-addons.git (fetch)
origin  https://github.com/[your name]/openhab-addons.git (push)
upstream        https://github.com/openhab/openhab-addons.git (fetch)
upstream        https://github.com/openhab/openhab-addons.git (push)

Then, you can rebase your PR's branch onto main:

git rebase upstream/main

Finally force-push the rebased branch to this PR's branch:

git push origin [your branch name of this PR] --force-with-lease

DON'T use merge! This can clutter the Git history and make the PR unusable.

@fwolter fwolter removed the awaiting feedback Awaiting feedback from the pull request author label Apr 18, 2022
wolfii and others added 14 commits April 24, 2022 16:43
Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>
Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>
Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>
Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>
Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>
Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>
Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>
Wolfgang Klimt added 6 commits April 24, 2022 16:46
Signed-off-by: Wolfgang Klimt <github@klimt.de>
Obeyed most of the review comments. Exceptions:

* binding is already added to bom/openhab-addons/pom.xml (at least I found it there and there was a commit notice in git log)
* mvn license:format brought back 2021, so I manually set everything to 2022. Should I try to rebase my whole branch?
* In two places the binding needs to adjust to minute boundaries, hence scheduleWithFixedDelay will not work.
* I removed empty trailing lines, but mvn spotless:apply brought them back
* The OhInfXmlUsageCheck seems to be wrong.
* The ConstantNameCheck in AwattarUtil seems to be mislead by the fact that all members of the class are static, including the logger. From my point of view it is not a real "constant".

Signed-off-by: Wolfgang Klimt <github@klimt.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>
@Wolfgang1966
Copy link
Author

Branch rebased.

@Wolfgang1966 Wolfgang1966 requested a review from fwolter April 24, 2022 15:04
@@ -0,0 +1,166 @@
binding.awattar.name = aWATTar Binding
Copy link
Member

Choose a reason for hiding this comment

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

It was about the entire file. As you already did all translations in the properties file, I suggest you to add the name property again and after this has been merged, you ask a maintainer to load your whole translation into Crowdin.

thingRefresher = scheduler.scheduleAtFixedRate(this::refreshChannels,
getMillisToNextMinute(1, timeZoneProvider), thingRefreshInterval * 1000, TimeUnit.MILLISECONDS);
}

Copy link
Member

Choose a reason for hiding this comment

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

This empty line needs to be removed to satisfy checkstyle.

Copy link
Author

Choose a reason for hiding this comment

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

Removed it, however, There was no warning when I built the binding locally. I will double check the build result after push.

Signed-off-by: Wolfgang Klimt <github@klimt.de>
@Wolfgang1966
Copy link
Author

Added the name property again, will take care of Crowdin after merge.

@Wolfgang1966 Wolfgang1966 requested a review from fwolter May 1, 2022 13:15
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
Now, you could add your binding's logo to the openHAB website. See https://www.openhab.org/docs/developer/bindings/#add-your-binding-s-logo-to-the-openhab-website

@Wolfgang1966
Copy link
Author

Wolfgang1966 commented May 2, 2022

Hi @fwolter , thanks for your approval.

Yours

Wolfgang

@fwolter fwolter merged commit 61de1a5 into openhab:main May 2, 2022
@fwolter fwolter added this to the 3.3 milestone May 2, 2022
@Wolfgang1966
Copy link
Author

Contacted aWATTar for the Logo

@Wolfgang1966 Wolfgang1966 deleted the awattar branch May 2, 2022 06:59
@lolodomo
Copy link
Contributor

lolodomo commented May 2, 2022

For your information, I uploaded the German translations in Crowdin.

I noticed these warnings when I compile:

[INFO] --- sat-plugin:0.12.0:report (sat-all) @ org.openhab.binding.awattar ---
[INFO] Individual report appended to summary report.
[WARNING] Code Analysis Tool has found:
 0 error(s)!
 2 warning(s)
 0 info(s)
[WARNING] org.openhab.binding.awattar.internal.handler.AwattarBestpriceHandler.java:[106]
For periodically executed jobs that do not require a fixed rate scheduleWithFixedDelay should be preferred over scheduleAtFixedRate.
[WARNING] org.openhab.binding.awattar.internal.handler.AwattarPriceHandler.java:[93]
For periodically executed jobs that do not require a fixed rate scheduleWithFixedDelay should be preferred over scheduleAtFixedRate.

@fwolter
Copy link
Member

fwolter commented May 2, 2022

Thanks! This is one of the rare cases where scheduleAtFixedRate needs to be used.

@lolodomo
Copy link
Contributor

lolodomo commented May 2, 2022

Ok

@Wolfgang1966
Copy link
Author

Is it possible to suppress the warning by some annotation or preceding comment in the code?

@fwolter
Copy link
Member

fwolter commented May 3, 2022

Good idea! You could annotate the method using it with @SuppressWarnings("PMD.AvoidScheduleAtFixedRateCheck").

andan67 pushed a commit to andan67/openhab-addons that referenced this pull request Nov 6, 2022
* First alpha version of the awattar binding

Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Corrected typos

Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>

* More typos

Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Improved time handling to consider time zone.

Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Corrected logical time problem, start adding nextprice thing

Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Added support for Austria

Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Use List instead of Set

Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Minor corrections

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Removed unneeded handler, corrected fetching of prices

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Added i18n, updated documentation

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Corrected pom.xml after rebase

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Updated version to 3.3.0-SNAPSHOT

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Corrected findings of Code analysis tool

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Updated copyright notice

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Updates to get rid of compiler warnings

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Worked on review comments from @fwolter

Obeyed most of the review comments. Exceptions:

* binding is already added to bom/openhab-addons/pom.xml (at least I found it there and there was a commit notice in git log)
* mvn license:format brought back 2021, so I manually set everything to 2022. Should I try to rebase my whole branch?
* In two places the binding needs to adjust to minute boundaries, hence scheduleWithFixedDelay will not work.
* I removed empty trailing lines, but mvn spotless:apply brought them back
* The OhInfXmlUsageCheck seems to be wrong.
* The ConstantNameCheck in AwattarUtil seems to be mislead by the fact that all members of the class are static, including the logger. From my point of view it is not a real "constant".

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Updated Readme to match code changes

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Further work on review comments from @fwolter

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Corrected config definition

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Changed Copyright to 2022

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Review comments from @fwolter. Improved timezone handling

Signed-off-by: Wolfgang Klimt <github@klimt.de>

Co-authored-by: wolfii <wolfgang.klimt@consol.de>
andrasU pushed a commit to andrasU/openhab-addons that referenced this pull request Nov 12, 2022
* First alpha version of the awattar binding

Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Corrected typos

Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>

* More typos

Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Improved time handling to consider time zone.

Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Corrected logical time problem, start adding nextprice thing

Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Added support for Austria

Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Use List instead of Set

Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Minor corrections

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Removed unneeded handler, corrected fetching of prices

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Added i18n, updated documentation

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Corrected pom.xml after rebase

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Updated version to 3.3.0-SNAPSHOT

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Corrected findings of Code analysis tool

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Updated copyright notice

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Updates to get rid of compiler warnings

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Worked on review comments from @fwolter

Obeyed most of the review comments. Exceptions:

* binding is already added to bom/openhab-addons/pom.xml (at least I found it there and there was a commit notice in git log)
* mvn license:format brought back 2021, so I manually set everything to 2022. Should I try to rebase my whole branch?
* In two places the binding needs to adjust to minute boundaries, hence scheduleWithFixedDelay will not work.
* I removed empty trailing lines, but mvn spotless:apply brought them back
* The OhInfXmlUsageCheck seems to be wrong.
* The ConstantNameCheck in AwattarUtil seems to be mislead by the fact that all members of the class are static, including the logger. From my point of view it is not a real "constant".

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Updated Readme to match code changes

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Further work on review comments from @fwolter

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Corrected config definition

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Changed Copyright to 2022

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Review comments from @fwolter. Improved timezone handling

Signed-off-by: Wolfgang Klimt <github@klimt.de>

Co-authored-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: Andras Uhrin <andras.uhrin@gmail.com>
psmedley pushed a commit to psmedley/openhab-addons that referenced this pull request Feb 23, 2023
* First alpha version of the awattar binding

Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Corrected typos

Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>

* More typos

Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Improved time handling to consider time zone.

Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Corrected logical time problem, start adding nextprice thing

Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Added support for Austria

Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Use List instead of Set

Signed-off-by: wolfii <wolfgang.klimt@consol.de>
Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Minor corrections

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Removed unneeded handler, corrected fetching of prices

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Added i18n, updated documentation

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Corrected pom.xml after rebase

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Updated version to 3.3.0-SNAPSHOT

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Corrected findings of Code analysis tool

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Updated copyright notice

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Updates to get rid of compiler warnings

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Worked on review comments from @fwolter

Obeyed most of the review comments. Exceptions:

* binding is already added to bom/openhab-addons/pom.xml (at least I found it there and there was a commit notice in git log)
* mvn license:format brought back 2021, so I manually set everything to 2022. Should I try to rebase my whole branch?
* In two places the binding needs to adjust to minute boundaries, hence scheduleWithFixedDelay will not work.
* I removed empty trailing lines, but mvn spotless:apply brought them back
* The OhInfXmlUsageCheck seems to be wrong.
* The ConstantNameCheck in AwattarUtil seems to be mislead by the fact that all members of the class are static, including the logger. From my point of view it is not a real "constant".

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Updated Readme to match code changes

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Further work on review comments from @fwolter

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Corrected config definition

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Changed Copyright to 2022

Signed-off-by: Wolfgang Klimt <github@klimt.de>

* Review comments from @fwolter. Improved timezone handling

Signed-off-by: Wolfgang Klimt <github@klimt.de>

Co-authored-by: wolfii <wolfgang.klimt@consol.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants