Skip to content

Commit 470ef87

Browse files
committed
[ddwrt] Fix Copilot review comments and radio name formatting
- SyslogParser: Reuse static Gson instance instead of per-call allocation - SyslogParser: Add Locale.ENGLISH to timestamp DateTimeFormatter - SshRunner: Add timeout to ch.open().verify() to prevent indefinite blocking - SshClientManager: Add shutdown() method for OSGi lifecycle cleanup - DDWRTHandlerFactory: Call SshClientManager.shutdown() on deactivate - DDWRTRadioThingHandler: Pass ifaceName (not interfaceId) to setRadioEnabled - DDWRTRadioThingHandler: Normalize interfaceId in initialize for manual config - thing-types.xml: Update radio interfaceId description to document mac:iface format - ddwrt.properties: Update radio interfaceId description - DDWRTBaseDevice: Change radioName separator from hyphen to space PR openhab#20505 Assisted-by: Windsurf:Claude Opus 4.6 Signed-off-by: Lee Ballard <ballle98@gmail.com>
1 parent 98ea7e9 commit 470ef87

8 files changed

Lines changed: 42 additions & 10 deletions

File tree

bundles/org.openhab.binding.ddwrt/src/main/java/org/openhab/binding/ddwrt/internal/DDWRTHandlerFactory.java

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121

2222
import org.eclipse.jdt.annotation.NonNullByDefault;
2323
import org.eclipse.jdt.annotation.Nullable;
24+
import org.openhab.binding.ddwrt.internal.api.SshClientManager;
2425
import org.openhab.binding.ddwrt.internal.handler.DDWRTClientHandler;
2526
import org.openhab.binding.ddwrt.internal.handler.DDWRTDeviceThingHandler;
2627
import org.openhab.binding.ddwrt.internal.handler.DDWRTFirewallRuleHandler;
@@ -32,7 +33,9 @@
3233
import org.openhab.core.thing.binding.BaseThingHandlerFactory;
3334
import org.openhab.core.thing.binding.ThingHandler;
3435
import org.openhab.core.thing.binding.ThingHandlerFactory;
36+
import org.osgi.service.component.ComponentContext;
3537
import org.osgi.service.component.annotations.Component;
38+
import org.osgi.service.component.annotations.Deactivate;
3639

3740
/**
3841
* The {@link DDWRTHandlerFactory} is responsible for creating things and thing
@@ -44,6 +47,13 @@
4447
@Component(configurationPid = "binding.ddwrt", service = ThingHandlerFactory.class)
4548
public class DDWRTHandlerFactory extends BaseThingHandlerFactory {
4649

50+
@Override
51+
@Deactivate
52+
protected void deactivate(ComponentContext componentContext) {
53+
SshClientManager.getInstance().shutdown();
54+
super.deactivate(componentContext);
55+
}
56+
4757
@Override
4858
public boolean supportsThingType(ThingTypeUID thingTypeUID) {
4959
return SUPPORTED_THING_TYPES_UIDS.contains(thingTypeUID);

bundles/org.openhab.binding.ddwrt/src/main/java/org/openhab/binding/ddwrt/internal/api/DDWRTBaseDevice.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -879,7 +879,7 @@ public void onWirelessEvent(SyslogEvent event) {
879879
String iface = Objects.requireNonNull(m.group(1));
880880
String action = Objects.requireNonNull(m.group(2));
881881
String clientMac = Objects.requireNonNull(m.group(3)).toLowerCase(Locale.ROOT);
882-
String radioName = (hostname.isEmpty() ? mac : hostname) + "-" + iface;
882+
String radioName = (hostname.isEmpty() ? mac : hostname) + " " + iface;
883883

884884
if ("CONNECTED".equalsIgnoreCase(action)) {
885885
// Look up the radio to get its SSID and channel
@@ -928,7 +928,7 @@ public void onWirelessEvent(SyslogEvent event) {
928928
String iface = Objects.requireNonNull(mlme.group(1));
929929
String clientMac = Objects.requireNonNull(mlme.group(2)).toLowerCase(Locale.ROOT);
930930
String action = Objects.requireNonNull(mlme.group(3)).toLowerCase(Locale.ROOT);
931-
String radioName = (hostname.isEmpty() ? mac : hostname) + "-" + iface;
931+
String radioName = (hostname.isEmpty() ? mac : hostname) + " " + iface;
932932

933933
boolean isConnect = "assoc".equals(action) || "authenticated".equals(action);
934934
boolean isDisconnect = "disassoc".equals(action) || "deauthenticated".equals(action);
@@ -1562,7 +1562,7 @@ protected void refreshWirelessClients(SshRunner runner) {
15621562
if (!radio.getParentDeviceMac().equals(mac)) {
15631563
continue;
15641564
}
1565-
String radioName = (hostname.isEmpty() ? mac : hostname) + "-" + radio.getIfaceName();
1565+
String radioName = (hostname.isEmpty() ? mac : hostname) + " " + radio.getIfaceName();
15661566
for (String clientMac : radio.getAssoclist()) {
15671567
// Thread-safe update of wireless client (computeWirelessClient handles null case)
15681568
cache.computeWirelessClient(clientMac, client -> {

bundles/org.openhab.binding.ddwrt/src/main/java/org/openhab/binding/ddwrt/internal/api/SshClientManager.java

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,19 @@ public static SshClientManager getInstance() {
5757
return INSTANCE;
5858
}
5959

60+
/**
61+
* Stop the SSH client and release resources. Called from the handler factory
62+
* deactivate to ensure clean shutdown during OSGi bundle restarts.
63+
*/
64+
public void shutdown() {
65+
try {
66+
client.stop();
67+
logger.debug("SSH client stopped");
68+
} catch (Exception e) {
69+
logger.debug("Error stopping SSH client: {}", e.getMessage());
70+
}
71+
}
72+
6073
private final SshClient client;
6174
private final String ohPrivateKeyDirString = OpenHAB.getUserDataFolder() + "/ddwrt/keys";
6275

bundles/org.openhab.binding.ddwrt/src/main/java/org/openhab/binding/ddwrt/internal/api/SshRunner.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ public CommandResult execResult(String command, Duration timeout) throws IOExcep
9494
ByteArrayOutputStream err = new ByteArrayOutputStream();
9595
ch.setOut(out);
9696
ch.setErr(err);
97-
ch.open().verify();
97+
ch.open().verify(timeout.toMillis());
9898
Set<ClientChannelEvent> events = ch.waitFor(
9999
EnumSet.of(ClientChannelEvent.CLOSED, ClientChannelEvent.EXIT_STATUS), timeout.toMillis());
100100
Integer rc = ch.getExitStatus();

bundles/org.openhab.binding.ddwrt/src/main/java/org/openhab/binding/ddwrt/internal/api/SyslogParser.java

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import java.time.LocalDateTime;
1717
import java.time.ZoneId;
1818
import java.time.format.DateTimeFormatter;
19+
import java.util.Locale;
1920
import java.util.Objects;
2021
import java.util.regex.Matcher;
2122
import java.util.regex.Pattern;
@@ -40,6 +41,8 @@ public class SyslogParser {
4041

4142
private final Logger logger;
4243

44+
private static final Gson GSON = new Gson();
45+
4346
private static final Pattern ANSI_ESCAPE = Pattern.compile("\u001B\\[[;?0-9]*[ -/]*[@-~]");
4447
private static final Pattern OSC_SEQUENCE = Pattern.compile("\u001B\\].*?(\u0007|\u001B\\\\)");
4548

@@ -289,7 +292,7 @@ private static String stripControlCodes(String value) {
289292
// Normalize single-digit days (e.g., "Mar 4" -> "Mar 4") by removing extra space
290293
String normalizedTimestamp = timestampStr.replaceFirst("^(\\w{3}) (\\d{1,2})", "$1 $2");
291294
LocalDateTime dt = LocalDateTime.parse(currentYear + " " + normalizedTimestamp,
292-
DateTimeFormatter.ofPattern("yyyy MMM d HH:mm:ss"));
295+
DateTimeFormatter.ofPattern("yyyy MMM d HH:mm:ss", Locale.ENGLISH));
293296
return dt.atZone(ZoneId.systemDefault()).toInstant();
294297
}
295298
} catch (Exception e) {
@@ -304,8 +307,7 @@ private static String stripControlCodes(String value) {
304307
*/
305308
private @Nullable SyslogEvent parseUbusLine(String jsonLine) {
306309
try {
307-
Gson gson = new Gson();
308-
JsonObject root = gson.fromJson(jsonLine, JsonObject.class);
310+
JsonObject root = GSON.fromJson(jsonLine, JsonObject.class);
309311
if (root == null) {
310312
return null;
311313
}

bundles/org.openhab.binding.ddwrt/src/main/java/org/openhab/binding/ddwrt/internal/handler/DDWRTRadioThingHandler.java

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,12 @@ protected boolean initialize(DDWRTRadioConfiguration config) {
6565
"@text/offline.conf-error-no-interfaceid");
6666
return false;
6767
}
68+
// If interfaceId is just the iface name (e.g. "wl0") and parentDeviceMac is set,
69+
// build the full cache key "mac:iface" so cache lookups work for manual configs.
70+
if (!config.interfaceId.contains(":") && !config.parentDeviceMac.isEmpty()) {
71+
config.interfaceId = config.parentDeviceMac.toLowerCase(Locale.ROOT) + ":" + config.interfaceId;
72+
logger.debug("Normalized interfaceId to {}", config.interfaceId);
73+
}
6874
return true;
6975
}
7076

@@ -96,7 +102,7 @@ protected boolean handleCommand(DDWRTNetwork network, DDWRTRadio radio, ChannelU
96102
boolean enabled = onOff == OnOffType.ON;
97103
logger.debug("{} radio {}", enabled ? "Enabling" : "Disabling", radio.getInterfaceId());
98104

99-
boolean success = network.setRadioEnabled(radio.getParentDeviceMac(), radio.getInterfaceId(), enabled);
105+
boolean success = network.setRadioEnabled(radio.getParentDeviceMac(), radio.getIfaceName(), enabled);
100106
if (success) {
101107
logger.debug("Radio {} command sent successfully", radio.getInterfaceId());
102108
// State will be updated on next network refresh cycle

bundles/org.openhab.binding.ddwrt/src/main/resources/OH-INF/i18n/ddwrt.properties

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ thing-type.config.ddwrt.network.user.description = Default SSH username. Precede
6161
thing-type.config.ddwrt.network.useSystemUser.label = Use System User
6262
thing-type.config.ddwrt.network.useSystemUser.description = Ignore the User field and resolve the username from ~/.ssh/config or the system username.
6363
thing-type.config.ddwrt.radio.interfaceId.label = Interface ID
64-
thing-type.config.ddwrt.radio.interfaceId.description = Wireless interface identifier (e.g. wl0, ath0, wlan0)
64+
thing-type.config.ddwrt.radio.interfaceId.description = Wireless interface identifier. Discovery sets this to deviceMAC:iface (e.g. aa:bb:cc:dd:ee:ff:wl0). For manual configuration, use just the interface name (e.g. wl0) together with parentDeviceMac.
6565
thing-type.config.ddwrt.radio.parentDeviceMac.label = Parent Device MAC
6666
thing-type.config.ddwrt.radio.parentDeviceMac.description = MAC address of the parent device
6767
thing-type.config.ddwrt.client.hostname.label = Hostname

bundles/org.openhab.binding.ddwrt/src/main/resources/OH-INF/thing/thing-types.xml

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,8 @@
208208
<config-description>
209209
<parameter name="interfaceId" type="text" required="true">
210210
<label>Interface ID</label>
211-
<description>Wireless interface identifier (e.g. wl0, ath0, wlan0)</description>
211+
<description>Wireless interface identifier. Discovery sets this to deviceMAC:iface (e.g. aa:bb:cc:dd:ee:ff:wl0). For
212+
manual configuration, use just the interface name (e.g. wl0) together with parentDeviceMac.</description>
212213
</parameter>
213214
<parameter name="parentDeviceMac" type="text" required="false">
214215
<label>Parent Device MAC</label>

0 commit comments

Comments
 (0)