From af4fce1e4f7faf9e5d89ded3ca31a20aac57ecf7 Mon Sep 17 00:00:00 2001 From: J-N-K Date: Wed, 13 Sep 2023 22:27:12 +0200 Subject: [PATCH] Fix CommunicationManager command handling (#3714) * Fix CommunicationManager Signed-off-by: Jan N. Klug --- .../thing/internal/CommunicationManager.java | 56 ++++++++++++------- .../profiles/ProfileCallbackImpl.java | 33 +++++++++-- .../CommunicationManagerOSGiTest.java | 39 ++----------- 3 files changed, 67 insertions(+), 61 deletions(-) diff --git a/bundles/org.openhab.core.thing/src/main/java/org/openhab/core/thing/internal/CommunicationManager.java b/bundles/org.openhab.core.thing/src/main/java/org/openhab/core/thing/internal/CommunicationManager.java index 1823f28a0f1..ab6d5f9dc15 100644 --- a/bundles/org.openhab.core.thing/src/main/java/org/openhab/core/thing/internal/CommunicationManager.java +++ b/bundles/org.openhab.core.thing/src/main/java/org/openhab/core/thing/internal/CommunicationManager.java @@ -220,7 +220,7 @@ private Profile getProfile(ItemChannelLink link, Item item, @Nullable Thing thin private ProfileCallback createCallback(ItemChannelLink link) { return new ProfileCallbackImpl(eventPublisher, safeCaller, itemStateConverter, link, thingRegistry::get, - this::getItem); + this::getItem, this::toAcceptedCommand); } private @Nullable ProfileTypeUID determineProfileTypeUID(ItemChannelLink link, Item item, @Nullable Thing thing) { @@ -281,9 +281,13 @@ private ProfileCallback createCallback(ItemChannelLink link) { if (item != null && thing != null) { Channel channel = thing.getChannel(link.getLinkedUID()); if (channel != null) { + String acceptedItemType = Objects.requireNonNullElse(channel.getAcceptedItemType(), ""); + if (acceptedItemType.startsWith("Number")) { + acceptedItemType = "Number"; + } context = new ProfileContextImpl(link.getConfiguration(), item.getAcceptedDataTypes(), - item.getAcceptedCommandTypes(), acceptedCommandTypeMap.getOrDefault( - Objects.requireNonNullElse(channel.getAcceptedItemType(), ""), List.of())); + item.getAcceptedCommandTypes(), + acceptedCommandTypeMap.getOrDefault(acceptedItemType, List.of())); } } @@ -394,17 +398,12 @@ private void handleEvent(String itemName, T type, @Nullable Str if (thing != null) { Channel channel = thing.getChannel(channelUID.getId()); if (channel != null) { - @Nullable - T convertedType = toAcceptedType(type, channel, acceptedTypesFunction, item); - if (convertedType != null) { - if (thing.getHandler() != null) { - Profile profile = getProfile(link, item, thing); - action.applyProfile(profile, thing, convertedType); - } - } else { - logger.debug( - "Received event '{}' ({}) could not be converted to any type accepted by item '{}' ({})", - type, type.getClass().getSimpleName(), itemName, item.getType()); + if (thing.getHandler() != null) { + // fix QuantityType/DecimalType, leave others as-is + @Nullable + T uomType = fixUoM(type, channel, item); + Profile profile = getProfile(link, item, thing); + action.applyProfile(profile, thing, uomType != null ? uomType : type); } } else { logger.debug("Received event '{}' for non-existing channel '{}', not forwarding it to the handler", @@ -418,8 +417,7 @@ private void handleEvent(String itemName, T type, @Nullable Str } @SuppressWarnings("unchecked") - private @Nullable T toAcceptedType(T originalType, Channel channel, - Function<@Nullable String, @Nullable List>> acceptedTypesFunction, Item item) { + private @Nullable T fixUoM(@Nullable T originalType, Channel channel, Item item) { String channelAcceptedItemType = channel.getAcceptedItemType(); if (channelAcceptedItemType == null) { @@ -442,22 +440,40 @@ private void handleEvent(String itemName, T type, @Nullable Str Unit unit = Objects.requireNonNull(((NumberItem) item).getUnit()); return (T) new QuantityType<>(decimalType.toBigDecimal(), unit); } + return null; + } + + public @Nullable Command toAcceptedCommand(Command originalType, @Nullable Channel channel, @Nullable Item item) { + if (item == null || channel == null) { + logger.warn("Trying to convert types for non-existing channel or item, discarding command."); + return null; + } + String channelAcceptedItemType = channel.getAcceptedItemType(); + + if (channelAcceptedItemType == null) { + return originalType; + } + + Command uomCommand = fixUoM(originalType, channel, item); + if (uomCommand != null) { + return uomCommand; + } // handle HSBType/PercentType if (CoreItemFactory.DIMMER.equals(channelAcceptedItemType) && originalType instanceof HSBType hsb) { - return (T) (hsb.as(PercentType.class)); + return hsb.as(PercentType.class); } // check for other cases if the type is acceptable - List> acceptedTypes = acceptedTypesFunction.apply(channelAcceptedItemType); + List> acceptedTypes = acceptedCommandTypeMap.get(channelAcceptedItemType); if (acceptedTypes == null || acceptedTypes.contains(originalType.getClass())) { return originalType; } else if (acceptedTypes.contains(PercentType.class) && originalType instanceof State state && PercentType.class.isAssignableFrom(originalType.getClass())) { - return (@Nullable T) state.as(PercentType.class); + return state.as(PercentType.class); } else if (acceptedTypes.contains(OnOffType.class) && originalType instanceof State state && PercentType.class.isAssignableFrom(originalType.getClass())) { - return (@Nullable T) state.as(OnOffType.class); + return state.as(OnOffType.class); } else { logger.debug("Received not accepted type '{}' for channel '{}'", originalType.getClass().getSimpleName(), channel.getUID()); diff --git a/bundles/org.openhab.core.thing/src/main/java/org/openhab/core/thing/internal/profiles/ProfileCallbackImpl.java b/bundles/org.openhab.core.thing/src/main/java/org/openhab/core/thing/internal/profiles/ProfileCallbackImpl.java index 463a4c653af..a0f40e53059 100644 --- a/bundles/org.openhab.core.thing/src/main/java/org/openhab/core/thing/internal/profiles/ProfileCallbackImpl.java +++ b/bundles/org.openhab.core.thing/src/main/java/org/openhab/core/thing/internal/profiles/ProfileCallbackImpl.java @@ -23,6 +23,7 @@ import org.openhab.core.items.events.ItemEventFactory; import org.openhab.core.library.items.StringItem; import org.openhab.core.library.types.StringType; +import org.openhab.core.thing.Channel; import org.openhab.core.thing.Thing; import org.openhab.core.thing.ThingUID; import org.openhab.core.thing.binding.ThingHandler; @@ -52,16 +53,19 @@ public class ProfileCallbackImpl implements ProfileCallback { private final Function itemProvider; private final SafeCaller safeCaller; private final ItemStateConverter itemStateConverter; + private final AcceptedTypeConverter acceptedTypeConverter; public ProfileCallbackImpl(EventPublisher eventPublisher, SafeCaller safeCaller, ItemStateConverter itemStateConverter, ItemChannelLink link, - Function thingProvider, Function itemProvider) { + Function thingProvider, Function itemProvider, + AcceptedTypeConverter acceptedTypeConverter) { this.eventPublisher = eventPublisher; this.safeCaller = safeCaller; this.itemStateConverter = itemStateConverter; this.link = link; this.thingProvider = thingProvider; this.itemProvider = itemProvider; + this.acceptedTypeConverter = acceptedTypeConverter; } @Override @@ -78,11 +82,22 @@ public void handleCommand(Command command) { if (ThingHandlerHelper.isHandlerInitialized(thing)) { logger.debug("Delegating command '{}' for item '{}' to handler for channel '{}'", command, link.getItemName(), link.getLinkedUID()); - safeCaller.create(handler, ThingHandler.class) - .withTimeout(CommunicationManager.THINGHANDLER_EVENT_TIMEOUT).onTimeout(() -> { - logger.warn("Handler for thing '{}' takes more than {}ms for handling a command", - handler.getThing().getUID(), CommunicationManager.THINGHANDLER_EVENT_TIMEOUT); - }).build().handleCommand(link.getLinkedUID(), command); + Channel channel = thing.getChannel(link.getLinkedUID()); + Command convertedCommand = acceptedTypeConverter.toAcceptedCommand(command, channel, + itemProvider.apply(link.getItemName())); + if (convertedCommand != null) { + safeCaller.create(handler, ThingHandler.class) + .withTimeout(CommunicationManager.THINGHANDLER_EVENT_TIMEOUT).onTimeout(() -> { + logger.warn("Handler for thing '{}' takes more than {}ms for handling a command", + handler.getThing().getUID(), + CommunicationManager.THINGHANDLER_EVENT_TIMEOUT); + }).build().handleCommand(link.getLinkedUID(), command); + } else { + logger.debug( + "Not delegating command '{}' for item '{}' to handler for channel '{}', " + + "because it was not possible to convcert it to an accepted type).", + command, link.getItemName(), link.getLinkedUID()); + } } else { logger.debug("Not delegating command '{}' for item '{}' to handler for channel '{}', " + "because handler is not initialized (thing must be in status UNKNOWN, ONLINE or OFFLINE but was {}).", @@ -129,4 +144,10 @@ public void sendUpdate(State state) { eventPublisher.post( ItemEventFactory.createStateEvent(link.getItemName(), acceptedState, link.getLinkedUID().toString())); } + + @FunctionalInterface + public interface AcceptedTypeConverter { + @Nullable + Command toAcceptedCommand(Command originalType, @Nullable Channel channel, @Nullable Item item); + } } diff --git a/itests/org.openhab.core.thing.tests/src/main/java/org/openhab/core/thing/internal/CommunicationManagerOSGiTest.java b/itests/org.openhab.core.thing.tests/src/main/java/org/openhab/core/thing/internal/CommunicationManagerOSGiTest.java index 8219b754b36..cd351a7c27c 100644 --- a/itests/org.openhab.core.thing.tests/src/main/java/org/openhab/core/thing/internal/CommunicationManagerOSGiTest.java +++ b/itests/org.openhab.core.thing.tests/src/main/java/org/openhab/core/thing/internal/CommunicationManagerOSGiTest.java @@ -27,7 +27,6 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; import org.mockito.junit.jupiter.MockitoSettings; @@ -81,7 +80,6 @@ import org.openhab.core.thing.type.ChannelType; import org.openhab.core.thing.type.ChannelTypeUID; import org.openhab.core.types.Command; -import org.openhab.core.types.State; /** * @@ -553,44 +551,15 @@ public void testProfileIsNotReusedOnThingChange() { } @Test - public void testItemCommandEventTypeDowncast() { + public void testItemCommandTypeDowncast() { Thing thing = ThingBuilder .create(THING_TYPE_UID, THING_UID).withChannels(ChannelBuilder .create(STATE_CHANNEL_UID_2, CoreItemFactory.DIMMER).withKind(ChannelKind.STATE).build()) .build(); thing.setHandler(thingHandlerMock); when(thingRegistryMock.get(eq(THING_UID))).thenReturn(thing); - - manager.receive(ItemEventFactory.createCommandEvent(ITEM_NAME_2, HSBType.fromRGB(128, 128, 128))); - waitForAssert(() -> { - ArgumentCaptor commandCaptor = ArgumentCaptor.forClass(Command.class); - verify(stateProfileMock).onCommandFromItem(commandCaptor.capture()); - Command command = commandCaptor.getValue(); - assertNotNull(command); - assertEquals(PercentType.class, command.getClass()); - }); - verifyNoMoreInteractions(stateProfileMock); - verifyNoMoreInteractions(triggerProfileMock); - } - - @Test - public void testItemStateEventTypeDowncast() { - Thing thing = ThingBuilder - .create(THING_TYPE_UID, THING_UID).withChannels(ChannelBuilder - .create(STATE_CHANNEL_UID_2, CoreItemFactory.DIMMER).withKind(ChannelKind.STATE).build()) - .build(); - thing.setHandler(thingHandlerMock); - when(thingRegistryMock.get(eq(THING_UID))).thenReturn(thing); - - manager.receive(ItemEventFactory.createStateUpdatedEvent(ITEM_NAME_2, HSBType.fromRGB(128, 128, 128))); - waitForAssert(() -> { - ArgumentCaptor stateCaptor = ArgumentCaptor.forClass(State.class); - verify(stateProfileMock).onStateUpdateFromItem(stateCaptor.capture()); - State state = stateCaptor.getValue(); - assertNotNull(state); - assertEquals(PercentType.class, state.getClass()); - }); - verifyNoMoreInteractions(stateProfileMock); - verifyNoMoreInteractions(triggerProfileMock); + Command command = manager.toAcceptedCommand(HSBType.fromRGB(128, 128, 128), + thing.getChannel(STATE_CHANNEL_UID_2), ITEM_2); + assertEquals(PercentType.class, command.getClass()); } }