Fixes after code review, take 2.

Signed-off-by: Espen Fossen <espenaf@junta.no>
openhab · Jul 21, 2022 · f775ddd · f775ddd
1 parent 4e065db
commit f775ddd
Show file tree

Hide file tree

Showing 12 changed files with 62 additions and 65 deletions.
diff --git a/bundles/org.openhab.binding.nobohub/README.md b/bundles/org.openhab.binding.nobohub/README.md
@@ -1,6 +1,6 @@
 # NoboHub Binding
 
-This binding controls the Glen Dimplex Nobø Hub using the Nobø Hub API v1.1 that can be found <a href="https://www.glendimplex.se/media/15650/nobo-hub-api-v-1-1-integration-for-advanced-users.pdf">here</a>
+This binding controls the Glen Dimplex Nobø Hub using the <a href="https://www.glendimplex.se/media/15650/nobo-hub-api-v-1-1-integration-for-advanced-users.pdf">Nobø Hub API v1.1</a>.
 
 ![Nobo Hub](doc/nobohub.jpg)
 
@@ -16,20 +16,26 @@ This binding is tested with the following devices:
 
 Not all thermostats are made equal.
 
-* NCU-1R: Comfort temperature setting on device overrides values from Hub. Making Comfort settings in the Hub useless.
+* NCU-1R: Comfort temperature setting on the device overrides values from the Hub, making the setting in the Hub useless.
 * NCU-2R: Synchronizes temperature settings to and from the Hub.
 
 ## Supported Things
 
-Nobø Hub is the hub that communicates with switches and thermostats.
+| Thing     | Thing Type | Description                                                                                     |
+|-----------|------------|-------------------------------------------------------------------------------------------------|
+| hub       | Bridge     | The Nobø Hub provides a gateway between your components, with the ability to organise in zones. |
+| component | Thing      | A component is a device, i.e. panel heater or switch.                                           |
+| zone      | Thing      | A zone can hold one or more components.                                                         |
+
 
 ## Discovery
 
-The hub will be automatically discovered. Before it can be used, you will have to update the configuration with the last three digits of its serial number.
+The hub will be automatically discovered. 
+Before it can be used, you will have to update the configuration with the last three digits of its serial number.
 
 When the hub is configured with the correct serial number, it will autodetect zones and components (thermostats and switches).
 
-## Binding Configuration
+## Thing Configuration
 
 ```
 # Configuration for Nobø Hub
@@ -75,9 +81,9 @@ Not all devices report this.
 ### nobo.things
 
 ```
-Bridge nobohub:nobohub:controller "Nobø Hub" [ hostName="192.168.1.10", serialNumber="SERIAL_NUMBER" ] {
-	Thing zone 1                             "Zone - Kitchen"            	[ id=1 ]
-	Thing component SERIAL_NUMBER_COMPONENT  "Heater - Kitchen"         	[ serialNumber="SERIAL_NUMBER_COMPONENT" ]
+Bridge nobohub:nobohub:controller "Nobø Hub" [ hostName="192.168.1.10", serialNumber="103000000000" ] {
+	Thing zone 1                  "Zone - Kitchen"            	[ id=1 ]
+	Thing component 184000000000  "Heater - Kitchen"         	[ serialNumber="184000000000" ]
 }
 ```
 
@@ -88,15 +94,15 @@ Bridge nobohub:nobohub:controller "Nobø Hub" [ hostName="192.168.1.10", serialN
 String	            Nobo_Hub_GlobalOverride         "Global Override %s"                <heating>       {channel="nobohub:nobohub:controller:activeOverrideName"}
 
 // Panel Heater
-Number:Temperature	PanelHeater_CurrentTemperatur   "Setpoint [%.1f °C]"                <temperature>   {channel="nobohub:component:controller:SERIAL_NUMBER_COMPONENT:currentTemperature"}
+Number:Temperature	PanelHeater_CurrentTemperature  "Setpoint [%.1f °C]"                <temperature>   {channel="nobohub:component:controller:184000000000:currentTemperature"}
 
 // Zone
 String	            Zone_ActiveWeekProfileName      "Active week profile name [%s]"     <calendar>      {channel="nobohub:zone:controller:1:activeWeekProfileName"}
 Number	            Zone_ActiveWeekProfile          "Active week profile [%d]"          <calendar>      {channel="nobohub:zone:controller:1:activeWeekProfile"}
 String	            Zone_ActiveStatus               "Active status %s]"                 <heating>       {channel="nobohub:zone:controller:1:calculatedWeekProfileStatus"}
-Number:Temperature	Zone_ComfortTemperatur          "Comfort temperature [%.1f °C]"     <temperature>   {channel="nobohub:zone:controller:1:comfortTemperature"}
+Number:Temperature	Zone_ComfortTemperature         "Comfort temperature [%.1f °C]"     <temperature>   {channel="nobohub:zone:controller:1:comfortTemperature"}
 Number:Temperature	Zone_EcoTemperatur              "Eco temperature [%.1f °C]"         <temperature>   {channel="nobohub:zone:controller:1:ecoTemperature"}
-Number:Temperature	Zone_CurrentTemperatur          "Current temperature [%.1f °C]"     <temperature>   {channel="nobohub:zone:controller:1:currentTemperature"}
+Number:Temperature	Zone_CurrentTemperature         "Current temperature [%.1f °C]"     <temperature>   {channel="nobohub:zone:controller:1:currentTemperature"}
 ```
 
 ### nobo.sitemap
@@ -124,9 +130,11 @@ sitemap nobo label="Nobø " {
 ## Organize your setup
 
 Nobø Hub uses a combination of status types (Normal, Comfort, Eco, Away), profiles types (Comfort, Eco, Away, Off), predefined temperature types (Comfort, Eco, Away), zones and override settings to organize and enable different features.
-This makes it possible to control the heaters in many different scenarios and combinations. The following is a suggested way of organizing the binding with the Hub for a good level of control and flexibility.
+This makes it possible to control the heaters in many different scenarios and combinations. 
+The following is a suggested way of organizing the binding with the Hub for a good level of control and flexibility.
 
-If you own panels with a physical Comfort temperature override, you need to use the Eco temperature type for setting level used by the day based profiles. If not, you can use either Comfort or Eco to set wanted leve.
+If you own panels with a physical Comfort temperature override, you need to use the Eco temperature type for setting level used by the day based profiles. 
+If not, you can use either Comfort or Eco to set wanted level.
 
 Start by creating the following profiles in the Nobø Hub App:
 
@@ -141,9 +149,11 @@ Start by creating the following profiles in the Nobø Hub App:
     Every day 06->16    Set to status [Comfort|Eco] between 06->16 every day, otherwise set to [Away|Off]
     Every day 06->23    Set to status [Comfort|Eco] between 06->23 every day, otherwise set to [Away|Off]
 
-Next set [Comfort|Eco] level for each zone to your requirements. For a more advanced setup, you can create a rule which both sets temperature level and profile.
+Next set [Comfort|Eco] level for each zone to your requirements. 
+For a more advanced setup, you can create a rule which both sets temperature level and profile.
 
-Then create a sitemap with a Selection pointing to the Week Profile item. The binding will now automatically update all available week profile options in the selection button:
+Then create a sitemap with a Selection pointing to the Week Profile item. 
+The binding will now automatically update all available week profile options in the selection button:
 
 ### nobo.sitemap
 
@@ -155,13 +165,3 @@ sitemap nobo label="Nobø " {
     }
 }
 ```
-
-## Bugs and logging
-
-If you find any bugs or unwanted behaviour, please contact the maintainer. To help the maintainer it would be great if you could send logs with a description of what is wrong. To turn on logging, go to the Keraf console and run
-
-log:set DEBUG org.openhab.binding.nobohub
-
-To see the log:
-
-log:tail
diff --git a/....binding.nobohub/src/main/java/org/openhab/binding/nobohub/internal/ComponentHandler.java b/....binding.nobohub/src/main/java/org/openhab/binding/nobohub/internal/ComponentHandler.java
@@ -62,12 +62,8 @@ public void onUpdate(Component component) {
 
         double temp = component.getTemperature();
         if (!Double.isNaN(temp)) {
-            try {
-                QuantityType<Temperature> currentTemperature = new QuantityType<>(temp, SIUnits.CELSIUS);
-                updateState(CHANNEL_COMPONENT_CURRENT_TEMPERATURE, currentTemperature);
-            } catch (NumberFormatException nfe) {
-                logger.debug("Couldn't set decimal value to temperature: {}", temp);
-            }
+            QuantityType<Temperature> currentTemperature = new QuantityType<>(temp, SIUnits.CELSIUS);
+            updateState(CHANNEL_COMPONENT_CURRENT_TEMPERATURE, currentTemperature);
         }
 
         updateProperty(Thing.PROPERTY_SERIAL_NUMBER, component.getSerialNumber().toString());
@@ -107,7 +103,7 @@ public void initialize() {
             SerialNumber sn = new SerialNumber(serialNumberString);
             if (!sn.isWellFormed()) {
                 updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR,
-                        "@text/message.component.illegal.serial [\"" + serialNumber + "\"]");
+                        "@text/message.component.illegal.serial [\"" + serialNumberString + "\"]");
             } else {
                 this.serialNumber = sn;
                 updateStatus(ThingStatus.ONLINE);
@@ -148,10 +144,9 @@ public void handleCommand(ChannelUID channelUID, Command command) {
         Bridge noboHub = getBridge();
         if (null != noboHub) {
             NoboHubBridgeHandler hubHandler = (NoboHubBridgeHandler) noboHub.getHandler();
+            SerialNumber serialNumber = this.serialNumber;
             if (null != serialNumber && null != hubHandler) {
-                SerialNumber sn = Helpers.castToNonNull(serialNumber, Thing.PROPERTY_SERIAL_NUMBER);
-                NoboHubBridgeHandler hh = Helpers.castToNonNull(hubHandler, "hubHandler");
-                return hh.getComponent(sn);
+                return hubHandler.getComponent(serialNumber);
             }
         }
 

diff --git a/...g.nobohub/src/main/java/org/openhab/binding/nobohub/internal/NoboHubBindingConstants.java b/...g.nobohub/src/main/java/org/openhab/binding/nobohub/internal/NoboHubBindingConstants.java
@@ -100,14 +100,14 @@ public class NoboHubBindingConstants {
     // Mappings
 
     public static final Map<String, String> REJECT_REASONS = Stream.of(new String[][] {
-            { "0", "Client command set too old, run it in with debug logs and let the maintainer know" },
-            { "1", "Hub serial number mismatch (should be 12 digits, if hub was autodetected, plase add the last three)" },
-            { "2", "Wrong number of arguments, run it in with debug logs and let the maintainer know" },
-            { "3", "Timestamp incorrectly formatted, run it in with debug logs and let the maintainer know" }, })
+            { "0", "Client command set too old. Please run with debug logs." },
+            { "1", "Hub serial number mismatch. Should be 12 digits, if hub was autodetected, please add the last three." },
+            { "2", "Wrong number of arguments. Please run with debug logs." },
+            { "3", "Timestamp incorrectly formatted. Please run with debug logs." }, })
             .collect(Collectors.collectingAndThen(Collectors.toMap(data -> data[0], data -> data[1]),
                     Collections::<String, String> unmodifiableMap));
 
-    // Full list of units: http://help.nobo.no/skriver/?chapterid=344&chapterlanguageid=2
+    // Full list of units: https://help.nobo.no/skriver/?chapterid=344&chapterlanguageid=2
     public static final Map<String, String> SERIALNUMBERS_FOR_TYPES = Stream
             .of(new String[][] { { "120", "RS-700" }, { "168", "NCU-2R" }, { "184", "NCU-1R" }, { "186", "NTD-4R" },
                     { "192", "TXF" }, { "198", "NCU-ER" }, { "210", "NTB-2R" }, { "234", "Nobø Switch" }, })

diff --git a/...ding.nobohub/src/main/java/org/openhab/binding/nobohub/internal/NoboHubBridgeHandler.java b/...ding.nobohub/src/main/java/org/openhab/binding/nobohub/internal/NoboHubBridgeHandler.java
@@ -21,8 +21,6 @@
 import java.time.Duration;
 import java.util.Collection;
 
-import javax.validation.constraints.NotNull;
-
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
 import org.openhab.binding.nobohub.internal.connection.HubCommunicationThread;
@@ -72,15 +70,13 @@ public class NoboHubBridgeHandler extends BaseBridgeHandler {
     private @Nullable NoboThingDiscoveryService discoveryService;
     private @Nullable Hub hub;
 
-    private final @NotNull OverrideRegister overrideRegister = new OverrideRegister();
-    private final @NotNull WeekProfileRegister weekProfileRegister = new WeekProfileRegister();
-    private final @NotNull ZoneRegister zoneRegister = new ZoneRegister();
-    private final @NotNull ComponentRegister componentRegister = new ComponentRegister();
-    private final NoboHubTranslationProvider messages;
+    private final OverrideRegister overrideRegister = new OverrideRegister();
+    private final WeekProfileRegister weekProfileRegister = new WeekProfileRegister();
+    private final ZoneRegister zoneRegister = new ZoneRegister();
+    private final ComponentRegister componentRegister = new ComponentRegister();
 
-    public NoboHubBridgeHandler(Bridge bridge, NoboHubTranslationProvider messages) {
+    public NoboHubBridgeHandler(Bridge bridge) {
         super(bridge);
-        this.messages = messages;
     }
 
     @Override
@@ -179,7 +175,8 @@ public void initialize() {
                     updateProperty(Thing.PROPERTY_SERIAL_NUMBER, serialNumber);
                     updateStatus(ThingStatus.ONLINE);
                 } else {
-                    updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR);
+                    updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR,
+                            "@text/message.bridge.connection.failed");
                 }
             } catch (NoboCommunicationException commEx) {
                 updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, commEx.getMessage());

diff --git a/...ing.nobohub/src/main/java/org/openhab/binding/nobohub/internal/NoboHubHandlerFactory.java b/...ing.nobohub/src/main/java/org/openhab/binding/nobohub/internal/NoboHubHandlerFactory.java
@@ -77,12 +77,12 @@ public boolean supportsThingType(ThingTypeUID thingTypeUID) {
         ThingTypeUID thingTypeUID = thing.getThingTypeUID();
 
         if (THING_TYPE_HUB.equals(thingTypeUID)) {
-            NoboHubBridgeHandler handler = new NoboHubBridgeHandler((Bridge) thing, i18nProvider);
+            NoboHubBridgeHandler handler = new NoboHubBridgeHandler((Bridge) thing);
             registerDiscoveryService(handler);
             return handler;
         } else if (THING_TYPE_ZONE.equals(thingTypeUID)) {
             logger.debug("Setting WeekProfileStateDescriptionOptionsProvider for: {}", thing.getLabel());
-            return new ZoneHandler(thing, stateDescriptionOptionsProvider);
+            return new ZoneHandler(thing, i18nProvider, stateDescriptionOptionsProvider);
         } else if (THING_TYPE_COMPONENT.equals(thingTypeUID)) {
             return new ComponentHandler(thing, i18nProvider);
         }

diff --git a/...enhab.binding.nobohub/src/main/java/org/openhab/binding/nobohub/internal/ZoneHandler.java b/...enhab.binding.nobohub/src/main/java/org/openhab/binding/nobohub/internal/ZoneHandler.java
@@ -61,11 +61,14 @@ public class ZoneHandler extends BaseThingHandler {
 
     private final WeekProfileStateDescriptionOptionsProvider weekProfileStateDescriptionOptionsProvider;
 
+    private final NoboHubTranslationProvider messages;
+
     protected int id;
 
-    public ZoneHandler(Thing thing,
+    public ZoneHandler(Thing thing, NoboHubTranslationProvider messages,
             WeekProfileStateDescriptionOptionsProvider weekProfileStateDescriptionOptionsProvider) {
         super(thing);
+        this.messages = messages;
         this.weekProfileStateDescriptionOptionsProvider = weekProfileStateDescriptionOptionsProvider;
     }
 
@@ -81,18 +84,14 @@ public void onUpdate(Zone zone) {
 
         Double temp = zone.getTemperature();
         if (temp != null && temp != Double.NaN) {
-            try {
-                QuantityType<Temperature> currentTemperature = new QuantityType<>(temp, SIUnits.CELSIUS);
-                updateState(CHANNEL_ZONE_CURRENT_TEMPERATURE, currentTemperature);
-            } catch (NumberFormatException nfe) {
-                logger.debug("Could not set decimal value to temperature: {}", temp);
-            }
+            QuantityType<Temperature> currentTemperature = new QuantityType<>(temp, SIUnits.CELSIUS);
+            updateState(CHANNEL_ZONE_CURRENT_TEMPERATURE, currentTemperature);
         }
 
         int activeWeekProfileId = zone.getActiveWeekProfileId();
         Bridge noboHub = getBridge();
         if (null != noboHub) {
-            logger.debug("Updating zone: {} at hub brige: {}", zone.getName(),
+            logger.debug("Updating zone: {} at hub bridge: {}", zone.getName(),
                     noboHub.getStatusInfo().getStatus().name());
             NoboHubBridgeHandler hubHandler = (NoboHubBridgeHandler) noboHub.getHandler();
             if (hubHandler != null) {
@@ -139,7 +138,8 @@ public void handleCommand(ChannelUID channelUID, Command command) {
             Zone zone = getZone();
             if (null == zone) {
                 logger.debug("Could not find Zone with id {} for channel {}", id, channelUID);
-                updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.GONE);
+                updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.GONE,
+                        messages.getText("message.zone.notfound", id, channelUID));
             } else {
                 onUpdate(zone);
                 Bridge noboHub = getBridge();

diff --git a/...src/main/java/org/openhab/binding/nobohub/internal/connection/HubCommunicationThread.java b/...src/main/java/org/openhab/binding/nobohub/internal/connection/HubCommunicationThread.java
@@ -89,7 +89,7 @@ public void run() {
                 hubConnection.disconnect();
             }
         } catch (NoboCommunicationException nce) {
-            logger.error("Error disconnecting from Hub", nce);
+            logger.debug("Error disconnecting from Hub", nce);
         }
     }
 

diff --git a/...src/main/java/org/openhab/binding/nobohub/internal/discovery/NoboHubDiscoveryService.java b/...src/main/java/org/openhab/binding/nobohub/internal/discovery/NoboHubDiscoveryService.java
@@ -101,7 +101,7 @@ public void run() {
                 MulticastSocket socket = new MulticastSocket(NOBO_HUB_MULTICAST_PORT);
                 found = waitOnSocket(socket, "multicast");
             } catch (IOException ioex) {
-                logger.error("Failed detecting Nobø Hub multicast", ioex);
+                logger.error("Failed detecting Nobø Hub via multicast", ioex);
             }
 
             if (!found) {

diff --git a/...c/main/java/org/openhab/binding/nobohub/internal/discovery/NoboThingDiscoveryService.java b/...c/main/java/org/openhab/binding/nobohub/internal/discovery/NoboThingDiscoveryService.java
@@ -15,6 +15,7 @@
 import static org.openhab.binding.nobohub.internal.NoboHubBindingConstants.AUTODISCOVERED_THING_TYPES_UIDS;
 import static org.openhab.binding.nobohub.internal.NoboHubBindingConstants.PROPERTY_MODEL;
 import static org.openhab.binding.nobohub.internal.NoboHubBindingConstants.PROPERTY_NAME;
+import static org.openhab.binding.nobohub.internal.NoboHubBindingConstants.PROPERTY_TEMPERATURE_SENSOR_FOR_ZONE;
 import static org.openhab.binding.nobohub.internal.NoboHubBindingConstants.PROPERTY_VENDOR_NAME;
 import static org.openhab.binding.nobohub.internal.NoboHubBindingConstants.PROPERTY_ZONE;
 import static org.openhab.binding.nobohub.internal.NoboHubBindingConstants.PROPERTY_ZONE_ID;
@@ -140,7 +141,7 @@ public void detectComponents(Collection<Component> components) {
                     if (zoneId >= 0) {
                         String tempForZoneName = getZoneName(zoneId);
                         if (tempForZoneName != null) {
-                            properties.put("temperatureSensorForZone", tempForZoneName);
+                            properties.put(PROPERTY_TEMPERATURE_SENSOR_FOR_ZONE, tempForZoneName);
                         }
                     }
 

diff --git a/bundles/org.openhab.binding.nobohub/src/main/resources/OH-INF/i18n/nobohub.properties b/bundles/org.openhab.binding.nobohub/src/main/resources/OH-INF/i18n/nobohub.properties
@@ -50,6 +50,8 @@ channel-type.nobohub.weekProfiles-channel-type.description = Name of the active
 message.missing.serial = Missing serial number in configuration
 message.bridge.status.failed = Failed to get status: {0}
 message.bridge.missing.hostname = Missing host name in configuration
+message.bridge.connection.failed = Failed to connect, check network connectivity and configuration
 message.component.illegal.serial = Illegal serial number: {0}
 message.component.notfound = Could not find Component with serial number {0} for channel {1}
 message.component.missing.id = Id not set for channel {0}
+message.zone.notfound = Could not find Zone with id {0} for channel {1}
diff --git a/bundles/org.openhab.binding.nobohub/src/main/resources/OH-INF/i18n/nobohub_no.properties b/bundles/org.openhab.binding.nobohub/src/main/resources/OH-INF/i18n/nobohub_no.properties
@@ -64,6 +64,8 @@ channel-type.nobohub.activeWeekProfile-channel-type.description = Id p
 message.missing.serial = Mangler serialnummer i konfigurasjon
 message.bridge.status.failed = Kunne ikke hente status: {0}
 message.bridge.missing.hostname = Mangler tjenernavn i konfigurasjon
+message.bridge.connection.failed = Kunne ikke koble til, sjekk nettverksforbindelsen og konfigurasjon
 message.component.illegal.serial = Serialnummer er ukjent eller feil: {0}
 message.component.notfound = Kunne ikke finne Komponent med serialnummer {0} for kanal {1}
 message.component.missing.id = Id er ikke satt for kanal {0}
+message.zone.notfound = Kunne ikke finne Sone med id {0} for kanal {1}
diff --git a/bundles/org.openhab.binding.nobohub/src/main/resources/OH-INF/thing/thing-types.xml b/bundles/org.openhab.binding.nobohub/src/main/resources/OH-INF/thing/thing-types.xml
@@ -104,7 +104,7 @@
 		<description>The current temperature from a device that supports reporting temperatures</description>
 		<category>Temperature</category>
 		<tags>
-			<tag>Setpoint</tag>
+			<tag>Measurement</tag>
 			<tag>Temperature</tag>
 		</tags>
 		<state pattern="%.3f °C" readOnly="true"/>