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

[energidataservice] Provide work-around for currency issues #16085

Merged
merged 2 commits into from
Dec 20, 2023
Merged

[energidataservice] Provide work-around for currency issues #16085

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f1b67ea
Provide work-around for currency issues
jlaur Dec 19, 2023
bc3a7d7
Verify unit before trying to instantiate QuantityType
jlaur Dec 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions bundles/org.openhab.binding.energidataservice/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,13 @@ This has the following advantages:
If you want electricity tax included in your total price, please add either `electricity-tax` or `reduced-electricity-tax` to the group - depending on which one applies.
See [Electricity Tax](#electricity-tax) for further information.

#### Currencies

There are some existing limitations related to currency support.
While the binding attempts to update channels in the correct currency, such attempts may face rejection.
In such cases, the binding will resort to omitting the currency unit.
While this ensures correct prices, it's important to note that the currency information may be incorrect in these instances.

#### Value-Added Tax

VAT is not included in any of the prices.
Expand Down
33 changes: 27 additions & 6 deletions ...java/org/openhab/binding/energidataservice/internal/handler/EnergiDataServiceHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;

import javax.measure.Unit;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
import org.eclipse.jetty.client.HttpClient;
Expand All @@ -56,9 +58,10 @@
import org.openhab.binding.energidataservice.internal.retry.RetryPolicyFactory;
import org.openhab.binding.energidataservice.internal.retry.RetryStrategy;
import org.openhab.core.i18n.TimeZoneProvider;
import org.openhab.core.library.dimension.EnergyPrice;
import org.openhab.core.library.types.DecimalType;
import org.openhab.core.library.types.QuantityType;
import org.openhab.core.library.types.StringType;
import org.openhab.core.library.unit.CurrencyUnits;
import org.openhab.core.thing.Channel;
import org.openhab.core.thing.ChannelUID;
import org.openhab.core.thing.Thing;
Expand All @@ -68,6 +71,7 @@
import org.openhab.core.thing.binding.ThingHandlerService;
import org.openhab.core.types.Command;
import org.openhab.core.types.RefreshType;
import org.openhab.core.types.State;
import org.openhab.core.types.TimeSeries;
import org.openhab.core.types.UnDefType;
import org.slf4j.Logger;
Expand Down Expand Up @@ -321,19 +325,36 @@ private void updateCurrentSpotPrice() {
return;
}
BigDecimal spotPrice = cacheManager.getSpotPrice();
updateState(CHANNEL_SPOT_PRICE,
spotPrice != null ? getEnergyPrice(spotPrice, config.getCurrency()) : UnDefType.UNDEF);
updatePriceState(CHANNEL_SPOT_PRICE, spotPrice, config.getCurrency());
}

private void updateCurrentTariff(String channelId, @Nullable BigDecimal tariff) {
if (!isLinked(channelId)) {
return;
}
updateState(channelId, tariff != null ? getEnergyPrice(tariff, CURRENCY_DKK) : UnDefType.UNDEF);
updatePriceState(channelId, tariff, CURRENCY_DKK);
}

private void updatePriceState(String channelID, @Nullable BigDecimal price, Currency currency) {
updateState(channelID, price != null ? getEnergyPrice(price, currency) : UnDefType.UNDEF);
}

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

Choose a reason for hiding this comment

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

You could also fall back to CurrencyUnits.BASE_CURRENCY.getSymbol(), which is always available. Or check if the desired unit is available by CurrencyUnits.getInstance().getUnit(String s).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I'm now verifying the unit before trying to instantiate QuantityType from it.

Additionally I will now fallback to currency code if there is no symbol for the currency, although my tests suggest that currency code as unit is not supported. Also I'm now using getSymbol() on the returned Unit rather than Currency.getSymbol(), because that is display unit for the Locale and thus prone to failure (e.g. "$" vs. "US$").

I don't think using base currency is such a good idea, because that would mean explicitly providing a wrong unit. The end result is currently the same though.

Copy link
Contributor Author

@jlaur jlaur Dec 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I see this only as a work-around to ensure reliable state updates in 4.1, would you agree? And do you see anything in all this that is worth creating an issue for in core?

We currently have the limitation that the UoM system know only one currency. When this will change, I would somehow expect:

  • Always being able to update state for known currencies, even if there is no currency exchange service provided/installed - without risking IllegalArgumentException being thrown. With known currencies, all I mean all defined by ISO 4217.
  • In this case IMHO it would still be better to keep the source currency unit rather than overwriting by user configured currency unit without converting the amount. I know this contradicts 4.0 UoM because we cannot enforce "target unit", and I don't have a good proposal for tackling that. It's different, because we can always expect to be able to convert °C to °F, but we can't always expect to be able to convert DKK to EUR. And we cannot make the assumption that exchange rate is 100, because then prices will be plain misleading.
  • Being able to update state with currency code as unit rather than symbol. When all currencies will become available and we are able to convert between them, symbols can become ambiguous as previously discussed.

}
}

private void updateHourlyPrices() {
Expand Down