Applied some review remarks

Signed-off-by: Sönke Küper <[email protected]>
openhab · Nov 28, 2021 · 7d8d6da · 7d8d6da
1 parent 9cd65e9
commit 7d8d6da
Show file tree

Hide file tree

Showing 11 changed files with 513 additions and 47 deletions.
diff --git a/bundles/org.openhab.binding.deutschebahn/README.md b/bundles/org.openhab.binding.deutschebahn/README.md
@@ -1,7 +1,7 @@
-# DeutscheBahn Binding
+# Deutsche Bahn Binding
 
-The DeutscheBahn Binding provides the latest timetable information for all trains that arrive or depart at a specific train station, including live information for delays and changes in timetable.
-The information are requested from the timetable api of DeutscheBahn developer portal, so you'll need a (free) developer account to use this binding.
+The Deutsche Bahn Binding provides the latest timetable information for all trains that arrive or depart at a specific train station, including live information for delays and changes in timetable.
+The information are requested from the timetable api of Deutsche Bahn developer portal, so you'll need a (free) developer account to use this binding.
 
 ## Supported Things
 
@@ -12,9 +12,9 @@ The information are requested from the timetable api of DeutscheBahn developer p
 
 ### Generate Access-Key for timetable API
 
-To configure a timetable you first need to register at DeutscheBahn developer portal and register for timetable API to get an access key.
+To configure a timetable you first need to register at Deutsche Bahn developer portal and register for timetable API to get an access key.
 
-1. Go to [DeutscheBahn Developer](https://developer.deutschebahn.com)
+1. Go to [Deutsche Bahn Developer](https://developer.deutschebahn.com)
 2. Register new account or login with an existing one
 3. If no application is configured yet (check Tab "Meine Anwendungen") create a new application. Only the name is required, any other fields can be left blank.
 4. Go to APIs - Timetables v1 (may be displayed on second page)
@@ -37,7 +37,7 @@ In addition you can configure if only arrivals, only departures or all trains sh
 
 | Property | Default | Required | Description |
 |-|-|-|-|
-| `accessToken` | | Yes | The access token for the timetable api within the developer portal of DeutscheBahn. |
+| `accessToken` | | Yes | The access token for the timetable api within the developer portal of Deutsche Bahn. |
 | `evaNo` | | Yes | The eva nr. of the train station for which the timetable will be requested.|
 | `trainFilter` |  | Yes | Selects the trains that will be displayed in the timetable. Either only arrivals, only departures or all trains can be displayed. |
 

diff --git a/bundles/org.openhab.binding.deutschebahn/pom.xml b/bundles/org.openhab.binding.deutschebahn/pom.xml
@@ -12,7 +12,7 @@
 
   <artifactId>org.openhab.binding.deutschebahn</artifactId>
 
-  <name>openHAB Add-ons :: Bundles :: DeutscheBahn Binding</name>
+  <name>openHAB Add-ons :: Bundles :: Deutsche Bahn Binding</name>
 
   <build>
     <plugins>

diff --git a/.../DeutscheBahnTimetableHandlerFactory.java → .../internal/DeutscheBahnHandlerFactory.java b/.../DeutscheBahnTimetableHandlerFactory.java → .../internal/DeutscheBahnHandlerFactory.java
@@ -30,13 +30,13 @@
 import org.osgi.service.component.annotations.Component;
 
 /**
- * The {@link DeutscheBahnTimetableHandlerFactory} is responsible for creating things and thing handlers.
+ * The {@link DeutscheBahnHandlerFactory} is responsible for creating things and thing handlers.
  *
  * @author Sönke Küper - Initial contribution
  */
 @NonNullByDefault
 @Component(configurationPid = "binding.deutschebahn", service = ThingHandlerFactory.class)
-public class DeutscheBahnTimetableHandlerFactory extends BaseThingHandlerFactory {
+public class DeutscheBahnHandlerFactory extends BaseThingHandlerFactory {
 
     private static final Set<ThingTypeUID> SUPPORTED_THING_TYPES_UIDS = Set.of(TIMETABLE_TYPE, TRAIN_TYPE);
 

diff --git a/...src/main/java/org/openhab/binding/deutschebahn/internal/DeutscheBahnTimetableHandler.java b/...src/main/java/org/openhab/binding/deutschebahn/internal/DeutscheBahnTimetableHandler.java
@@ -36,6 +36,7 @@
 import org.openhab.binding.deutschebahn.internal.timetable.dto.TimetableStop;
 import org.openhab.core.io.net.http.HttpUtil;
 import org.openhab.core.thing.Bridge;
+import org.openhab.core.thing.Channel;
 import org.openhab.core.thing.ChannelUID;
 import org.openhab.core.thing.Thing;
 import org.openhab.core.thing.ThingStatus;
@@ -44,6 +45,7 @@
 import org.openhab.core.thing.binding.BaseBridgeHandler;
 import org.openhab.core.thing.binding.ThingHandler;
 import org.openhab.core.types.Command;
+import org.openhab.core.types.UnDefType;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.xml.sax.SAXException;
@@ -100,7 +102,6 @@ public int getMaxPosition() {
     private @Nullable ScheduledFuture<?> updateJob;
 
     private final Logger logger = LoggerFactory.getLogger(DeutscheBahnTimetableHandler.class);
-    private @Nullable DeutscheBahnTimetableConfiguration config;
     private @Nullable TimetableLoader loader;
 
     private TimetablesV1ApiFactory timetablesV1ApiFactory;
@@ -112,22 +113,22 @@ public int getMaxPosition() {
      */
     public DeutscheBahnTimetableHandler( //
             final Bridge bridge, //
-            TimetablesV1ApiFactory timetablesV1ApiFactory, //
+            final TimetablesV1ApiFactory timetablesV1ApiFactory, //
             final Supplier<Date> currentTimeProvider) {
         super(bridge);
         this.timetablesV1ApiFactory = timetablesV1ApiFactory;
         this.currentTimeProvider = currentTimeProvider;
     }
 
     private List<TimetableStop> loadTimetable() {
-        if (this.loader == null) {
-            this.updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR);
+        final TimetableLoader currentLoader = this.loader;
+        if (currentLoader == null) {
+            this.updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.HANDLER_INITIALIZING_ERROR);
             return Collections.emptyList();
         }
 
         try {
-            @SuppressWarnings("null")
-            final List<TimetableStop> stops = this.loader.getTimetableStops();
+            final List<TimetableStop> stops = currentLoader.getTimetableStops();
             this.updateStatus(ThingStatus.ONLINE);
             return stops;
         } catch (final IOException e) {
@@ -143,16 +144,14 @@ private List<TimetableStop> loadTimetable() {
     public void handleCommand(final ChannelUID channelUID, final Command command) {
     }
 
-    @SuppressWarnings("null")
     @Override
     public void initialize() {
-        this.config = this.getConfigAs(DeutscheBahnTimetableConfiguration.class);
+        final DeutscheBahnTimetableConfiguration config = this.getConfigAs(DeutscheBahnTimetableConfiguration.class);
 
         try {
-            final TimetablesV1Api api = this.timetablesV1ApiFactory.create(this.config.accessToken,
-                    HttpUtil::executeUrl);
+            final TimetablesV1Api api = this.timetablesV1ApiFactory.create(config.accessToken, HttpUtil::executeUrl);
 
-            final TimetableStopFilter stopFilter = this.config.getTimetableStopFilter();
+            final TimetableStopFilter stopFilter = config.getTimetableStopFilter();
 
             final EventType eventSelection = stopFilter == TimetableStopFilter.ARRIVALS ? EventType.ARRIVAL
                     : EventType.ARRIVAL;
@@ -162,7 +161,7 @@ public void initialize() {
                     stopFilter, //
                     eventSelection, //
                     currentTimeProvider, //
-                    this.config.evaNo, //
+                    config.evaNo, //
                     1); // will be updated on first call
 
             this.updateStatus(ThingStatus.UNKNOWN);
@@ -173,7 +172,7 @@ public void initialize() {
             });
         } catch (JAXBException | SAXException | URISyntaxException e) {
             this.logger.error("Error initializing api", e);
-            this.updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR);
+            this.updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, e.getMessage());
         }
     }
 
@@ -221,23 +220,41 @@ private void stopUpdateJob() {
         }
     }
 
-    @SuppressWarnings("null")
     private void updateChannels() {
-        if (this.loader == null) {
-            this.updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR);
+        final TimetableLoader currentLoader = this.loader;
+        if (currentLoader == null) {
+            this.updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.HANDLER_INITIALIZING_ERROR);
             return;
         }
         final GroupedThings groupedThings = this.groupThingsPerPosition();
-        this.loader.setStopCount(groupedThings.getMaxPosition());
+        currentLoader.setStopCount(groupedThings.getMaxPosition());
         final List<TimetableStop> timetableStops = this.loadTimetable();
         if (timetableStops.isEmpty()) {
+            updateThingsToUndefined(groupedThings);
             return;
         }
 
         this.logger.debug("Retrieved {} timetable stops.", timetableStops.size());
         this.updateThings(groupedThings, timetableStops);
     }
 
+    /**
+     * No data was retrieved, so update all channel values to undefined.
+     */
+    private void updateThingsToUndefined(GroupedThings groupedThings) {
+        for (List<Thing> things : groupedThings.thingsPerPosition.values()) {
+            for (Thing thing : things) {
+                updateChannelsToUndefined(thing);
+            }
+        }
+    }
+
+    private void updateChannelsToUndefined(Thing thing) {
+        for (Channel channel : thing.getChannels()) {
+            this.updateState(channel.getUID(), UnDefType.UNDEF);
+        }
+    }
+
     private void updateThings(GroupedThings groupedThings, final List<TimetableStop> timetableStops) {
         int position = 1;
         for (final TimetableStop stop : timetableStops) {
@@ -254,6 +271,17 @@ private void updateThings(GroupedThings groupedThings, final List<TimetableStop>
             }
             position++;
         }
+
+        // Update all things to undefined, for which no data was received.
+        while (position <= groupedThings.getMaxPosition()) {
+            final List<Thing> thingsAtPosition = groupedThings.getThingsAtPosition(position);
+            if (thingsAtPosition != null) {
+                for (Thing thing : thingsAtPosition) {
+                    updateChannelsToUndefined(thing);
+                }
+            }
+            position++;
+        }
     }
 
     /**

diff --git a/...ahn/src/main/java/org/openhab/binding/deutschebahn/internal/DeutscheBahnTrainHandler.java b/...ahn/src/main/java/org/openhab/binding/deutschebahn/internal/DeutscheBahnTrainHandler.java
@@ -102,7 +102,7 @@ public void initialize() {
         this.updateStatus(ThingStatus.UNKNOWN);
 
         if (this.getBridge() == null) {
-            this.updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR);
+            this.updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, "Please select bridge");
             return;
         }
 

diff --git a/bundles/org.openhab.binding.deutschebahn/src/main/resources/OH-INF/binding/binding.xml b/bundles/org.openhab.binding.deutschebahn/src/main/resources/OH-INF/binding/binding.xml
@@ -3,7 +3,7 @@
 	xmlns:binding="https://openhab.org/schemas/binding/v1.0.0"
 	xsi:schemaLocation="https://openhab.org/schemas/binding/v1.0.0 https://openhab.org/schemas/binding-1.0.0.xsd">
 
-	<name>DeutscheBahn Binding</name>
+	<name>Deutsche Bahn Binding</name>
 	<description>This binding provides timetable information for train stations of Deutsche Bahn.</description>
 
 </binding:binding>
diff --git a/bundles/org.openhab.binding.deutschebahn/src/main/resources/OH-INF/thing/thing-types.xml b/bundles/org.openhab.binding.deutschebahn/src/main/resources/OH-INF/thing/thing-types.xml
@@ -6,24 +6,24 @@
 
 	<bridge-type id="timetable">
 
-		<label>DeutscheBahn Timetable</label>
-		<description>Connection to the timetable API of DeutscheBahn. Provides timetable data that can be displayed using the
+		<label>Deutsche Bahn Timetable</label>
+		<description>Connection to the timetable API of Deutsche Bahn. Provides timetable data that can be displayed using the
 			train things.</description>
 
 		<config-description>
 			<parameter name="accessToken" type="text" required="true">
 				<label>Access Token</label>
-				<description>Access Token from DeutscheBahn developer portal for accessing the webservice api.</description>
+				<description>Access Token from Deutsche Bahn developer portal for accessing the webservice api.</description>
 			</parameter>
 			<parameter name="evaNo" type="text" required="true" pattern="80\d{5,5}">
-				<label>evaNo of Station</label>
+				<label>EvaNo of Station</label>
 				<description>evaNo of the station, for which the timetable should be requested. see
 					https://data.deutschebahn.com/dataset.tags.EVA-Nr..html</description>
 			</parameter>
 			<parameter name="trainFilter" type="text" readOnly="false">
 				<advanced>true</advanced>
 				<default>departures</default>
-				<label>train filter</label>
+				<label>Train Filter</label>
 				<description>Selects the trains that will be be displayed in this timetable. If not set only departures will be
 					provided.</description>
 				<options>
@@ -127,14 +127,14 @@
 
 	<channel-type id="filter-flags" advanced="true">
 		<item-type>String</item-type>
-		<label>Filter flags</label>
+		<label>Filter Flags</label>
 		<description>Provides the filter flags.</description>
 		<state readOnly="true"/>
 	</channel-type>
 
 	<channel-type id="trip-type" advanced="true">
 		<item-type>String</item-type>
-		<label>Trip type</label>
+		<label>Trip Type</label>
 		<description>Provides the type of the trip.</description>
 		<state readOnly="true">
 			<options>
@@ -265,8 +265,9 @@
 	<channel-type id="hidden" advanced="true">
 		<item-type>Switch</item-type>
 		<label>Hidden</label>
-		<description>On if the event should not be shown on WBT because travellers are not supposed to enter or exit the train
-			at this stop.</description>
+		<description>On if the event should not be shown, because travellers are not supposed to enter or exit the train
+			at
+			this stop.</description>
 		<state readOnly="true"/>
 	</channel-type>
 
@@ -287,52 +288,52 @@
 
 	<channel-type id="planned-distant-endpoint" advanced="true">
 		<item-type>String</item-type>
-		<label>Planned distant endpoint</label>
+		<label>Planned Distant Endpoint</label>
 		<description>Planned distant endpoint.</description>
 		<state readOnly="true"/>
 	</channel-type>
 
 	<channel-type id="changed-distant-endpoint" advanced="true">
 		<item-type>String</item-type>
-		<label>Changed distant endpoint</label>
+		<label>Changed Distant Endpoint</label>
 		<description>Changed distant endpoint.</description>
 		<state readOnly="true"/>
 	</channel-type>
 
 	<channel-type id="distant-change" advanced="true">
 		<item-type>Number</item-type>
-		<label>Distant change</label>
+		<label>Distant Change</label>
 		<description>distant change</description>
 		<state readOnly="true"/>
 	</channel-type>
 
 	<!-- Channels with derived values from other channels -->
 	<channel-type id="planned-final-station">
 		<item-type>String</item-type>
-		<label>Planned final Station</label>
+		<label>Planned Final Station</label>
 		<description>Planned final station of the train. For arrivals the starting station is returned, for departures the
 			target station is returned.</description>
 		<state readOnly="true"/>
 	</channel-type>
 	<channel-type id="planned-intermediate-stations">
 		<item-type>String</item-type>
-		<label>Planned intermediate Stations</label>
+		<label>Planned Intermediate Stations</label>
 		<description>Returns the planned stations this train came from (for arrivals) or the stations this train will go to
 			(for departures). Stations will be separated by single dash.</description>
 		<state readOnly="true"/>
 	</channel-type>
 
 	<channel-type id="changed-final-station">
 		<item-type>String</item-type>
-		<label>Changed final Station</label>
+		<label>Changed Final Station</label>
 		<description>Changed final station of the train. For arrivals the starting station is returned, for departures the
 			target station is returned.</description>
 		<state readOnly="true"/>
 	</channel-type>
 
 	<channel-type id="changed-intermediate-stations">
 		<item-type>String</item-type>
-		<label>Changed intermediate Stations</label>
+		<label>Changed Intermediate Stations</label>
 		<description>Returns the changed stations this train came from (for arrivals) or the stations this train will go to
 			(for departures). Stations will be separated by single dash.</description>
 		<state readOnly="true"/>