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

[knx] Allow sending items with units to KNX bus. #12675

Merged
merged 1 commit into from
May 8, 2022

Conversation

holgerfriedrich
Copy link
Member

Items with dimensions (QuantityType) are now translated and can be sent to the
KNX bus. This requires the correct DPT to be specified in the channel
definition. Fixes #10706.

Signed-off-by: Holger Friedrich mail@holger-friedrich.de

@lolodomo lolodomo added the enhancement An enhancement or new feature for an existing add-on label May 3, 2022
case 23: {
final String unit = qt.getUnit().toString();
if (unit.contains("°C")) {
targetOhUnit = "°C/%";
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this unit ?

Copy link
Member Author

Choose a reason for hiding this comment

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

9.023 DPT_KelvinPerPercent, this is a delta and we need to convert to the matching base unit first (automatic conversion °C to K or °F will fail due to the offset/scaling)

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, my question was: is "°C/%" is a valid unit supported by UoM ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually is seem so. DSL rules let you construct this

        var k = 25|K
        var p = 10|%
        var result = k/p
        var dimensionless = p.toUnit("")

Printing to the log gets you

...script.UoM   ] - this is k: 25 K
...script.UoM   ] - this is p: 10 %
...script.UoM   ] - this is k/p: 2.5 K/%
...script.UoM   ] - this is p converted to dimensionless: 0.1

The values are sent to the KNX bus when I use sendCommand, both tested with items defined as Number and Number:Temperature. It leads to same value on KNX side.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, strange but if you tested it, I believe you.

@lolodomo
Copy link
Contributor

lolodomo commented May 4, 2022

@SJKA : would you like to review ?

@holgerfriedrich
Copy link
Member Author

@lolodomo thanks for reviewing this

@holgerfriedrich
Copy link
Member Author

@lolodomo I have the feeling that our CODEOWNERS file is outdated.... @SJKA joined github 2021....

@sjsf Did you initially submit the code for the knx binding?

@lolodomo
Copy link
Contributor

lolodomo commented May 6, 2022

@lolodomo I have the feeling that our CODEOWNERS file is outdated.... @SJKA joined github 2021....

@sjsf Did you initially submit the code for the knx binding?

I see "Simon Kaufmann" as author of a lot of classes. Is sjka =Simon Kaufmann ? This is probable...

@kaikreuzer : do you know if there still a maintainer for the KNX maintainer ? If not, I will merge this PR without waiting.

@holgerfriedrich
Copy link
Member Author

holgerfriedrich commented May 6, 2022

@lolodomo but to me it seems that GitHub re-issued this user account to someone else... Just have a look at the two user profiles mentioned above

@kaikreuzer
Copy link
Member

@lolodomo Yes, I think Simon is no longer active here - I have thus created #12694 and added me as a code owner as I was the original author of the 1.x binding (the first binding ever written for openHAB ;-)).
If you approve the changes, I can have a short second glance over it, so that we can merge it.

@kaikreuzer kaikreuzer requested a review from lolodomo May 7, 2022 06:36
@lolodomo
Copy link
Contributor

lolodomo commented May 7, 2022

If you approve the changes, I can have a short second glance over it, so that we can merge it.

Yes please

bundles/org.openhab.binding.knx/README.md Outdated Show resolved Hide resolved
key, expectedTypeClass, command, dpt);
return new WriteSpecImpl(config, dpt, command);
}
}
if (command instanceof QuantityType) {
Copy link
Member

Choose a reason for hiding this comment

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

Hm, is it really correct to do an independent "if" here and not check at all the dpt anymore?
I would expect the typeHelper in line 173 to return DecimalType as the expectedTypeClass.
If this is the case, we would probably only need to adapt line 175 to

                    if (expectedTypeClass.isAssignableFrom(command.getClass())) {

so that it also evaluates to true, if command is a QuantityType and not only if it is a DecimalType.

Wdyt? Am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kaikreuzer I just did a quick check: expectedTypeClass returns DecimalType, command.getClass()is QuantityType and isAssignableFrom returns false.

I don't think that we nedd to check the dpt - the type conversion will anyway only work if the unit conversion is possible.

Copy link
Member

Choose a reason for hiding this comment

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

You're right, I forgot that QuantityType does not extend DecimalType. 😞
I would nonetheless prefer to do a proper check of the type in the if clause.
If expectedTypeClass is null, we should never proceed, so the code should imho move into the if statement above.
And then we could simply enhance line 175 to

                    if (expectedTypeClass.isInstance(command) || (expectedTypeClass==DecimalType.class && command instanceof QuantityType<?>)) {

(and slightly change the tracing message).

Copy link
Member Author

Choose a reason for hiding this comment

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

This looks reasonable, updated code as requested.
Thanks Kai!

Items with dimensions (QuantityType) are now translated and can be sent to the
KNX bus. This requires the correct DPT to be specified in the channel
definition. Fixes openhab#10706.

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>
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.

Great, thank you!

@kaikreuzer kaikreuzer merged commit 8830ed5 into openhab:main May 8, 2022
@kaikreuzer kaikreuzer added this to the 3.3 milestone May 8, 2022
@holgerfriedrich holgerfriedrich deleted the pr-knx-units branch May 8, 2022 10:12
@butterfly2501
Copy link

butterfly2501 commented Aug 21, 2022

@holgerfriedrich @kaikreuzer: I debug the component to send items with unit yesterday because my humidity sensor was not working and always got getCommandSpec no Spec found.
It was never working wih the dpt 9.007 with or without unit. So my workaround is now with a seperate field and to change the value from % to °C and use instead dpt 9.001. Can you please check in the code if the unit for humidity % is correct processed.

andan67 pushed a commit to andan67/openhab-addons that referenced this pull request Nov 6, 2022
Items with dimensions (QuantityType) are now translated and can be sent to the
KNX bus. This requires the correct DPT to be specified in the channel
definition. Fixes openhab#10706.

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>
andrasU pushed a commit to andrasU/openhab-addons that referenced this pull request Nov 12, 2022
Items with dimensions (QuantityType) are now translated and can be sent to the
KNX bus. This requires the correct DPT to be specified in the channel
definition. Fixes openhab#10706.

Signed-off-by: Holger Friedrich <mail@holger-friedrich.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
Items with dimensions (QuantityType) are now translated and can be sent to the
KNX bus. This requires the correct DPT to be specified in the channel
definition. Fixes openhab#10706.

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>
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 for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[KNX] Items with Dimension are not written to the bus.
4 participants