Skip to content

Commit bd588df

Browse files
lsiepellsiepel
authored
[zwavejs] Improve metadata handling (#21105)
* normalize thing properties * Improve inclusion/exclusion handling * Remove mix of unrelated metadata * Add test for handling incoming event with factor * Fix race condition Signed-off-by: Leo Siepel <leosiepel@gmail.com>
1 parent 2676d1b commit bd588df

10 files changed

Lines changed: 217 additions & 32 deletions

File tree

‎bundles/org.openhab.binding.zwavejs/src/main/java/org/openhab/binding/zwavejs/internal/action/ZwaveJSActions.java‎

Lines changed: 65 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
package org.openhab.binding.zwavejs.internal.action;
1414

1515
import java.util.concurrent.ScheduledExecutorService;
16+
import java.util.concurrent.ScheduledFuture;
1617
import java.util.concurrent.TimeUnit;
1718

1819
import org.eclipse.jdt.annotation.NonNullByDefault;
@@ -42,20 +43,30 @@
4243
@NonNullByDefault
4344
public class ZwaveJSActions implements ThingActions {
4445
private final Logger logger = LoggerFactory.getLogger(ZwaveJSActions.class);
45-
private static final ScheduledExecutorService SCHEDULER = ThreadPoolManager
46-
.getScheduledPool(BindingConstants.BINDING_ID);
46+
private final ScheduledExecutorService scheduler;
4747
private static final int INCLUSION_EXCLUSION_TIMEOUT_SECONDS = 30;
4848
private @Nullable ZwaveJSBridgeHandler handler;
49+
private @Nullable ScheduledFuture<?> actionStopJob;
50+
private @Nullable ActiveAction activeAction;
51+
52+
public ZwaveJSActions() {
53+
this(ThreadPoolManager.getScheduledPool(BindingConstants.BINDING_ID));
54+
}
55+
56+
ZwaveJSActions(ScheduledExecutorService scheduler) {
57+
this.scheduler = scheduler;
58+
}
4959

5060
@RuleAction(label = "@text/action.start-inclusion.label", description = "@text/action.start-inclusion.description")
51-
public void startInclusion() {
61+
public synchronized void startInclusion() {
5262
ZwaveJSBridgeHandler localHandler = handler;
5363
if (localHandler != null) {
5464
logger.debug("Inclusion action issued");
65+
stopActiveAction(localHandler);
5566
localHandler.startInclusion();
56-
SCHEDULER.schedule(() -> {
57-
localHandler.stopInclusion();
58-
}, INCLUSION_EXCLUSION_TIMEOUT_SECONDS, TimeUnit.SECONDS);
67+
ActiveAction action = this.activeAction = new ActiveAction(ActionType.INCLUSION);
68+
actionStopJob = scheduler.schedule(() -> stopActiveAction(localHandler, action),
69+
INCLUSION_EXCLUSION_TIMEOUT_SECONDS, TimeUnit.SECONDS);
5970
}
6071
}
6172

@@ -64,14 +75,15 @@ public static void startInclusion(ThingActions actions) {
6475
}
6576

6677
@RuleAction(label = "@text/action.start-exclusion.label", description = "@text/action.start-exclusion.description")
67-
public void startExclusion() {
78+
public synchronized void startExclusion() {
6879
ZwaveJSBridgeHandler localHandler = handler;
6980
if (localHandler != null) {
7081
logger.debug("Exclusion action issued");
82+
stopActiveAction(localHandler);
7183
localHandler.startExclusion();
72-
SCHEDULER.schedule(() -> {
73-
localHandler.stopExclusion();
74-
}, INCLUSION_EXCLUSION_TIMEOUT_SECONDS, java.util.concurrent.TimeUnit.SECONDS);
84+
ActiveAction action = this.activeAction = new ActiveAction(ActionType.EXCLUSION);
85+
actionStopJob = scheduler.schedule(() -> stopActiveAction(localHandler, action),
86+
INCLUSION_EXCLUSION_TIMEOUT_SECONDS, TimeUnit.SECONDS);
7587
}
7688
}
7789

@@ -99,14 +111,56 @@ public static void sendMulticastCommand(ThingActions actions, String nodeIDs, In
99111
}
100112

101113
@Override
102-
public void setThingHandler(@Nullable ThingHandler handler) {
114+
public synchronized void setThingHandler(@Nullable ThingHandler handler) {
115+
cancelActionStopJob();
116+
activeAction = null;
103117
if (handler instanceof ZwaveJSBridgeHandler bridgeHandler) {
104118
this.handler = bridgeHandler;
119+
} else {
120+
this.handler = null;
105121
}
106122
}
107123

108124
@Override
109125
public @Nullable ThingHandler getThingHandler() {
110126
return handler;
111127
}
128+
129+
private synchronized void stopActiveAction(ZwaveJSBridgeHandler expectedHandler, ActiveAction expectedAction) {
130+
if (expectedHandler.equals(handler) && expectedAction.equals(activeAction)) {
131+
stopActiveAction(expectedHandler);
132+
}
133+
}
134+
135+
private void stopActiveAction(ZwaveJSBridgeHandler handler) {
136+
cancelActionStopJob();
137+
ActiveAction activeAction = this.activeAction;
138+
this.activeAction = null;
139+
if (activeAction != null && activeAction.type == ActionType.INCLUSION) {
140+
handler.stopInclusion();
141+
} else if (activeAction != null && activeAction.type == ActionType.EXCLUSION) {
142+
handler.stopExclusion();
143+
}
144+
}
145+
146+
private void cancelActionStopJob() {
147+
ScheduledFuture<?> actionStopJob = this.actionStopJob;
148+
if (actionStopJob != null) {
149+
actionStopJob.cancel(false);
150+
this.actionStopJob = null;
151+
}
152+
}
153+
154+
private static class ActiveAction {
155+
private final ActionType type;
156+
157+
private ActiveAction(ActionType type) {
158+
this.type = type;
159+
}
160+
}
161+
162+
private enum ActionType {
163+
INCLUSION,
164+
EXCLUSION
165+
}
112166
}

‎bundles/org.openhab.binding.zwavejs/src/main/java/org/openhab/binding/zwavejs/internal/conversion/BaseMetadata.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,7 @@ protected BaseMetadata(int nodeId, Value value) {
128128
this.min = value.metadata.min;
129129
this.max = value.metadata.max;
130130
this.id = generateChannelId(value);
131+
this.value = value.value;
131132

132133
this.label = normalizeLabel(value.metadata.label, value.endpoint, value.propertyName);
133134
this.description = value.metadata.description != null ? value.metadata.description : null;
@@ -140,7 +141,6 @@ protected BaseMetadata(int nodeId, Value value) {
140141
logger.warn("Node {}, unable to parse unitSymbol '{}', please file a bug report", nodeId, unitSymbol);
141142
}
142143
this.optionList = value.metadata.states;
143-
this.value = value.value;
144144
this.isAdvanced = isAdvanced(value.commandClass, value.propertyName, value.propertyKey);
145145

146146
if (writable) {

‎bundles/org.openhab.binding.zwavejs/src/main/java/org/openhab/binding/zwavejs/internal/conversion/ChannelMetadata.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,8 +108,12 @@ public boolean isIgnoredCommandClass(@Nullable String commandClassName) {
108108
* not possible
109109
*/
110110
public @Nullable State setState(Object value, String itemType, @Nullable String unitSymbol, boolean inverted) {
111+
return setState(value, itemType, unitSymbol, inverted, determineFactor(unitSymbol));
112+
}
113+
114+
public @Nullable State setState(Object value, String itemType, @Nullable String unitSymbol, boolean inverted,
115+
Double factor) {
111116
this.unitSymbol = normalizeUnit(unitSymbol, value);
112-
Double factor = determineFactor(unitSymbol);
113117
this.unit = UnitUtils.parseUnit(this.unitSymbol);
114118
if (unitSymbol != null && this.unit == null) {
115119
logger.warn("Node {}. Unable to parse unitSymbol '{}' from channel config, this is a bug", nodeId,

‎bundles/org.openhab.binding.zwavejs/src/main/java/org/openhab/binding/zwavejs/internal/discovery/NodeDiscoveryService.java‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -119,14 +119,14 @@ public void addNodeDiscovery(@Nullable Node node) {
119119

120120
properties.put(CONFIG_NODE_ID, node.nodeId);
121121

122-
properties.put(PROPERTY_NODE_IS_LISTENING, node.isListening);
123-
properties.put(PROPERTY_NODE_IS_ROUTING, node.isRouting);
124-
properties.put(PROPERTY_NODE_IS_SECURE, node.isSecure);
122+
properties.put(PROPERTY_NODE_IS_LISTENING, String.valueOf(node.isListening));
123+
properties.put(PROPERTY_NODE_IS_ROUTING, String.valueOf(node.isRouting));
124+
properties.put(PROPERTY_NODE_IS_SECURE, String.valueOf(node.isSecure));
125125
properties.put(PROPERTY_VENDOR, manufacturer);
126126
properties.put(PROPERTY_MODEL_ID, product);
127-
properties.put(PROPERTY_NODE_LASTSEEN, node.lastSeen);
128-
properties.put(PROPERTY_NODE_FREQ_LISTENING, node.isFrequentListening);
129-
properties.put(PROPERTY_FIRMWARE_VERSION, node.firmwareVersion);
127+
properties.put(PROPERTY_NODE_LASTSEEN, node.lastSeen != null ? node.lastSeen.toString() : "");
128+
properties.put(PROPERTY_NODE_FREQ_LISTENING, String.valueOf(node.isFrequentListening));
129+
properties.put(PROPERTY_FIRMWARE_VERSION, node.firmwareVersion != null ? node.firmwareVersion : "");
130130

131131
DiscoveryResult discoveryResult = DiscoveryResultBuilder.create(thingUID).withThingType(thingTypeUID)
132132
.withProperties(properties).withBridge(getBridgeUID()).withRepresentationProperty(CONFIG_NODE_ID)

‎bundles/org.openhab.binding.zwavejs/src/main/java/org/openhab/binding/zwavejs/internal/handler/ZwaveJSNodeHandler.java‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -301,7 +301,7 @@ public void handleCommand(ChannelUID channelUID, Command command) {
301301
} else if (command instanceof PercentType percentTypeCommand) {
302302
zwaveCommand.value = handlePercentTypeCommand(channel, colorCap, channelConfig, percentTypeCommand);
303303
} else if (command instanceof DecimalType decimalCommand) {
304-
zwaveCommand.value = decimalCommand.doubleValue();
304+
zwaveCommand.value = decimalCommand.doubleValue() / channelConfig.factor;
305305
} else if (command instanceof DateTimeType dateTimeCommand) {
306306
throw new UnsupportedOperationException(dateTimeCommand.toString() + " is currently not supported");
307307
} else if (command instanceof IncreaseDecreaseType increaseDecreaseCommand) {
@@ -552,7 +552,7 @@ public boolean onNodeStateChanged(Event event) {
552552
}
553553

554554
State state = metadata.setState(event.args.newValue, Objects.requireNonNull(channel.getAcceptedItemType()),
555-
channelConfig.incomingUnit, channelConfig.inverted);
555+
channelConfig.incomingUnit, channelConfig.inverted, channelConfig.factor);
556556

557557
if (state == null) {
558558
return true;
@@ -574,7 +574,7 @@ public boolean onNodeStateChanged(Event event) {
574574
rollerShutterCapability.setPosition(newValue.intValue(), isUpDownInverted);
575575
rollerShutterState = metadata.setState(event.args.newValue,
576576
Objects.requireNonNull(channel.getAcceptedItemType()), channelConfig.incomingUnit,
577-
rollerShutterConfig.inverted);
577+
rollerShutterConfig.inverted, channelConfig.factor);
578578
} else if (event.args.newValue instanceof Boolean newValue) {
579579
boolean isCommandForUp = channelId.equals(rollerShutterCapability.upChannel.getId());
580580
boolean isCommandForDown = channelId.equals(rollerShutterCapability.downChannel.getId());
@@ -837,13 +837,19 @@ private ThingBuilder updateChannels(ThingBuilder builder, ZwaveJSTypeGeneratorRe
837837
private void initializeChannelAndConfigState(Node node, ZwaveJSTypeGeneratorResult result) {
838838
// Set initial state for linked channels
839839
for (Channel channel : thing.getChannels()) {
840-
if (result.values.containsKey(channel.getUID().getId()) && isLinked(channel.getUID())) {
841-
ChannelMetadata dummy = new ChannelMetadata(getId(), node.values.get(0));
840+
String channelId = channel.getUID().getId();
841+
if (result.values.containsKey(channelId) && isLinked(channel.getUID())) {
842+
ChannelMetadata metadata = result.channelMetadata.get(channelId);
843+
if (metadata == null) {
844+
logger.debug("Node {}. Channel {} has a value but no metadata, skipping initial state", node.nodeId,
845+
channelId);
846+
continue;
847+
}
842848
ZwaveJSChannelConfiguration channelConfig = channel.getConfiguration()
843849
.as(ZwaveJSChannelConfiguration.class);
844-
State state = dummy.setState(Objects.requireNonNull(result.values.get(channel.getUID().getId())),
850+
State state = metadata.setState(Objects.requireNonNull(result.values.get(channelId)),
845851
Objects.requireNonNull(channel.getAcceptedItemType()), channelConfig.incomingUnit,
846-
channelConfig.inverted);
852+
channelConfig.inverted, channelConfig.factor);
847853
if (state != null) {
848854
// Initialize color and color temperature channels
849855
ColorCapability colorCap = colorCapabilities.get(channelConfig.endpoint);

‎bundles/org.openhab.binding.zwavejs/src/main/java/org/openhab/binding/zwavejs/internal/type/ZwaveJSTypeGeneratorImpl.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,7 @@ private void createRawNotificationChannel(ThingUID thingUID, Node node, ZwaveJST
254254
.build();
255255

256256
result.channels.put(details.id, channel);
257+
result.channelMetadata.put(details.id, details);
257258
}
258259
}
259260

@@ -374,6 +375,7 @@ private Map<String, Channel> createChannel(ThingUID thingUID, ZwaveJSTypeGenerat
374375
}
375376

376377
result.channels.put(details.id, builder.build());
378+
result.channelMetadata.put(details.id, details);
377379

378380
// if necessary add or update the entry in our ZwaveJSTypeGeneratorResult's map of ColorCapabilities
379381
updateColorCapabilities(thingUID, details, result);
@@ -798,6 +800,7 @@ private void addRollerShutterChannels(ThingUID thingUID, Node node, ZwaveJSTypeG
798800
.build();
799801

800802
result.channels.put(details.id, channel);
803+
result.channelMetadata.put(details.id, details);
801804
Object dimmerValue = result.values.get(rollerShutterCapability.dimmerChannel.getId());
802805
if (dimmerValue != null) {
803806
result.values.put(details.id, dimmerValue);
@@ -851,6 +854,7 @@ private void addColorTemperatureChannel(ThingUID thingUID, Node node, ZwaveJSTyp
851854
.build();
852855

853856
result.channels.put(details.id, channel);
857+
result.channelMetadata.put(details.id, details);
854858
colorCapability.colorTempChannel = channel.getUID();
855859
});
856860
}

‎bundles/org.openhab.binding.zwavejs/src/main/java/org/openhab/binding/zwavejs/internal/type/ZwaveJSTypeGeneratorResult.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import java.util.Map;
1717

1818
import org.eclipse.jdt.annotation.NonNullByDefault;
19+
import org.openhab.binding.zwavejs.internal.conversion.ChannelMetadata;
1920
import org.openhab.binding.zwavejs.internal.type.capabilities.ColorCapability;
2021
import org.openhab.binding.zwavejs.internal.type.capabilities.RollerShutterCapability;
2122
import org.openhab.core.thing.Channel;
@@ -33,6 +34,7 @@ public class ZwaveJSTypeGeneratorResult {
3334

3435
public Map<String, Channel> channels = new HashMap<>();
3536
public Map<String, Object> values = new HashMap<>();
37+
public Map<String, ChannelMetadata> channelMetadata = new HashMap<>();
3638
public Map<Integer, ColorCapability> colorCapabilities = new HashMap<>();
3739
public Map<Integer, RollerShutterCapability> rollerShutterCapabilities = new HashMap<>();
3840
public String location = "";

‎bundles/org.openhab.binding.zwavejs/src/test/java/org/openhab/binding/zwavejs/internal/action/ZwaveJSActionsTest.java‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
/*
2+
* Copyright (c) 2010-2026 Contributors to the openHAB project
3+
*
4+
* See the NOTICE file(s) distributed with this work for additional
5+
* information.
6+
*
7+
* This program and the accompanying materials are made available under the
8+
* terms of the Eclipse Public License 2.0 which is available at
9+
* http://www.eclipse.org/legal/epl-2.0
10+
*
11+
* SPDX-License-Identifier: EPL-2.0
12+
*/
13+
package org.openhab.binding.zwavejs.internal.action;
14+
15+
import static org.mockito.Mockito.*;
16+
17+
import java.util.List;
18+
import java.util.concurrent.ScheduledExecutorService;
19+
import java.util.concurrent.ScheduledFuture;
20+
import java.util.concurrent.TimeUnit;
21+
22+
import org.eclipse.jdt.annotation.NonNullByDefault;
23+
import org.junit.jupiter.api.Test;
24+
import org.mockito.ArgumentCaptor;
25+
import org.openhab.binding.zwavejs.internal.handler.ZwaveJSBridgeHandler;
26+
27+
/**
28+
* @author Leo Siepel - Initial contribution
29+
*/
30+
@NonNullByDefault
31+
public class ZwaveJSActionsTest {
32+
33+
@Test
34+
public void staleTimeoutDoesNotStopNewInvocationOfSameAction() {
35+
ScheduledExecutorService scheduler = mock(ScheduledExecutorService.class);
36+
ScheduledFuture<?> firstStopJob = mock(ScheduledFuture.class);
37+
ScheduledFuture<?> secondStopJob = mock(ScheduledFuture.class);
38+
ArgumentCaptor<Runnable> timeoutCaptor = ArgumentCaptor.forClass(Runnable.class);
39+
doReturn(firstStopJob, secondStopJob).when(scheduler).schedule(timeoutCaptor.capture(), eq(30L),
40+
eq(TimeUnit.SECONDS));
41+
42+
ZwaveJSBridgeHandler handler = mock(ZwaveJSBridgeHandler.class);
43+
ZwaveJSActions actions = new ZwaveJSActions(scheduler);
44+
actions.setThingHandler(handler);
45+
46+
actions.startInclusion();
47+
actions.startInclusion();
48+
49+
verify(handler, times(2)).startInclusion();
50+
verify(handler).stopInclusion();
51+
verify(firstStopJob).cancel(false);
52+
53+
List<Runnable> timeouts = timeoutCaptor.getAllValues();
54+
timeouts.get(0).run();
55+
56+
verify(handler).stopInclusion();
57+
verify(secondStopJob, never()).cancel(false);
58+
59+
timeouts.get(1).run();
60+
61+
verify(handler, times(2)).stopInclusion();
62+
verify(secondStopJob).cancel(false);
63+
}
64+
}

‎bundles/org.openhab.binding.zwavejs/src/test/java/org/openhab/binding/zwavejs/internal/discovery/NodeDiscoveryServiceTest.java‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -83,14 +83,14 @@ public void testAddNodeDiscovery() {
8383
DiscoveryResult result = captor.getValue();
8484
Map<String, Object> expectedProperties = new HashMap<>();
8585
expectedProperties.put("id", node.nodeId);
86-
expectedProperties.put(PROPERTY_NODE_IS_LISTENING, node.isListening);
87-
expectedProperties.put(PROPERTY_NODE_IS_ROUTING, node.isRouting);
88-
expectedProperties.put(PROPERTY_NODE_IS_SECURE, node.isSecure);
86+
expectedProperties.put(PROPERTY_NODE_IS_LISTENING, String.valueOf(node.isListening));
87+
expectedProperties.put(PROPERTY_NODE_IS_ROUTING, String.valueOf(node.isRouting));
88+
expectedProperties.put(PROPERTY_NODE_IS_SECURE, String.valueOf(node.isSecure));
8989
expectedProperties.put(PROPERTY_VENDOR, node.deviceConfig.manufacturer);
9090
expectedProperties.put(PROPERTY_MODEL_ID, node.deviceConfig.label);
91-
expectedProperties.put(PROPERTY_NODE_LASTSEEN, node.lastSeen);
92-
expectedProperties.put(PROPERTY_NODE_FREQ_LISTENING, node.isFrequentListening);
93-
expectedProperties.put(PROPERTY_FIRMWARE_VERSION, node.firmwareVersion);
91+
expectedProperties.put(PROPERTY_NODE_LASTSEEN, node.lastSeen.toString());
92+
expectedProperties.put(PROPERTY_NODE_FREQ_LISTENING, String.valueOf(node.isFrequentListening));
93+
expectedProperties.put(PROPERTY_FIRMWARE_VERSION, "");
9494

9595
assertEquals(expectedProperties, result.getProperties());
9696
assertEquals(bridgeUID, result.getBridgeUID());

0 commit comments

Comments
 (0)