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

[UoM] Add currency handling #3503

Merged
merged 4 commits into from
Dec 16, 2023
Merged

[UoM] Add currency handling #3503

merged 4 commits into from
Dec 16, 2023

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented Mar 28, 2023

Closes #3408
Related to #3478

This enables currency handling for UoM. You can define prices (e.g. 0.336 €/kWh) and do calculations based on that like any other unit.

Currently you can define a "system currency" (e.g. , defaults zu @ if not set at all) which is then available as any other unit. It is also possible to define a CurrencyProvider which adds additional currencies and their exchange rates (relative to the system currency). Switching between different currency providers is not yet implemented.

@J-N-K J-N-K requested a review from a team as a code owner March 28, 2023 17:39
@J-N-K J-N-K added the enhancement An enhancement or new feature of the Core label Mar 28, 2023
@J-N-K J-N-K marked this pull request as draft March 28, 2023 17:39
@J-N-K J-N-K added the work in progress A PR that is not yet ready to be merged label Mar 28, 2023
@openhab-bot
Copy link
Collaborator

Can't set status; build succeeded.

@J-N-K
Copy link
Member Author

J-N-K commented Mar 29, 2023

Maybe we should make three parameters for the CurrencyUnit? ISO-Code ("DKK"), symbol ("Kr") and name ("Danske Kroner")?

The problem is that especially the symbols are not necessarily unique, so it may be difficult to determine the correct currency if there is something like 15 $.

Edit: I think we can omit the name and use the name field for the three letter ISO code. We can also validate that when instantiating a new unit.

@jlaur
Copy link
Contributor

jlaur commented Mar 29, 2023

The problem is that especially the symbols are not necessarily unique, so it may be difficult to determine the correct currency if there is something like 15 $.

Edit: I think we can omit the name and use the name field for the three letter ISO code. We can also validate that when instantiating a new unit.

Yes, the unique key should definitely be the currency code.

The symbol is probably subject to some kind of localization, since it can be placed before or after the amount, for example "$ 15" in the US, and "15 €" in Europe. And the currency name (if included) should support I18N (e.g. "den danske krone" in Danish or "Danish krone" in English).

On top of that, this could be formatted also as the sub unit (1/100) "øre", e.g. "1 krone og 25 øre", although this would usually be written "1,25 kr.". Just listing everything I can think of now. 🙂 But again, the most important start is to use the ISO currency code.

Copy link
Contributor

@splatch splatch left a comment

Choose a reason for hiding this comment

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

Small note on naming. CurrencyProvider suggest that it is something to declare currencies while it seems to provide also exchange rate. The scale factor might also be a bit unfortunate term, cause it might imply a more static nature than it actually is.

@J-N-K J-N-K force-pushed the feature-currency branch 2 times, most recently from bef2173 to 0036306 Compare May 13, 2023 13:57
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.

Some minor comments (I know it's still a draft).

I plan to refactor openhab/openhab-addons#14376 quickly to use this when finished and merged.


@Test
public void testPriceCalculation() {
QuantityType<?> unitPrice = new QuantityType<>("0.25 €/kWh");
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this only work for "0.25 EUR/kWh", since the symbol is ambiguous and we're using the currency code as key?

Copy link
Member Author

Choose a reason for hiding this comment

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

The CurrencyProvider has to take care that the symbol is unique. For the system provider there is only one currency, so there can't be ambiguity. For (future) extensions, the provider has to make sure that e.g. Kr or $ is only used for one currency and either not set a symbol for other currencies or prefix them (like AUS$).

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, it's quite possible (even likely) I don't fully understand it yet. Let's say you have two currencies:

  • Currency code: NOK, symbol: kr
  • Currency code: SEK, symbol: kr

I then receive an energy price in NOK, e.g. 1 NOK/kWh, and would like that converted (through an exchange rate service) to SEK. How would I then define that quantity, if not like new QuantityType<>("1 NOK/kWh")? I imagined that symbols would only be used for something like state patterns, i.e. only for display purposes.

Copy link
Member Author

Choose a reason for hiding this comment

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

You can always user the currency code. The CurrencyProvider has to make sure that aliases (symbols) are unique, so it can only use kr for one currency.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. Perhaps a test for currency code could be added as well, e.g. "0.25 EUR/kWh" or "1.75 DKK/kWh" (to pick a currency having an ambiguous symbol)?

Copy link
Member Author

Choose a reason for hiding this comment

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

There should be no ambiguous symbol within a given set of currencies. This has to be ensured by the CurrencyProvider. How should I test that this is the case?

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't mean to test that, only to ensure coverage for using a currency code rather than currency symbol. So simply the same testcase but using for example "EUR" instead of "€".

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.

Submitting review to publish comment made after unrelated minor code comments.


@Test
public void testPriceCalculation() {
QuantityType<?> unitPrice = new QuantityType<>("0.25 €/kWh");
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, it's quite possible (even likely) I don't fully understand it yet. Let's say you have two currencies:

  • Currency code: NOK, symbol: kr
  • Currency code: SEK, symbol: kr

I then receive an energy price in NOK, e.g. 1 NOK/kWh, and would like that converted (through an exchange rate service) to SEK. How would I then define that quantity, if not like new QuantityType<>("1 NOK/kWh")? I imagined that symbols would only be used for something like state patterns, i.e. only for display purposes.

@J-N-K
Copy link
Member Author

J-N-K commented May 31, 2023

The CurrencyService is responsible for injecting a set of one or more currencies to the UoM system. It always injects the whole set from a CurrencyProvider, previously selected sets from other CurrencyProviders are removed. It is the responsibility of a CurrencyProvider that all symbols are unique, so a CurrencyProvider that defines DKK, NOK and SEK may assign the kr symbol to only one of these currencies. Using the ISO codes is always possible (the symbols are just aliases for the ISO code). Besides the set of currency it also provides a "system currency" (a "reference" currency) and an exchange rate for each currency relative to that "system currency".

The system provider (which is part of the I18NProviderImpl only provides one currency, that is the one defined by the locale. Currently no other implementations exist.

@J-N-K J-N-K removed the work in progress A PR that is not yet ready to be merged label Aug 29, 2023
@J-N-K J-N-K marked this pull request as ready for review August 29, 2023 17:40
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.

Two minor comments.

@jlaur
Copy link
Contributor

jlaur commented Oct 15, 2023

@openhab/core-maintainers - any chance to have this PR reviewed? 🙂

@kaikreuzer
Copy link
Member

I can have a look at it asap.
@jlaur As you are deep in the topic yourself: Are you fine with the current state here? The "a CurrencyProvider that defines DKK, NOK and SEK may assign the kr symbol to only one of these currencies" doesn't seem to have your approval, if I get your comments right?

@kaikreuzer kaikreuzer requested a review from jlaur October 27, 2023 19:42
@jlaur
Copy link
Contributor

jlaur commented Oct 27, 2023

I can have a look at it asap.

Awesome, thanks! 👍

@jlaur As you are deep in the topic yourself: Are you fine with the current state here?

From reading the code I don't see any issues. But I have to admit that I scratched only the surface and tried to ask a few questions from my somewhat limited understanding of this area (I read only the new code, not the current code). Everything else I'm leaving for core maintainers, I'm just a passer-by with some interest in the topic. 😄

I did not yet get my hands dirty with it, i.e. tried to build the core project, implement currency support in Energi Data Service and/or implement a currency provider. Only then would I probably really understand what is being offered here. 🙂 I am eager to try it out though, and the concept looks promising.

The "a CurrencyProvider that defines DKK, NOK and SEK may assign the kr symbol to only one of these currencies" doesn't seem to have your approval, if I get your comments right?

Correct, but that's only a slight difference of opinion and should not prevent a merge. Since currency symbols like "kr." or "$" are ambiguous I don't see the benefit of using them for anything other than display/formatting purposes. Currency codes like DKK are ISO standard and unambiguous.

@jlaur
Copy link
Contributor

jlaur commented Oct 28, 2023

I did not yet get my hands dirty with it, i.e. tried to build the core project, implement currency support in Energi Data Service and/or implement a currency provider. Only then would I probably really understand what is being offered here. 🙂 I am eager to try it out though, and the concept looks promising.

Until I can try this out, maybe let me run through an example to verify if my understanding is correct.

New channel type definition:

	<channel-type id="spot-price">
		<item-type>Number:EnergyPrice</item-type>
		<label>Spot Price</label>
		<description>Spot price.</description>
		<category>Price</category>
		<state readOnly="true" pattern="%.9f %unit%"></state>
	</channel-type>

I would assume the channel can be updated like this:

updateState(CHANNEL_SPOT_PRICE, new QuantityType<>(spotPrice + " DKK/kWh");

(here I'm not sure if it can be provided as value and unit separately, but that's a minor thing)

For the spotPrice value 1, with a linked item without unit metadata and for base unit configured as "DKK", I would now assume the state pattern description will make sure to display the item state as "1.000000000 kr./kWh".

Is that correct?

If I now change either base unit configuration to "EUR" or item metadata unit to "EUR/kWh", I'm not sure what would then be displayed, since I don't have any CurrencyProvider implementation yet that can convert DKK to EUR. Will it still show DKK/kr.? Or maybe result in an error because a conversion is not possible?

If I implement a currency exchange converter (i.e. a CurrencyProvider) that is able to convert DKK to EUR, I would then assume that the previous channel update example and unit configuration of "EUR/kWh" would then result in a state similar to "0.130000000 €/kWh"?

Last, in a JavaScript rule I would now assume I could do something like this:

var spotPrice = items.SpotPrice.quantityState; // Unit is "EUR/kWh"
var consumption = Quantity("2 kWh");
var totalPrice = consumption.multiply(spotPrice);
console.log("Total price: " + totalPrice);
console.log("Total price in US dollars: " + totalPrice.toUnit("USD"));

and I would see this logged:

Total price: 0.26 €
Total price in US dollars: 0.28 $

@J-N-K - can you confirm or correct my assumptions here? @kaikreuzer - to me this would seem logical and correct, so if confirmed, I'm very happy with the result. If not, let's figure out where I'm wrong and synchronize.

@jlaur
Copy link
Contributor

jlaur commented Oct 28, 2023

The "a CurrencyProvider that defines DKK, NOK and SEK may assign the kr symbol to only one of these currencies" doesn't seem to have your approval, if I get your comments right?

Correct, but that's only a slight difference of opinion and should not prevent a merge. Since currency symbols like "kr." or "$" are ambiguous I don't see the benefit of using them for anything other than display/formatting purposes. Currency codes like DKK are ISO standard and unambiguous.

In relation to this, I'm now wondering how to override/configure display unit. Let's say channel electricity#spot-price here is updated with energy price in DKK/kWh:

Number:EnergyPrice EnergiDataService_SpotPrice "Spotpris [%.2f %unit%]" <price> (EnergiDataService_TotalPrice) { unit="DKK/kWh", channel="energidataservice:service:energidataservice:electricity#spot-price" [profile="transform:VAT"] }

I assume I can trigger a currency conversion for display purposes:

Number:EnergyPrice EnergiDataService_SpotPrice "Spotpris [%.2f EUR/kWh]" <price> (EnergiDataService_TotalPrice) { unit="DKK/kWh", channel="energidataservice:service:energidataservice:electricity#spot-price" [profile="transform:VAT"] }

But what if I wanted to not only trigger a currency conversion, but also currency symbol display?

Number:EnergyPrice EnergiDataService_SpotPrice "Spotpris [%.2f kr./kWh]" <price> (EnergiDataService_TotalPrice) { unit="DKK/kWh", channel="energidataservice:service:energidataservice:electricity#spot-price" [profile="transform:VAT"] }

(note: symbol for DKK is "kr." with dot... for SEK and NOK it would be just "kr" without dot)

This could be ambiguous, so what would actually happen - is it possible at all? I agree that unit symbol is often preferable for display purposes.

@jlaur
Copy link
Contributor

jlaur commented Oct 29, 2023

See also #3597 (comment).

Signed-off-by: Jan N. Klug <[email protected]>
Signed-off-by: Jan N. Klug <[email protected]>
Signed-off-by: Jan N. Klug <[email protected]>
Signed-off-by: Jan N. Klug <[email protected]>
@jlaur
Copy link
Contributor

jlaur commented Oct 31, 2023

Thanks for rebasing, @J-N-K. I have now built org.openhab.core.config.core and org.openhab.core from your fork and replaced the JARs in my installation from latest snapshot.

Configured provider:
image

(I'm wondering if this should be part of regional settings rather than a separate unit settings - do you expect more unit settings since you put it separately?)

Created an item:

label: Currency Item
type: Number:Currency
category: price
groupNames: []
groupType: None
function: null
tags: []

image

And first tests:

openhab> openhab:update CurrencyItem "100 kr."
Update has been sent successfully.
openhab> openhab:update CurrencyItem "200 DKK"
Error: State '200 DKK' is not valid for item 'CurrencyItem'
Valid data types are: ( DecimalType QuantityType UnDefType )
openhab> openhab:update CurrencyItem "300 $"
Error: State '300 $' is not valid for item 'CurrencyItem'
Valid data types are: ( DecimalType QuantityType UnDefType )
openhab> openhab:update CurrencyItem "400 €"
Error: State '400 €' is not valid for item 'CurrencyItem'
Valid data types are: ( DecimalType QuantityType UnDefType )
openhab> openhab:update CurrencyItem "500 kr"
Error: State '500 kr' is not valid for item 'CurrencyItem'
Valid data types are: ( DecimalType QuantityType UnDefType )

Result:
image

Besides not being able to use currency code for the update, I think it behaves as I would expect. I assume the error when trying unit symbols $, € and kr. is because I have configured DKK indirectly through LocaleBasedCurrencyProvider and because I don't have any CurrencyProvider that can convert DKK to any of those? EDIT: After changing to fixed currency SEK, my existing state of 100 kr. (DKK) changed to 100 SEK. Then:

openhab> openhab:update CurrencyItem "200 kr"
Error: State '200 kr' is not valid for item 'CurrencyItem'
Valid data types are: ( DecimalType QuantityType UnDefType )
openhab> openhab:update CurrencyItem "200 SEK"
Update has been sent successfully.

So when using the FixedCurrencyProvider, currency code can only be used as unit when updating state, and is also shown in the UI, whereas when using LocaleBasedCurrencyProvider, currency symbol is used for both. This seems inconsistent, is this expected?

Also, is it expected that the state value is kept, but the unit is changed, when another currency is configured? Unit metadata doesn't seem to have much impact:

image

When eventually adding a new CurrencyProvider implementing currency exchange, how do I select in the unit settings that my global/system currency is DKK and at the same time select which currency provider to use? Or do you need to define the currency on each item in this case?

@kaikreuzer
Copy link
Member

Since currency symbols like "kr." or "$" are ambiguous I don't see the benefit of using them for anything other than display/formatting purposes.

This is correct, but for display/formatting purposes you would want SEK and NOK both use "kr" as a symbol - and if only one of both is allowed to use it, that would be a problem, wouldn't it?

@jlaur
Copy link
Contributor

jlaur commented Nov 7, 2023

Since currency symbols like "kr." or "$" are ambiguous I don't see the benefit of using them for anything other than display/formatting purposes.

This is correct, but for display/formatting purposes you would want SEK and NOK both use "kr" as a symbol - and if only one of both is allowed to use it, that would be a problem, wouldn't it?

Indeed. Perhaps my misconception is that I somehow assumed that display/formatting could have an asymmetric relation to "real" unit, so that both SEK/NOK would be displayed as "kr" but still maintain their currency code for all other matters. But of course this wouldn't work the other way, so for example a sitemap Input would have to accept unit "kr" as otherwise the user would have to correct the content from e.g. "1 kr" to "1 SEK" in order to provide the quantity in the opposite direction.

After running the tests in my previous post I'm still a bit confused about the roles of FixedCurrencyProvider vs. LocaleBasedCurrencyProvider in relation to currency code vs. currency symbol.

If it's possible for a binding to publish SEK, and for a user to use a currency exchange service (through a CurrencyProvider I assume) to convert to NOK (after all, Norway and Sweden are neighbors 🙂), then it should be fine. If that would not work, I think we have a problem to solve.

@J-N-K
Copy link
Member Author

J-N-K commented Dec 10, 2023

If a CurrencyProvider provides SEK and NOK and their exchange rate, it's perfectly possible to convert between them, and it is done in the same way like meter to foot.

Both, LocaleBasedCurrencyProvider and FixedCurrencyProvider provide exactly one currency. So they essentially allow using that unit in price calculations. The difference is that the LocaleBasedCurrencyProvider uses the configured locale to determine the currency, while the FixedCurrencyProider can be configured to any currency the user thinks is a good choice. Neither of them allow conversion of currencies, as they only know about one currency.

What is the state here? Any change request or can this be merged?

@jlaur
Copy link
Contributor

jlaur commented Dec 10, 2023

What is the state here? Any change request or can this be merged?

From my perspective, here's a shortened summary of concerns from my testing session:

  • When using the FixedCurrencyProvider, currency code can only be used as unit when updating state, and is also shown in the UI, whereas when using LocaleBasedCurrencyProvider, currency symbol is used for both. This seems inconsistent, is this expected?
  • Is it expected that the state value is kept, but the unit is changed, when another currency is configured? Unit metadata doesn't seem to have much impact. See details above.
  • UI: I'm wondering if this should be part of regional settings rather than a separate unit settings - do you expect more unit settings since you put it separately? It it should have its own unit section, I think the existing unit settings from regional settings should be moved there.

@J-N-K
Copy link
Member Author

J-N-K commented Dec 10, 2023

  • The FixedCurrencyProvider does provide exactly the currency that is configured. You can also define something like JLC for "Jacob Laursen's currency". Maybe we could add an option to add the symbol. The LocaleBasedCurrencyProvider selects a "real" (i.e. existing) currency and (if available) a symbol for that.
  • If you switch the currency, you essentially say that "what was called x before is now y". The unit stays the same, it just gets a new name (and symbol if available). I believe that this happens only on rare occasions, I cannot think of a use-case of a frequently switched currency.
  • Currently the metadata has no effect, because with the two providers we have, the whole UoM system knows only exactly one unit (=currency), the one provided by the selected CurrencyProvider. The CurrencyProvider provides a "system unit" and a set of additional units and their conversion factors to the system unit. If we had the same for length, a LengthProvider could set a system unit of m and also provider ft and Å, because it knows how to convert between those. If you set a value of 5 parsec it would not work, because neither the unit parsec nor the conversion factor to m is known.
  • IMO the whole settings page needs to be reworked for localized settings. Why do we have "Ephemeris" as a section and need to set "Country" again, if we have the same setting under "Regional Settings". IMO it would be much better to move the UoM settings to a new section. I can also imagine that we have configurable "default" units for other dimensions (like length).

@jlaur
Copy link
Contributor

jlaur commented Dec 10, 2023

Thank you for your detailed answer, @J-N-K. That removes my last doubts. Regarding the settings page, this can always be reworked later without breaking anything. I just wondered why a new page was introduced when we can already configure the measurement system in regional settings.

If by any chance this PR would be ready for a merge soon, I should be able to implement currency support in Energy Data Service binding for 4.1. Currency exchange probably not. 😄

@jlaur
Copy link
Contributor

jlaur commented Dec 15, 2023

@kaikreuzer - gentle ping. 🙂

Copy link
Member

@kaikreuzer kaikreuzer 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 @J-N-K.
@jlaur Sorry for the delay, I didn't follow your discussion here and thus didn't notice that you approved the PR. You still have one day left for adapting your binding. 😄

@kaikreuzer kaikreuzer merged commit c8a6cf2 into openhab:main Dec 16, 2023
3 checks passed
@kaikreuzer kaikreuzer added this to the 4.1 milestone Dec 16, 2023
@jlaur
Copy link
Contributor

jlaur commented Dec 16, 2023

Sorry for the delay, I didn't follow your discussion here and thus didn't notice that you approved the PR. You still have one day left for adapting your binding. 😄

Thank you. 😄 I'll give it a go, I have a few hours left. Any chance of a full build now so that a PR in addons will build successfully?

@J-N-K J-N-K deleted the feature-currency branch December 16, 2023 11:17
@J-N-K
Copy link
Member Author

J-N-K commented Dec 16, 2023

I have triggered a core build, once it's done I'll trigger an add-ons build and a distro-build.

@jlaur
Copy link
Contributor

jlaur commented Dec 16, 2023

I have triggered a core build, once it's done I'll trigger an add-ons build and a distro-build.

Thanks! Meanwhile I started looking into the implementation. Is it correct that a state update for kr./kWh can/should now be done like this?

BigDecimal price = new BigDecimal("1");
updateState("price", new QuantityType<EnergyPrice>(price + " DKK/kWh"));

@kaikreuzer
Copy link
Member

I had already triggered a core build and all is available with https://ci.openhab.org/job/openHAB-Core/1310/.

@kaikreuzer
Copy link
Member

With the addons build I am waiting for openhab/openhab-addons#16060 to succeed first.

@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/using-the-new-currency-units-of-measurement-in-4-1/152338/22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature of the Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Currency type UoM
5 participants