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] Correctly support state sub-types for DPTs #16337

Merged
merged 2 commits into from Feb 2, 2024
Merged

Conversation

kaikreuzer
Copy link
Member

This fixes a problem when KNX channels are linked to items that support sub-types of what the DPTs can handle in case of "-control" channels or channels with a follow profile.

More concretely: Having a Color Item linked through the follow profile to a DPT 1.001 results in None of the configured GAs on channel 'xxx' could handle the command, while it would be expected that true is sent for anything that isn't equal to the state OFF.

This PR makes sure that the binding tries to check if the command is a state that can be converted to a suitable subtype for the linked DPT and then sends out the according KNX telegram.

Signed-off-by: Kai Kreuzer <kai@openhab.org>
Signed-off-by: Kai Kreuzer <kai@openhab.org>
@kaikreuzer kaikreuzer added the bug An unexpected problem or unintended behavior of an add-on label Jan 28, 2024
@kaikreuzer kaikreuzer requested a review from a team January 28, 2024 22:01
@kaikreuzer kaikreuzer changed the title Knxcmd [knx] Correctly support state sub-types for DPTs Jan 28, 2024
@holgerfriedrich
Copy link
Member

@kaikreuzer
I tried this with the following settings:
knx.things:

    Thing device hsb_test "test" {
        Type color: in "rgb knx in" [ga="232.600:10/0/0"]
        Type switch: colsw "sc" [ ga="12/0/25" ]
    }

default.items:

Color rgbin {channel="knx:device:ip:hsb_test:in", channel="knx:device:ip:hsb_test:colsw"[profile="follow"] }

Sending value 0,0,0 from the console sends the corresponding packet with status OFF.
Sending values != 0,0,0 results in sending status ON to 12/0/25.
DPT1 part seems to work as intended.

However, no packet to control the color seems to be sent to the bus on GA 10/0/0.
Without your change it does not work as well. Can you reproduce?

@kaikreuzer
Copy link
Member Author

@holgerfriedrich I never tried rgb DPTs, since I don't have any such devices on my KNX bus.
But what you describe seems unrelated to this PR, so I would suggest to open a separate issue for it.

@holgerfriedrich
Copy link
Member

holgerfriedrich commented Feb 1, 2024

I found the mistake in the config above:

Type color: in "rgb knx in" [hsb="232.600:10/0/0"]

Now it works as expected.

@kaikreuzer
Copy link
Member Author

Awesome!

Copy link
Member

@wborn wborn 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 the fix!

@wborn wborn merged commit 7671f4b into openhab:main Feb 2, 2024
3 checks passed
@wborn wborn added this to the 4.2 milestone Feb 2, 2024
@J-N-K
Copy link
Member

J-N-K commented Feb 3, 2024

This breaks dimmer channels, at least "sometimes".

    @Test
    void test5001PercentType() throws KNXFormatException {
        Configuration configuration = new Configuration(Map.of("switch", "1.001:1/2/1", "position", "5.001:1/2/2"));
        Channel channel = Objects.requireNonNull(mock(Channel.class));
        when(channel.getChannelTypeUID()).thenReturn(DIMMER_CHANNEL_TYPE_UID);
        when(channel.getConfiguration()).thenAnswer(i -> configuration);

        KNXChannel knxChannel = KNXChannelFactory.createKnxChannel(channel);
        assertThat(knxChannel, instanceOf(TypeDimmer.class));

        Command command = new PercentType("100");
        OutboundSpec outboundSpec = knxChannel.getCommandSpec(command);
        assertThat(outboundSpec, is(notNullValue()));

        String mappedValue = ValueEncoder.encode(outboundSpec.getValue(), outboundSpec.getDPT());
        assertThat(mappedValue, is("100"));
        assertThat(outboundSpec.getValue(), is(instanceOf(PercentType.class)));
    }

Fails sometimes, sometimes not, usually with

ERROR] org.openhab.binding.knx.internal.channel.KNXChannelTest.test5001PercentType -- Time elapsed: 0.007 s <<< FAILURE!
java.lang.AssertionError: 

Expected: is "100"
     but: was "1"
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
	at org.openhab.binding.knx.internal.channel.KNXChannelTest.test5001PercentType(KNXChannelTest.java:200)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

I tried to find out why this happens and enhanced the logging in the new code and found three different outputs:

[main] ERROR o.o.b.k.internal.channel.KNXChannel - getCommandSpec checking keys '[increaseDecrease, switch, position]' for command '100' (class org.openhab.core.library.types.PercentType)
[main] ERROR o.o.b.k.internal.channel.KNXChannel - getCommandSpec command class class org.openhab.core.library.types.PercentType is a sub-class of the expected type class class org.openhab.core.library.types.DecimalType for key position

[main] ERROR o.o.b.k.internal.channel.KNXChannel - getCommandSpec checking keys '[position, increaseDecrease, switch]' for command '100' (class org.openhab.core.library.types.PercentType)
[main] ERROR o.o.b.k.internal.channel.KNXChannel - getCommandSpec command class class org.openhab.core.library.types.PercentType is a sub-class of the expected type class class org.openhab.core.library.types.QuantityType for key position

[main] ERROR o.o.b.k.internal.channel.KNXChannel - getCommandSpec checking keys '[switch, position, increaseDecrease]' for command '100' (class org.openhab.core.library.types.PercentType)
[main] ERROR o.o.b.k.internal.channel.KNXChannel - getCommandSpec command class class org.openhab.core.library.types.PercentType matches expected type class class org.openhab.core.library.types.PercentType for key position

The output of the gaKeys in the log gave me the idea what is going on: a Set does not preserve order of elements (except LinkedHashSet, but Set.of can return any implementation). Depending on the order of elements in Set.of(QuantityType.class, DecimalType.class, PercentType.class) (which is the value of DPTXlator8BitUnsigned.DPT_SCALING.getID()inDPTUtil.DPT_TYPE_MAP) and returned as expectedTypes`, we either get the correct or wrong value.

This was not important before, because we only checked for class-presence but did not apply any conversion, while we now change the type.

@J-N-K
Copy link
Member

J-N-K commented Feb 3, 2024

@kaikreuzer @holgerfriedrich Shall we revert it for now? It seems worse than before, at least for me.

@J-N-K J-N-K mentioned this pull request Feb 4, 2024
@kaikreuzer kaikreuzer deleted the knxcmd branch February 5, 2024 22:16
austvik pushed a commit to austvik/openhab-addons that referenced this pull request Mar 27, 2024
Signed-off-by: Kai Kreuzer <kai@openhab.org>
Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants