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

[amberelectric] Initial contribution #16850

Merged
merged 31 commits into from
Jun 15, 2024
Merged

Conversation

psmedley
Copy link
Contributor

@psmedley psmedley commented Jun 6, 2024

No description provided.

Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
@psmedley psmedley added the new binding If someone has started to work on a binding. For a new binding PR. label Jun 6, 2024
@psmedley psmedley requested a review from a team as a code owner June 6, 2024 09:57
@psmedley
Copy link
Contributor Author

psmedley commented Jun 6, 2024

I've done my best to include as many relevant fixes from #16667 as possible, as all my bindings run off a similar codebase.

Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! I have provided some quick feedback from a first round. In general the code looks good and to the point.

@jlaur jlaur changed the title [amber-electric] Initial Contribution [amberelectric] Initial contribution Jun 6, 2024
Signed-off-by: Paul Smedley <paul@smedley.id.au>
@psmedley psmedley requested a review from jlaur June 6, 2024 23:41
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Copy link
Contributor

@jlaur jlaur 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 quickly addressing review comments. 👍 I have provided a few new comments. I don't see any major issues in general in the binding, so I'll see if I can complete my review tomorrow.

psmedley and others added 8 commits June 8, 2024 07:37
…H-INF/thing/thing-types.xml

Co-authored-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Co-authored-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Co-authored-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
…enhab/binding/amberelectric/internal/AmberElectricWebTargets.java

Co-authored-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
@psmedley
Copy link
Contributor Author

psmedley commented Jun 9, 2024

Interesting.... the binding is running fine on my laptop, copying it over to my production machine I'm getting "Invalid Quantity value: 0.2650741 AUD/kWh"

If I set the default currency in OH to AUD, it's fine.

@jlaur
Copy link
Contributor

jlaur commented Jun 9, 2024

Interesting.... the binding is running fine on my laptop, copying it over to my production machine I'm getting "Invalid Quantity value: 0.2650741 AUD/kWh"

If I set the default currency in OH to AUD, it's fine.

Which version of openHAB is your production system running?

@psmedley
Copy link
Contributor Author

psmedley commented Jun 9, 2024

4.1.3

Signed-off-by: Paul Smedley <paul@smedley.id.au>
@jlaur
Copy link
Contributor

jlaur commented Jun 10, 2024

4.1.3

That should be fine since openhab/openhab-core#4016 was backported.

If I set the default currency in OH to AUD, it's fine.

And what is your currency provider settings when you are getting the error?

Did you have a look at #16085 and #16217, I don't know if this would provide any hints?

@psmedley
Copy link
Contributor Author

4.1.3

That should be fine since openhab/openhab-core#4016 was backported.

If I set the default currency in OH to AUD, it's fine.

And what is your currency provider settings when you are getting the error?

Did you have a look at #16085 and #16217, I don't know if this would provide any hints?

The currency provider setting was not set when I was getting the error.

Signed-off-by: Paul Smedley <paul@smedley.id.au>
@jlaur
Copy link
Contributor

jlaur commented Jun 12, 2024

Did you have a look at #16085 and #16217, I don't know if this would provide any hints?

The currency provider setting was not set when I was getting the error.

As already posted, have a look here:

private State getEnergyPrice(BigDecimal price, Currency currency) {
String currencyCode = currency.getCurrencyCode();
Unit<?> unit = CurrencyUnits.getInstance().getUnit(currencyCode);
if (unit == null) {
logger.trace("Currency {} is unknown, falling back to DecimalType", currency.getCurrencyCode());
return new DecimalType(price);
}
try {
return new QuantityType<>(price + " " + currencyCode + "/kWh");
} catch (IllegalArgumentException e) {
logger.debug("Unable to create QuantityType, falling back to DecimalType", e);
return new DecimalType(price);
}
}

The issue is that AUD is not known as a valid unit when no currency provider is set. It's not ideal, but currently has to be handled/worked around.

Signed-off-by: Paul Smedley <paul@smedley.id.au>
@psmedley
Copy link
Contributor Author

Did you have a look at #16085 and #16217, I don't know if this would provide any hints?

The currency provider setting was not set when I was getting the error.

As already posted, have a look here:

private State getEnergyPrice(BigDecimal price, Currency currency) {
String currencyCode = currency.getCurrencyCode();
Unit<?> unit = CurrencyUnits.getInstance().getUnit(currencyCode);
if (unit == null) {
logger.trace("Currency {} is unknown, falling back to DecimalType", currency.getCurrencyCode());
return new DecimalType(price);
}
try {
return new QuantityType<>(price + " " + currencyCode + "/kWh");
} catch (IllegalArgumentException e) {
logger.debug("Unable to create QuantityType, falling back to DecimalType", e);
return new DecimalType(price);
}
}

The issue is that AUD is not known as a valid unit when no currency provider is set. It's not ideal, but currently has to be handled/worked around.

Should be fixed in latest commit :)

Signed-off-by: Paul Smedley <paul@smedley.id.au>
@psmedley psmedley requested a review from jlaur June 14, 2024 21:14
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Now, you could add your binding's logo to the openHAB website. See https://next.openhab.org/docs/developer/addons/#add-your-add-on-s-logo-to-the-openhab-website-and-the-ui

@jlaur
Copy link
Contributor

jlaur commented Jun 14, 2024

@psmedley - sorry, two small things before merging:

  • Can you please add yourself as binding maintainer?
  • There is one checkstyle warning to fix. You could take a look at target/code-analysis/report.html.

Signed-off-by: Paul Smedley <paul@smedley.id.au>
@psmedley
Copy link
Contributor Author

Just wanted to say thanks for @jlaur for his prompt reviews and patience with me in getting this to the point of being merged!

@psmedley
Copy link
Contributor Author

LGTM, thanks!

Now, you could add your binding's logo to the openHAB website. See https://next.openhab.org/docs/developer/addons/#add-your-add-on-s-logo-to-the-openhab-website-and-the-ui

https://github.com/psmedley/openhab-docs/tree/amberelectric is ready to create the PR once merge occurs.

@jlaur jlaur merged commit 30fbc99 into openhab:main Jun 15, 2024
5 checks passed
@jlaur jlaur added this to the 4.2 milestone Jun 15, 2024
@psmedley psmedley deleted the amberelectric branch June 15, 2024 10:14
psmedley added a commit to psmedley/openhab-addons that referenced this pull request Jun 29, 2024
Signed-off-by: Paul Smedley <paul@smedley.id.au>
pgfeller pushed a commit to pgfeller/openhab-addons that referenced this pull request Sep 29, 2024
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Patrik Gfeller <patrik.gfeller@proton.me>
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.

3 participants