[vdr] Initial contribution #9947

MatthiasKlocke · 2021-01-24T17:02:31Z

This is the initial contibution of the VDR binding as announced in #9931.

A VDR binding was already part of openHAB 1 but until now nobody converted it to openHAB 3.
Therefore I created a new binding more or less from scratch that supports the old binding's functionality.
As there are still some users of the old vdr binding I decided to open a pull request so the binding could be included in the official openHAB distribution.

There has been a community discussion about it here: Vdr binding

The Video Disk Recorder (VDR) binding allows openHAB to control your own Video Disk Recorder.

The binding is based on VDR's own SVDRP (Simple VDR Protocol) connectivity. It supports remote control like changing volume and channels as well as sending key commands to your VDR. Also current and next EPG event data is available.
All supported functionality can be found in the README file.

The old binding was using an external library (svdrp4j) to connect to VDR. For the new binding I decided to write my own thin client implementation (package org.openhab.binding.vdr.internal.svdrp) for SVDRP connection as the external library is not available in public maven repositories and for the binding only specific functionality was needed.

To use the binding a VDR instance is needed that the binding can connect to with VDR's SVDRP protocol.

Binding Build:
org.openhab.binding.vdr-3.1.0-SNAPSHOT.jar

Signed-off-by: Matthias Klocke <[email protected]>

corrected formatting and comment issues Signed-off-by: Matthias Klocke <[email protected]>

comment and formatting issues Signed-off-by: Matthias Klocke <[email protected]>

Unit Tests comparing Start and End Dates of EPG events did not reflect TimeZones. That resulted in failed tests when tests where executed on Machines that did not run in local time zone but in UTC. Unit Tests were corrected so that the desired values to check against were enhanced by a time zone. Signed-off-by: Matthias Klocke <[email protected]>

fwolter

Thanks for your contribution! I reviewed your code and here is my feedback.

fwolter · 2021-02-10T18:02:38Z

bundles/org.openhab.binding.vdr/README.md

+
+## Supported Things
+
+The binding provides only one thing type: VDR. You can create one thing for each VDR instance at your home.


Can you mention the Thing Type ID?

bundles/org.openhab.binding.vdr/README.md

fwolter · 2021-02-10T18:07:43Z

bundles/org.openhab.binding.vdr/README.md

+| currentEventSubTitle | String   | Current EPG Event Sub Title             |
+| currentEventBegin    | DateTime | Current EPG Event Begin                 |
+| currentEventEnd      | DateTime | Current EPG Event End                   |
+| currentEventDuration | Number   | Current EPG Event Duration in Minutes   |


You could make use of Units of Measure and make the type Number:Time. See https://www.openhab.org/docs/concepts/units-of-measurement.html

Same for nextEventDuration.

fwolter · 2021-02-10T18:10:40Z

bundles/org.openhab.binding.vdr/src/main/java/org/openhab/binding/vdr/internal/VDRHandler.java

+                                this.getThing().getUID(), channelUID, e.getMessage());
+                    }
+                });
+                updateStatus(ThingStatus.OFFLINE);


You could set the message your are logging here as the status detail message to display it in the UI. Then, you can remove the logging, as the message is also logged by the framework.

Thanks, I thought about it and removed the logging. The code is reached when the thing is really offline and there's nothing wrong so it is not needed to give it a status detail message.

It's good practice to set a text message anyway, as you might troubleshoot why the Thing is offline. It could also be an error in name resolution or a network failure. The status detail message is only displayed if you click on the Thing and not in the overview. So, don't be afraid of polluting the UI with detailed messages.

fwolter · 2021-02-10T18:11:01Z

bundles/org.openhab.binding.vdr/src/main/java/org/openhab/binding/vdr/internal/VDRHandler.java

+                        if ("power".equals(channelUID.getIdWithoutGroup())) {
+                            updateState(channelUID, OnOffType.OFF);
+                        }
+                    } catch (Exception e) {


What type of exception do you expect here? Can you specify the concrete type?

Removed try and catch as it was not necessary.

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

fwolter · 2021-02-10T18:34:32Z

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

+	<channel-type id="vdrPower">
+		<item-type>Switch</item-type>
+		<label>Power State</label>
+		<description>Power State (to switch off VDR)</description>
+		<state readOnly="false"/>
+	</channel-type>


There are predefined channel types you can use. See https://www.openhab.org/docs/developer/bindings/thing-xml.html#system-state-channel-types This could replaced by system.power.

fwolter · 2021-02-10T18:35:41Z

...org.openhab.binding.vdr/src/test/java/org/openhab/binding/vdr/internal/SVDRPChannelTest.java

+@NonNullByDefault
+public class SVDRPChannelTest {
+
+    private final String channelResponseOk = "3 WDR HD Bielefeld";


Does this channel even exist? :)

You got me here. It's all part of a big conspiracy ;-)

Signed-off-by: Matthias Klocke <[email protected]> Co-authored-by: Fabian Wolter <[email protected]> Signed-off-by: Matthias Klocke <[email protected]>

Signed-off-by: Matthias Klocke <[email protected]>

fwolter · 2021-02-16T10:34:04Z

bundles/org.openhab.binding.vdr/src/main/java/org/openhab/binding/vdr/internal/VDRHandler.java

+                                this.getThing().getUID(), channelUID, e.getMessage());
+                    }
+                });
+                updateStatus(ThingStatus.OFFLINE);


It's good practice to set a text message anyway, as you might troubleshoot why the Thing is offline. It could also be an error in name resolution or a network failure. The status detail message is only displayed if you click on the Thing and not in the overview. So, don't be afraid of polluting the UI with detailed messages.

fwolter · 2021-02-16T11:12:12Z

bundles/org.openhab.binding.vdr/src/main/java/org/openhab/binding/vdr/internal/VDRHandler.java

+                            result = new DateTimeType(LocalDateTime
+                                    .ofInstant(entry.getBegin().toInstant(), timeZoneProvider.getTimeZone())
+                                    .atZone(TimeZone.getDefault().toZoneId()));


The semantic of your code is "take the VDR time and treat it as it was the timezone configured within OH and convert it to the system default time". But what you want is to convert it to the timezone configured within OH.

This suggestion assumes that the time, VDR reports, has the same timezone as the operating system OH runs on.

Suggested change

result = new DateTimeType(LocalDateTime

.ofInstant(entry.getBegin().toInstant(), timeZoneProvider.getTimeZone())

.atZone(TimeZone.getDefault().toZoneId()));

result = new DateTimeType(LocalDateTime

.ofInstant(entry.getBegin().toInstant(), ZoneId.systemDefault())

.atZone(timeZoneProvider.getTimeZone()));

Thanks, removed all old java date time api.

fwolter · 2021-02-16T11:14:37Z

...penhab.binding.vdr/src/main/java/org/openhab/binding/vdr/internal/svdrp/SVDRPClientImpl.java

+     */
+    @Override
+    public void openConnection() throws SVDRPConnectionException, SVDRPParseResponseException {
+        @Nullable


The compiler is more intelligent on local variables. Therefore, the Nullable could be removed. Please check all.

fwolter · 2021-02-16T11:21:42Z

....openhab.binding.vdr/src/main/java/org/openhab/binding/vdr/internal/svdrp/SVDRPEpgEvent.java

+                            long begin = Long.parseLong(lt.nextToken());
+                            begin *= 1000L;
+                            entry.setBegin(new Date(begin));


You could directly use Instant instead of converting it to Date and then to Instant.

Suggested change

long begin = Long.parseLong(lt.nextToken());

begin *= 1000L;

entry.setBegin(new Date(begin));

long begin = Long.parseLong(lt.nextToken())

entry.setBegin(Instant.ofEpochSecond(begin));

fwolter · 2021-02-16T11:27:12Z

....openhab.binding.vdr/src/main/java/org/openhab/binding/vdr/internal/svdrp/SVDRPEpgEvent.java

+                        Calendar calendar = Calendar.getInstance();
+                        calendar.setTime(entry.getBegin());
+                        calendar.add(Calendar.MINUTE, entry.getDuration());
+                        entry.setEnd(calendar.getTime());


Then, you also get rid of the old Calendar API.

Suggested change

Calendar calendar = Calendar.getInstance();

calendar.setTime(entry.getBegin());

calendar.add(Calendar.MINUTE, entry.getDuration());

entry.setEnd(calendar.getTime());

entry.setEnd(entry.getBegin().plus(entry.getDuration(), ChronoUnit.MINUTES));

fwolter · 2021-02-16T11:29:04Z

bundles/org.openhab.binding.vdr/src/main/resources/OH-INF/binding/binding.xml

+	xsi:schemaLocation="https://openhab.org/schemas/binding/v1.0.0 https://openhab.org/schemas/binding-1.0.0.xsd">
+
+	<name>VDR Binding</name>
+	<description>The Video Disk Recorder (VDR) binding allows openHAB to control your own Video Disk Recorder


Suggested change

<description>The Video Disk Recorder (VDR) binding allows openHAB to control your own Video Disk Recorder

<description>The Video Disk Recorder (VDR) binding allows to control your own Video Disk Recorder

fwolter · 2021-02-16T11:29:50Z

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

+	</channel-type>
+	<channel-type id="vdrEventDuration">
+		<item-type>Number:Time</item-type>
+		<label>Event Duration </label>


Suggested change

<label>Event Duration </label>

<label>Event Duration</label>

better thing status handling, removed old java date/time api, formatting issues Signed-off-by: MatthiasKlocke <[email protected]> Signed-off-by: Matthias Klocke <[email protected]>

fwolter · 2021-02-18T08:36:07Z

bundles/org.openhab.binding.vdr/src/main/java/org/openhab/binding/vdr/internal/VDRHandler.java

-                updateStatus(ThingStatus.OFFLINE);
+                updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.NONE, ce.getMessage());
+            } catch (SVDRPParseResponseException se) {
+                updateStatus(ThingStatus.UNKNOWN, ThingStatusDetail.COMMUNICATION_ERROR, se.getMessage());


The UNKNOWN status has no status detail nor message. You could make this OFFLINE, too.

Please see my comment below for the other issue.

fwolter · 2021-02-18T08:36:27Z

bundles/org.openhab.binding.vdr/src/main/java/org/openhab/binding/vdr/internal/VDRHandler.java

                // also update power channel when thing is offline when initializing
                thing.getChannels().stream().map(c -> c.getUID()).filter(this::isLinked).forEach(channelUID -> {
                    if (VDRBindingConstants.CHANNEL_UID_POWER.equals(channelUID.getIdWithoutGroup())) {
                        updateState(channelUID, OnOffType.OFF);
                    }
                });
-                updateStatus(ThingStatus.OFFLINE);
+                updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.NONE, ce.getMessage());


This seems rather ThingStatusDetail.COMMUNICATION_ERROR?

My intention was that this is normal offline state, that is not critical or an error. If the VDR cannot be reached because it is turned off I did not want to show it in Error State but as suggested by you with a message on detail page.
.
If there is a problem (which is represented by SVDRPParseResponseException) then I set ThingStatus to unknown because of a communication error and I don't really know if it's really online or offline. If this is the case then ThingStatus on thing overview page is also displayed as communication error.

As a VDR is not running 24x7 for many people it's status will be offline regularly because it cannot be reached. So there's no action for the openhab user to do. If there is a problem in communication (and it's reachable) then currently Communication Error is displayed also in Thing list in openHAB.

If you say that "Communication Error" is the normal state for things that cannot be reached (also in other bindings) then I will change ThingStatusDetail to communication error (also it feels a little wrong for me).

I understand your point. But, indeed, other bindings set the Thing status OFFLINE, when their Thing is not reachable. Whether this is supposed to happen on a regular basis or not, doesn't play any role. In fact it is a communication error if the Thing is not reachable, for whatever reason.

The UNKNOWN status should only be set during initialization. See the definition https://www.openhab.org/docs/concepts/things.html#thing-status

Thanks for your explanation, Fabian. I now set the communication error in both cases. And thanks for your patience.
Just one more question: I now set the combination of Status Offline and StatusDetail Communication Error. that was what you meant, or did you also mean to set status to Online in error case?

set thing status to communication error when communication with thing goes wrong. Signed-off-by: Matthias Klocke <[email protected]>

fwolter

LGTM

As this is a new binding, another maintainer needs to review your code before it can be merged.

cpmeister · 2021-02-25T23:40:16Z

bundles/org.openhab.binding.vdr/src/main/java/org/openhab/binding/vdr/internal/VDRHandler.java

+        updateStatus(ThingStatus.UNKNOWN);
+
+        // background initialization:
+        scheduler.execute(() -> {


Any reason that you couldn't just execute scheduleRefreshThread() instead of this asynchronous process?

Thanks for your advice!
The scheduleRefreshThread did almost everything as the coding in initaliziation. So I changed to your suggestion, but did not yet commit as I have one question about it.

As I interpret the diagram (Things | openhab) the openHAB thing lifecycle allows transitions from initialized back to uninitialized and the back to initialized. So I'd like to keep the stopRefreshThread call in line 93 to ensure that there is no existing refresh thread still running. Would that be OK or is there a better alternative I have overlooked?

There is no harm in calling stopRefreshThread first if you think it would be safer, although I don't really think it is required.

Thanks!
I left the line in for now but will do some long-term analysis on my local machine with extra debugging to see if and when it's called.

Hello @cpmeister !
I have removed the stopRefreshThread call in initialization now as suggested by you. In my local environment with debug output the method's content was never called in the last month. Therefore I now also think the call is not necessary.
Apart from that I solved a little bug in the SVDRP client.

Can I do anything else for you?

Consolidated onVDRRefresh and initialize methods Signed-off-by: Matthias Klocke <[email protected]>

corrected Channel label for Next EPG Event End Signed-off-by: Matthias Klocke <[email protected]>

Removed stopRefreshThread in initialization method. Optimized SVDRP Parsing for volume in case VDR is muted Signed-off-by: Matthias Klocke <[email protected]>

fwolter

If I see correctly, @cpmeister's comment is addressed.

Due to lack of review capacity, the rules for new bindings were weakened. New bindings need only the approval of one reviewer instead of two from now on.

Signed-off-by: Matthias Klocke <[email protected]> Signed-off-by: John Marshall <[email protected]>

Signed-off-by: Matthias Klocke <[email protected]>

MatthiasKlocke added 3 commits January 23, 2021 22:53

[vdr] Initial contribution of the VDR binding

3126d74

Signed-off-by: Matthias Klocke <[email protected]>

[vdr] Initial contribution of the VDR binding

d1f2cca

corrected formatting and comment issues Signed-off-by: Matthias Klocke <[email protected]>

[vdr] Initial contribution of the VDR binding

4c17761

comment and formatting issues Signed-off-by: Matthias Klocke <[email protected]>

MatthiasKlocke requested a review from a team as a code owner January 24, 2021 17:02

cpmeister added new binding If someone has started to work on a binding. For a new binding PR. oh1 migration Relates to migrating an openHAB 1 addon to openHAB 2 labels Jan 24, 2021

Hilbrand mentioned this pull request Jan 24, 2021

openHAB 1 addon compatibility overview with openHAB 3 #6179

Closed

fwolter self-assigned this Feb 10, 2021

fwolter requested changes Feb 10, 2021

View reviewed changes

MatthiasKlocke and others added 2 commits February 13, 2021 11:26

Apply suggestions from code review

c55de7a

Signed-off-by: Matthias Klocke <[email protected]> Co-authored-by: Fabian Wolter <[email protected]> Signed-off-by: Matthias Klocke <[email protected]>

Correction and Cleanup of Code based on Review

13a2b52

Signed-off-by: Matthias Klocke <[email protected]>

MatthiasKlocke force-pushed the 9931-vdr-binding-initial-contribution branch from 90602c4 to 13a2b52 Compare February 13, 2021 10:30

fwolter requested changes Feb 16, 2021

View reviewed changes

Apply suggestions from code review

6968a91

better thing status handling, removed old java date/time api, formatting issues Signed-off-by: MatthiasKlocke <[email protected]> Signed-off-by: Matthias Klocke <[email protected]>

MatthiasKlocke force-pushed the 9931-vdr-binding-initial-contribution branch from d0f94ce to 6968a91 Compare February 17, 2021 22:27

fwolter reviewed Feb 18, 2021

View reviewed changes

Apply Suggestions from code review

07f7168

set thing status to communication error when communication with thing goes wrong. Signed-off-by: Matthias Klocke <[email protected]>

fwolter approved these changes Feb 18, 2021

View reviewed changes

fwolter removed their assignment Feb 18, 2021

fwolter added the cre Coordinated Review Effort label Feb 18, 2021

cpmeister requested changes Feb 25, 2021

View reviewed changes

cpmeister self-assigned this Feb 25, 2021

MatthiasKlocke added 3 commits February 27, 2021 22:13

Code Review Suggestion - harmonize initialize and refresh methods

f79e051

Consolidated onVDRRefresh and initialize methods Signed-off-by: Matthias Klocke <[email protected]>

Fix EPG Event label description

3f8a9cd

corrected Channel label for Next EPG Event End Signed-off-by: Matthias Klocke <[email protected]>

Code Suggestion - remove stopRefreshThread in initialization method

5119248

Removed stopRefreshThread in initialization method. Optimized SVDRP Parsing for volume in case VDR is muted Signed-off-by: Matthias Klocke <[email protected]>

fwolter approved these changes Apr 11, 2021

View reviewed changes

fwolter merged commit 5d5cd78 into openhab:main Apr 11, 2021

fwolter changed the title ~~[vdr] New binding: replacement for openhab1 vdr binding~~ [vdr] Initial contribution Apr 11, 2021

fwolter removed the cre Coordinated Review Effort label Apr 11, 2021

fwolter added this to the 3.1 milestone Apr 11, 2021

themillhousegroup pushed a commit to themillhousegroup/openhab2-addons that referenced this pull request May 10, 2021

[vdr] Initial contribution (openhab#9947)

e675597

Signed-off-by: Matthias Klocke <[email protected]> Signed-off-by: John Marshall <[email protected]>

computergeek1507 pushed a commit to computergeek1507/openhab-addons that referenced this pull request Jul 13, 2021

[vdr] Initial contribution (openhab#9947)

4cfe6c6

Signed-off-by: Matthias Klocke <[email protected]>

thinkingstone pushed a commit to thinkingstone/openhab-addons that referenced this pull request Nov 7, 2021

[vdr] Initial contribution (openhab#9947)

c433639

Signed-off-by: Matthias Klocke <[email protected]>

marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022

[vdr] Initial contribution (openhab#9947)

15b9494

Signed-off-by: Matthias Klocke <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[vdr] Initial contribution #9947

[vdr] Initial contribution #9947

MatthiasKlocke commented Jan 24, 2021 •

edited

Loading

fwolter left a comment

fwolter Feb 10, 2021

fwolter Feb 10, 2021

fwolter Feb 10, 2021

MatthiasKlocke Feb 13, 2021

fwolter Feb 16, 2021

fwolter Feb 10, 2021

MatthiasKlocke Feb 13, 2021

fwolter Feb 10, 2021

fwolter Feb 10, 2021

MatthiasKlocke Feb 13, 2021

fwolter Feb 16, 2021

fwolter Feb 16, 2021

MatthiasKlocke Feb 17, 2021

fwolter Feb 16, 2021

fwolter Feb 16, 2021

fwolter Feb 16, 2021

fwolter Feb 16, 2021

fwolter Feb 16, 2021

fwolter Feb 18, 2021

MatthiasKlocke Feb 18, 2021

fwolter Feb 18, 2021

MatthiasKlocke Feb 18, 2021

fwolter Feb 18, 2021

MatthiasKlocke Feb 18, 2021 •

edited

Loading

fwolter left a comment

cpmeister Feb 25, 2021

MatthiasKlocke Feb 27, 2021

cpmeister Feb 27, 2021

MatthiasKlocke Feb 27, 2021

MatthiasKlocke Apr 5, 2021

fwolter left a comment


		## Supported Things

		The binding provides only one thing type: VDR. You can create one thing for each VDR instance at your home.

	<description>The Video Disk Recorder (VDR) binding allows openHAB to control your own Video Disk Recorder
	<description>The Video Disk Recorder (VDR) binding allows to control your own Video Disk Recorder

[vdr] Initial contribution #9947

[vdr] Initial contribution #9947

Conversation

MatthiasKlocke commented Jan 24, 2021 • edited Loading

fwolter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthiasKlocke Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

fwolter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fwolter left a comment

Choose a reason for hiding this comment

MatthiasKlocke commented Jan 24, 2021 •

edited

Loading

MatthiasKlocke Feb 18, 2021 •

edited

Loading