-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[tacmi] Reworked unit-mapping between TA and OH; added support for timespans #17556
base: main
Are you sure you want to change the base?
Conversation
…mespans Signed-off-by: Christian Niessner <github-marvkis@christian-niessner.de>
e5d66e0
to
25c45f6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into this. This is not a full review, just a quick look at what this is about.
bundles/org.openhab.binding.tacmi/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
private final String OHM = "" + (char) 0x2126; // U+2126 is the 'real' OHM sign | ||
private final String KOHM = "k" + (char) 0x2126; // U+2126 is the 'real' OHM sign |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not looked into detail, but i see some special handling for ohm and killo ohm.
According to https://www.openhab.org/docs/concepts/units-of-measurement.html ohm is supported and by default the kilo prefix is also supported. Could you elaborate on why the default UoM handling is insufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your message got me thinking and I think I have the problem: OHM is mistakenly registered with the Greek omega sign (U+03A9) and not with the supposed OHM sign (U+2126). TA reports the correct OHM sign and the lookup fails.
The screenshot shows the Greek omega in the values
from the unit definition:
And this is how it arrives from TA:
I've already looked into the issues of https://github.com/unitsofmeasurement/indriya/ but it seems nobody noticed it yet.
I could do a
// Special rewrite for electrical resistance measurements
// U+2126 is the 'real' OHM sign, but it seems to be registered as Greek Omega
// (U+03A9) in the units
String unitStrRepl = unitStr.replace((char) 0x2126, (char) 0x03A9);
Unit<?> parsedUnit = SimpleUnitFormat.getInstance().parse(unitStrRepl);
instead of defining the own units. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just perform a char replace before feeding the value to UoM so that you do not need any custom indriya override ?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is was I suggested with the snipplet above ;) I'll rework this part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it holds reference to the SimpleUnitFormat
it seemed like a custom something.
…ing/thing-types.xml Co-authored-by: lsiepel <leosiepel@gmail.com> Signed-off-by: Christian Niessner <marvkis@users.noreply.github.com>
if (unitData == null) { | ||
// we try to lookup the unit given by TA. | ||
try { | ||
Unit<?> parsedUnit = SimpleUnitFormat.getInstance().parse(unitStr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @lsiepel - this is where I access SimpleUnitFormat
to find the appropriate unit for the string we received from the TA equipment. If there is a better way to look up units, please give me a pointer ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi. I Tried it with Units.getInstance().getUnit()
but it didn't resolve the units properly...
…esistance (Ohm) Signed-off-by: Christian Niessner <github-marvkis@christian-niessner.de>
…resolve units properly
Hi,
this PR picks up the discussion with @holana in Issue #12445 to find a more generic way to map units between the TA devices and OpenHAB.
I had the idea to use a so called 'fixed value setting' within TA to test all possible unit types and validate the mapping to OH. As this is now a generic way it should also work for additional units added by TA as long as they are supported by the OpenHAB.
OpenHAB core team: I'm using some classes from
tech.units.indriya
for unit lookups. I also had to create an additional unit (kΩ) that is used by TA. Accessing these packages generates a warning. How should we handle this?Thanks & bye,
chris