[denonmarantz] new Denon / Marantz 2.0 binding #2859

jwveldhuis · 2017-11-19T22:04:35Z

This is a OpenHAB 2 port of the existing 1.x Denon binding (created by @idserda). As the binding supports both Denon and Marantz receivers decided to change the name to 'denonmarantz'.

Improvements:

support for Discovery (MDNS)
defined channels (including an 'advanced' Channel to send any custom command)
improved Telnet stability (although that has been addressed in the old binding)

Note that this binding is still work in progress. Need others to test this binding as I only own one Marantz receiver (SR5008) myself. That is the main reason for this PR, as I know some cleanup, fixing and documentation is required.

This is my first binding, and it took more time and effort than I'd hoped for.. ;)
Happy to learn how to further improve.

jwveldhuis · 2017-11-19T22:11:47Z

Not sure why Jenkins / Travis builds failed? Something I can do about that?

Update nevermind, saw that the line in Travis tailing the stderr log is expandable ;)
Now saw this:

[ERROR] [ERROR] Some problems were encountered while processing the POMs:
[FATAL] Non-resolvable parent POM for org.openhab.binding:org.openhab.binding.denonmarantz:2.1.0->SNAPSHOT: Could not find artifact > org.openhab.binding:pom:pom:2.1.0-SNAPSHOT in sonatype-snapshots (https://oss.sonatype.org/content
/repositories/snapshots/) and 'parent.relativePath' points at wrong local POM @ line 6, column 11

Will check what I can do about this, any suggestions are welcomed ;)

jwveldhuis · 2017-11-20T15:11:09Z

Build issues fixed, found the jar file of the binding so other can test now.

jwveldhuis · 2017-12-30T17:05:41Z

Testing by several users has been done over the past weeks.
I need to add a README.md.

jwveldhuis · 2018-01-01T11:15:40Z

@martinvw ready for review!
Just squashed all commits and added "Signed-off-by".

jwveldhuis · 2018-01-03T14:22:10Z

Binding is made available at the Eclipse Marketplace: https://marketplace.eclipse.org/content/denonmarantz-binding

triller-telekom

Thank you for porting this binding to openHAB2!

I did a first review and left some comments inside the code.

One major thing I would propose is that you should think about splitting your connector into one that is responsible for telnet and one that is responsible for http.

triller-telekom · 2018-01-19T10:17:05Z

addons/binding/org.openhab.binding.denonmarantz/ESH-INF/thing/thing-types.xml

+              <advanced>true</advanced>
+            </parameter-group>
+
+            <parameter name="zoneCount" type="integer" required="true" groupName="receiverProperties">


If you provide a default value you can omit the required=true

triller-telekom · 2018-01-19T10:17:38Z

addons/binding/org.openhab.binding.denonmarantz/ESH-INF/thing/thing-types.xml

+                <description>Hostname or IP address of the AVR to control.</description>
+            </parameter>
+
+            <parameter name="telnetEnabled" type="boolean" required="true" groupName="connectionSettings">


If you provide a default value you can omit the required=true

triller-telekom · 2018-01-19T10:18:32Z

addons/binding/org.openhab.binding.denonmarantz/ESH-INF/thing/thing-types.xml

+                <default>false</default>
+            </parameter>
+
+            <parameter name="telnetPort" type="integer" required="false" groupName="telnetSettings">


required=false is the default so you can remove it.

triller-telekom · 2018-01-19T10:18:55Z

addons/binding/org.openhab.binding.denonmarantz/ESH-INF/thing/thing-types.xml

+                <advanced>true</advanced>
+            </parameter>
+
+            <parameter name="httpPort" type="integer" required="true" groupName="httpSettings">


If you provide a default value you can omit the required=true

triller-telekom · 2018-01-19T10:19:09Z

addons/binding/org.openhab.binding.denonmarantz/ESH-INF/thing/thing-types.xml

+                <advanced>true</advanced>
+            </parameter>
+
+            <parameter name="httpPollingInterval" type="integer" required="true" groupName="httpSettings">


If you provide a default value you can omit the required=true

triller-telekom · 2018-01-19T13:27:58Z

...enonmarantz/src/main/java/org/openhab/binding/denonmarantz/DenonMarantzBindingConstants.java

@@ -0,0 +1,86 @@
+/**
+ * Copyright (c) 2010-2017 by the respective copyright holders.


Please run mvn license:format to update it to 2018 for all files (some of yours even have 2015 :))

triller-telekom · 2018-01-19T13:29:16Z

...main/java/org/openhab/binding/denonmarantz/internal/communication/adapters/OnOffAdapter.java

+    @Override
+    public Boolean unmarshal(String v) throws Exception {
+        if (v != null) {
+            return v.toLowerCase().equals("on");


Doesn't this return a "primitive" boolean instead of the Boolean object?

triller-telekom · 2018-01-19T14:28:50Z

...va/org/openhab/binding/denonmarantz/internal/discovery/DenonMarantzDiscoveryParticipant.java

+                    telnetEnable = false;
+                    logger.debug("We can access the HTTP API, disabling the Telnet mode by default");
+                }
+            } catch (Exception e) {


See above: Please do not catch Exception

See above, please suggest a solution for this.

triller-telekom · 2018-01-19T14:33:37Z

...va/org/openhab/binding/denonmarantz/internal/discovery/DenonMarantzDiscoveryParticipant.java

+        Matcher matcher = DENON_MARANTZ_PATTERN.matcher(service.getQualifiedName());
+        if (matcher.matches()) {
+            logger.debug("This seems like a supported Denon/Marantz AVR!");
+            this.serial = matcher.group(1).toLowerCase();


What if you detect more than one? By storing the serial inside a class variable you will overwrite it in case there are 2 or more results.

Ahh, didn't realize that! Thought the Participant would get instantiated for reach result, which obviously isn't the case. Good catch, this could have resulted in a hard to debug issue.

triller-telekom · 2018-01-19T14:34:48Z

...src/main/java/org/openhab/binding/denonmarantz/internal/telnet/DenonMarantzTelnetClient.java

+ * @author Jeroen Idserda
+ * @author Jan-Willem Veldhuis
+ */
+public class DenonMarantzTelnetClient extends Thread {


Please make it a Runnable and schedule it with the scheduler (scheduler.submit..)

jwveldhuis · 2018-01-26T12:22:47Z

@triller-telekom thank you for reviewing! I need to find time to work on your feedback.

jwveldhuis · 2018-02-01T09:48:51Z

Note to self: need to fix issue in 2.3.0 caused by this Smarthome PR: #4787 and fixed here: #2997
(I used the Yamaha binding as a reference for adding Channels dynamically)

Edit: took care of this.. Added static mapping from ChannelType to ItemType to avoid a lot of complex code to fetch those from the thing-types.xml file. Can be refactored later on when smarthome is enhanced with eclipse-archived/smarthome#4950

triller-telekom · 2018-02-02T10:07:45Z

@jwveldhuis Thank you for your comments. If you have any updates on the code side please create a NEW commit on your branch and push it. Please do NOT amend your previous commit and do a force push. Reviewing the code is a lot easier if we only see the diff to the previous reviews. Thanks.

jwveldhuis · 2018-02-03T16:28:46Z

@triller-telekom how do I handle the merge with upstream/master in between, should that be 'signed-off' as well?
Sorry I'm not too familiar with git / github.

Update: Nevermind, I've reverted the branch back to previous commit and forced push a new commit. Looks like you don't need to sync the fork with master.
Still curious how the git workflow should work, as I want to test locally against the latest changes on upstream/master.

Looks like this could work:

# merge the current master in local checkout of fork, don't commit
git merge upstream/master --no-commit

# do dev work / testing

# abort the merge 
git merge --abort 

# now commit and push to fork..

triller-telekom

Thank you for your changes. the code looks much cleaner now with the separation of the http and telnet clients.

If you want to include the current status of the master branch in your feature branch, please rebase your branch onto the current master, never to a merge. Easiest thing to do that is to change tot he master branch and pull it: git checkout master; git pull and afterwards go into your branch git checkout YOUR_BRANCH; git rebase master.

This will replay all your changes on top of the current master to keep a clean and straight version history without any merge commits.

I have left some minor comments in the code.

triller-telekom · 2018-02-05T13:40:35Z

addons/binding/org.openhab.binding.denonmarantz/META-INF/MANIFEST.MF

 org.openhab.binding.denonmarantz,
 org.openhab.binding.denonmarantz.handler,
+ org.osgi.service.component.annotations;resolution:=optional,


You can remove this optional import.

When I removed it I get this error for the Handler:
The type org.eclipse.jdt.annotation.NonNullByDefault cannot be resolved. It is indirectly referenced from required .class files

Are you sure that you removed org.osgi.service.component.annotations;resolution:=optional, and NOT accidentally org.eclipse.jdt.annotation;resolution:=optional,? Because the later one would explain your error message...

Mea culpa, yes I did.
However, when I remove the org.osgi.service.component.annotations;resolution:=optional, I see the following warning in Eclipse:
DS Annotations missing from permanent build path

This is because your branch is too old. I have just tested a plain checkout with the current master and everything is fine in the IDE. if I rebase your branch onto the current master and remove org.osgi.service.component.annotations;resolution:=optional, from the MANIFEST.MF, everything is alright.

So please rebase your branch onto the current master and do a push with --force. Careful though, because this will overwrite the branch on github. So you might want to do a backup of your current local repository before doing so since you said you had problems with git before.

However, I just did a git rebase master in your branch locally (after pulling the latest master branch of course) on my machine and everything went smooth.

I am on the latest commit (commit dc204c6) of openhab2-addons, still see the warning.
Is it perhaps a setting outside of this repo? What was changed to resolve this warning?

I am using openhab2-addons and openhab-distro in my IDE, I don't use the other internal repo's.

Nevertheless, I will apply the change and ignore the local warning (I see them for other bindings as well), perhaps related to my IDE setup.

All of you checked out repos have to be on the newest version, i.e. "openhab1-addons openhab2-addons openhab-core openhab-distro smarthome"

I only have openhab2-addons and openhab-distro and both are up to date (while setting up the IDE I only picked "openHAB Development" (required) and "openHAB2 Add-ons" from the optional set of repos).

Confirmed warning is gone when all git repos are included in the IDE.

triller-telekom · 2018-02-05T13:42:08Z

addons/binding/org.openhab.binding.denonmarantz/README.md

@@ -57,26 +62,30 @@ The DenonMarantz AVR supports the following channels (some channels are model sp
 |  mainVolume       | Dimmer | Main zone volume


Please add (R) or (RW) on all channels to make it consistent

triller-telekom · 2018-02-05T13:54:25Z

...denonmarantz/src/main/java/org/openhab/binding/denonmarantz/handler/DenonMarantzHandler.java

    @Override
    public void initialize() {
-        // create connection (either Telnet & HTTP or HTTP only)
+        config = getConfigAs(DenonMarantzConfiguration.class);
+        denonMarantzState = new DenonMarantzState(this);


Maybe you want to check the configuration before doing anything?

triller-telekom · 2018-02-05T14:04:43Z

....denonmarantz/src/main/java/org/openhab/binding/denonmarantz/internal/DenonMarantzState.java

@@ -58,6 +58,10 @@ public DenonMarantzState(DenonMarantzStateChangedListener listener) {
        this.listener = listener;
    }

+    public void connectionError(String errorMessage) {
+        listener.connectionError(errorMessage);


What if the listener is null?

It can't be. State is instantiated from the DenonMarantzHandler like this, the handler is the (only) listener.

denonMarantzState = new DenonMarantzState(this);

Yes I saw that is is initialized with the handler, but code gets changed over time and who knows if there won't be another instance created somewhere in the future? Just saying this because with the term listener I usually connect that it is a 0-n relationship, i.e. it is optional or there may be many.

I agree, will rename the variable to handler to avoid future confusion.

triller-telekom · 2018-02-05T14:22:58Z

...java/org/openhab/binding/denonmarantz/internal/connector/http/DenonMarantzHttpConnector.java

+
+    private void startPolling() {
+        if (!isPolling()) {
+            logger.info("HTTP polling started.");


Please decrease to debug level.

triller-telekom · 2018-02-05T14:30:06Z

...java/org/openhab/binding/denonmarantz/internal/connector/http/DenonMarantzHttpConnector.java

+    }
+
+    private void updateSecondaryZones() {
+        for (int i = 2; i <= config.getZoneCount(); i++) {


Why are you using a loop at all if you have a switch-case later on anyway?

To reuse the code to construct the url, to get the document and to log about that.

triller-telekom · 2018-02-05T14:32:09Z

...java/org/openhab/binding/denonmarantz/internal/connector/http/DenonMarantzHttpConnector.java

+        String ret = IOUtils.toString(is);
+
+        connection.disconnect();
+


triller-telekom · 2018-02-05T14:50:54Z

...va/org/openhab/binding/denonmarantz/internal/discovery/DenonMarantzDiscoveryParticipant.java

            try {
                httpClient.start();
                response = httpClient.newRequest("http://" + host + "/goform/Deviceinfo.xml")
                        .timeout(3, TimeUnit.SECONDS).send();
-                int status = response.getStatus();
-                if (status == 200) {
+                if (response.getStatus() == 200) {


Isn't there a constant like HTTP_STATUS.OK or something instead of the 200?

triller-telekom · 2018-02-05T14:51:28Z

...va/org/openhab/binding/denonmarantz/internal/discovery/DenonMarantzDiscoveryParticipant.java

+                try {
+                    response = httpClient.newRequest("http://" + host + ":8080/goform/Deviceinfo.xml")
+                            .timeout(3, TimeUnit.SECONDS).send();
+                    if (response.getStatus() == 200) {


triller-telekom · 2018-02-05T14:53:34Z

...va/org/openhab/binding/denonmarantz/internal/discovery/DenonMarantzDiscoveryParticipant.java

+                    logger.debug("Failed in fetching the Deviceinfo.xml to determine zone count: {}", e.getMessage());
+                }
+
+                if (status == 200 && response != null) {


jwveldhuis · 2018-02-05T15:40:41Z

@triller-telekom ahh, thanks for this basic git-HOWTO :) Searched and tried a lot, couldn't find this.
Will work on your latest comments.
And indeed, the code is a lot cleaner now. When I started this I wanted to keep as close as possible to the 1.x binding, now eventually almost everything got changed.. ;)

jwveldhuis · 2018-02-05T21:11:11Z

@triller-telekom what is the recommended pattern to handle RunTimeException in a Runnable scheduled with ScheduledExecutorService?
Learned today that the Runnable just stops and NO error is logged anywhere.. :/
See https://stackoverflow.com/a/24902026/2486310

I am planning to handle it like this:

pollingJob = scheduler.scheduleWithFixedDelay(new Runnable() {
    @Override
    public void run() {
        try {
            refreshHttpProperties();
        } catch (Throwable t) {
            StringBuilder sb = new StringBuilder();
            for (StackTraceElement s : t.getStackTrace()) {
                sb.append(s.toString()).append("\n");
            }
            logger.error("Error while polling Http: \"{}\". Stacktrace: \n{}", t.getMessage(),
                    sb.toString());
        }
    }
}, config.httpPollingInterval, config.httpPollingInterval, TimeUnit.SECONDS);

In this case (see trail of comments in #2204) the following happened:
User assigned Dimmer item and assigned it to a Number channel (not using Paper UI).
At update of the channel state a java.lang.IllegalArgumentException: Value must be between 0 and 100 was thrown. This caused the Runnable to stop.

Took me 4 hours to diagnose this 😕 😦

triller-telekom · 2018-02-06T15:44:13Z

Regarding your RuntimeException in a Runnable problem: We had eclipse-archived/smarthome#4772

But it turned out that some jobs were not executed and thus we had to revert it via eclipse-archived/smarthome#4932

I just tried to play around with it but unfortunately I could not really reproduce it :( Neither in my IDE pure ESH setup nor in a basic installation of OH 2.2 with a patched smarthome.core bundle (one that contained the wrappedscheduler).

So for now I guess you have to stick to catching the RuntimeException on your own. If you found out some more details while you where near the code in your debug session anyway, help from your side is more than welcome :)

triller-telekom · 2018-02-08T12:26:33Z

@jwveldhuis FYI: With eclipse-archived/smarthome#5045 I have fixed the logging of Exceptions in scheduled jobs :)

If you address the few minor issues from above we can handover your PR to the committers for a final review and merge.

jwveldhuis · 2018-02-08T12:44:55Z

@triller-telekom excellent! Thank you!
Need to find time to finish the last issues.
Also started to work on dynamically fetching the Input options (e.g. TV, NET, TUNER). I will postpone that for a follow-up PR to not further delay this.

jwveldhuis · 2018-02-08T12:58:48Z

@triller-telekom does your fix only address the error logging, or does it also keep the scheduler running for subsequent tasks (I don't think so)?
So I think I still need to catch RuntimeException to prevent the scheduler from stopping. Will perform some testing around this.

triller-telekom · 2018-02-08T14:09:12Z

So I think I still need to catch RuntimeException to prevent the scheduler from stopping. Will perform some testing around this.

Yes indeed. My fix is only to make the error show up in the logs. if you already know that a RuntimeException may occur and how to deal with it, then you can catch it. So my fix doesn't change any behavior it just makes a wrong behavior obvious.

jwveldhuis · 2018-02-08T21:43:40Z

Pushed changes.
Again struggled with git. I rebased to master, but then couldn't push:

 ! [rejected]          denon-marantz-binding -> denon-marantz-binding (non-fast-forward)
error: failed to push some refs to 'https://github.com/jwveldhuis/openhab2-addons.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.

In git log I saw my previous commits got a different commit hash due to the git rebase. Would GitHub still recognise the previous commits or would it mess up the code review? I now reverted back to the previous state of my fork and then put the changes on top, then pushed.

triller-telekom

Thank you for your changes. The code looks really good now except for two small things where I have left a comment.

Regarding the git rebase: You noticed correctly that the hash of you commit has changed because it was rewritten onto the master branch. In order to push this rebased version of you branch you have to somehow "force" it via git push --force-with-lease

A git push --force would also be sufficient but in projects where you work with others together on the same branch you could overwrite their work if you are not careful. I just recently learned the lease command and think its a very good thing to have.

A colleague of me has recently shared a nice link to a good introduction for git rebase.

triller-telekom · 2018-02-09T10:39:35Z

...denonmarantz/src/main/java/org/openhab/binding/denonmarantz/handler/DenonMarantzHandler.java

@@ -123,7 +126,7 @@ public void handleCommand(ChannelUID channelUID, Command command) {
                    break;

                default:
-                    logger.warn("Command for channel {} not supported.", channelUID.getId());
+                    logger.warn("Command for (read only) channel {} not supported.", channelUID.getId());


Why did you add the "(read only)" here? You could get a command for an arbitrary channel...

But I am thinking about whether if it would make sense to reduce the log level to debug, because you are not interested in commands for other channels.

triller-telekom · 2018-02-09T10:40:44Z

...src/main/java/org/openhab/binding/denonmarantz/internal/connector/DenonMarantzConnector.java

@@ -12,6 +12,7 @@
 import java.math.RoundingMode;
 import java.util.concurrent.ScheduledExecutorService;

+import org.eclipse.jdt.annotation.NonNull;


Please do not use @NonNull, use @NonNullByDefault on the class and @Nullable where appropriate instead, if you want to make use of the null annotations.

jwveldhuis · 2018-02-09T13:30:20Z

@triller-telekom done.

triller-telekom · 2018-02-09T13:36:31Z

@kaikreuzer @martinvw This PR is now ready for a final review and can be merged afterwards. Please have a final look at it. Thanks.

triller-telekom · 2018-02-09T13:37:41Z

Thank you very much @jwveldhuis !

jwveldhuis · 2018-02-09T13:43:31Z

Thank you for the extensive review! Enjoyed the collaboration!
Once this is merged will come back with some more enhancements ;)

kaikreuzer · 2018-02-13T17:20:17Z

Thanks for this contribution @jwveldhuis!
I just started with doing a final review. What immediately caught my eye is that you do not seem to be familiar with the concept of channel groups - those are actually an ideal fit for the different zones of an AVR, which all have the very same features (= set of channels). You'd therefore only have to define all those channels once and then add three groups for the three zones. Would that make sense?

jwveldhuis · 2018-04-26T10:22:46Z

@kaikreuzer there are many models (Denon and Marantz) and I don't have a clear overview which ones support multiple zones (2 or 3). Also more models come to market over time, by dynamically detecting (or letting the user set) the number of zones we avoid having to update the binding for each new model.

Adding channels dynamically is already done in the current version of this binding.
When I add 'zoneX#volume', how will the framework know that I am referring to a channel-group of type 'zone'? I guess I do need to assign all channel-groups upfront and then remove the unsupported channels later on?

kaikreuzer · 2018-04-26T13:31:14Z

by dynamically detecting the number of zones we avoid having to update the binding

ok, fine.

I guess I do need to assign all channel-groups upfront

As mentioned above: There is no information about channel groups at all in the Thing, so there is no need to create or assign any. All that exists as a reference is the channel id with 'zoneX#volume' format (in constrast to the normal 'volume' channel id).

jwveldhuis · 2018-04-26T15:28:55Z

@kaikreuzer I start to get it now.. Struggling with the code which dynamically adds / removes channels. Was using the ChannelTypeUID as a unique identifier to add/remove channels, this obviously does no longer work. Need to think of an alternative mechanism.

Big drawback of using channel groups is the lack of custom labels / descriptions for each channel.
How to override the labels / descriptions? (I guess that defeats the purpose of channel groups?)
In Paper UI it is becoming messy for users now the custom channels for each zone are replaced by generic ones:

Zone control
- Power (zone)
- Volume
- Mute
- Input source
Zone control
- Power (zone)
- Volume
- Mute
- Input source
Zone control
- Power (zone)
- Volume
- Mute
- Input source

How to distinguish Main Zone from Zone1 and Zone2? I used to have custom labels, e.g. "Power (zone2)", "Mute (zone3)", etc.

@jwveldhuis

Signed-off-by: Jan-Willem Veldhuis <[email protected]> (github: @jwveldhuis)

Signed-off-by: Jan-Willem Veldhuis <[email protected]> (@github: jwveldhuis)

Signed-off-by: Jan-Willem Veldhuis <[email protected]> (github: jwveldhuis)

Signed-off-by: Jan-Willem Veldhuis <[email protected]>

Zone3. Fixed comments according to code analysis report. Signed-off-by: Jan-Willem Veldhuis <[email protected]> (github: jwveldhuis)

jwveldhuis · 2018-04-29T19:03:07Z

Just pushed the updated Binding. Have created separate channel groups in order to provide a custom description for each channel group (while still reusing the common channels).

kaikreuzer · 2018-04-30T14:07:53Z

Sorry for my late reply - there actually is no need to define multiple channel-group-types. For custom labels&descriptions, you can simply overwrite them for each channel group:

<channel-groups>
    <channel-group typeId="zone" id="mainZone">
        <label>Main Zone</label>
        <description>The main zone of this AVR</description>
    </channel-group>
    <channel-group typeId="zone" id="zone2">
	    <label>Zone 2</label>
        <description>Zone 2 of this AVR</description>
    </channel-group>
    ...
</channel-groups>

This should simplify you xmls again a bit (and hopefully not require any code changes at all).

Signed-off-by: Jan-Willem Veldhuis <[email protected]> (github: jwveldhuis)

jwveldhuis · 2018-04-30T14:38:49Z

@kaikreuzer ah thanks, now it all makes sense 😁
Just pushed the suggested change.

kaikreuzer

Thanks! Now that we have settled the modelling, I had a detailed look through the code as well and left a few review comments on it.

Please also note that you should add an "Also-By Jeroen Idserda..." to your commit message as @idserda is a co-author (and by adding him, I assume we also have his approval for this contribution).

kaikreuzer · 2018-05-01T19:34:15Z

...denonmarantz/src/main/java/org/openhab/binding/denonmarantz/handler/DenonMarantzHandler.java

+ */
+public class DenonMarantzHandler extends BaseThingHandler implements DenonMarantzStateChangedListener {
+
+    private Logger logger = LoggerFactory.getLogger(DenonMarantzHandler.class);


add "final" keyword

kaikreuzer · 2018-05-01T19:37:53Z

addons/binding/org.openhab.binding.denonmarantz/ESH-INF/thing/thing-types.xml

+
+        <config-description>
+            <parameter-group name="receiverProperties">
+                <label>Receiver properties</label>


You have a mix of cases in your labels - please follow a single style. Usually, we use capitals in labels, here e.g. "Receiver Properties".

kaikreuzer · 2018-05-01T19:38:07Z

addons/binding/org.openhab.binding.denonmarantz/ESH-INF/thing/thing-types.xml

+            </parameter>
+
+            <parameter name="telnetEnabled" type="boolean" groupName="connectionSettings">
+                <label>Use Telnet port?</label>


remove the question mark

kaikreuzer · 2018-05-01T19:39:57Z

...denonmarantz/src/main/java/org/openhab/binding/denonmarantz/handler/DenonMarantzHandler.java

+        }
+
+        if (connector instanceof DenonMarantzHttpConnector && command instanceof RefreshType) {
+            // Refreshing individual channels isn't supported by the Http connector.


Not ideal, but if the polling is done so frequently (every 5 secs if I see it right), it should be ok.

I know, but the AVR can't handle too many HTTP requests at once. I had to power cycle my AVR during tests when all linked channels got polled during startup.

Sure. Just fyi: We have a expiring cache that is meant for this situation (reducing the number of calls that go out to the device).

kaikreuzer · 2018-05-01T19:42:02Z

...denonmarantz/src/main/java/org/openhab/binding/denonmarantz/handler/DenonMarantzHandler.java

+        denonMarantzState = new DenonMarantzState(this);
+        configureZoneChannels();
+        // create connection (either Telnet or HTTP)
+        // ThingStatus ONLINE/OFFLINE is set when AVR status is known.


How long can that take? If that might be a few seconds (what I assume due to potential HTTP timeouts), you should set the status to UNKNOWN here first.

kaikreuzer · 2018-05-01T20:08:49Z

...a/org/openhab/binding/denonmarantz/internal/connector/telnet/DenonMarantzTelnetListener.java

+    /**
+     *
+     */
+    public void receivedLine(String line);


remove public

kaikreuzer · 2018-05-01T20:08:58Z

...a/org/openhab/binding/denonmarantz/internal/connector/telnet/DenonMarantzTelnetListener.java

+
+    /**
+     * The telnet client has successfully connected to the receiver.
+     */


add JavaDoc for parameter

kaikreuzer · 2018-05-01T20:09:04Z

...a/org/openhab/binding/denonmarantz/internal/connector/telnet/DenonMarantzTelnetListener.java

+    /**
+     * The telnet client has successfully connected to the receiver.
+     */
+    public void telnetClientConnected(boolean connected);


remove public

kaikreuzer · 2018-05-01T20:12:35Z

...va/org/openhab/binding/denonmarantz/internal/discovery/DenonMarantzDiscoveryParticipant.java

+
+            logger.debug("IP Address: {}", host);
+
+            // try a HTTP request to autoconfigure the HTTP / Telnet parameter


This does not look like a good idea - this method is expected to return fast; doing an HTTP call means it will block for potentially a few seconds. Better leave some properties unset in the discovery result.

I agree it's not perfect. For some models HTTP is not an option, so for those I want to set 'Use Telnet' to true in the Thing parameter. The default for this parameter is false.

Should I do this instead?

set some property in the Thing indicating it was created via a Discovery Result (I guess there is no way to determine otherwise?)

if set, auto-detect and alter the value for parameter 'Use Telnet' during Initialization

E.g. I don't want to override the parameter when "explicitly" (there is a default) set by the user during manual Thing creation (for whatever reason).

I found another option, but have trouble implementing this:
Limit the number of times the procedure is invoked to exactly once, by stopping further (relatively expensive) discovery activities as soon as it's clear we already configured or discovered the Thing.
https://www.eclipse.org/smarthome/documentation/development/bindings/discovery-services.html#compare-with-existing-results

I tried to implement the ExtendedDiscoveryService with the MDNSDiscoveryParticipant, this does not work unfortunately. How can I add this callback to the MDNSDiscoveryService? How do I approach this?

@jwveldhuis This is a missing feature which I have just implemented in eclipse-archived/smarthome#5526

@triller-telekom perfect!

I am not sure whether this would be a solution. After all, you still have to do HTTP requests in a method that must return fast. And discovery is supposed to be passive in most cases, it should really just sit there and listen.

Couldn't you simply leave this property unset (=null) and upon handler initialisation, you do the HTTP call and figure out the right value, if it is still null? This would allow you to set the value without bothering the user and still avoid the risk of overwriting a value that the user has specifically set.

Ok, I have now moved this part over the the Handler. Added benefit is the auto configuration for telnet/http and zone count is now also applied to Things configured in text files.

kaikreuzer · 2018-05-01T20:15:36Z

addons/binding/pom.xml

@@ -31,6 +31,7 @@
    <module>org.openhab.binding.coolmasternet</module>
    <module>org.openhab.binding.dlinksmarthome</module>
    <module>org.openhab.binding.dscalarm</module>
+    <module>org.openhab.binding.denonmarantz</module>


please also add the binding to https://github.com/openhab/openhab2-addons/blob/master/features/openhab-addons/src/main/feature/feature.xml

idserda · 2018-05-02T07:00:03Z

Great work! Not sure how much of my original code is still left but you definitely have my approval.

Signed-off-by: Jan-Willem Veldhuis <[email protected]> (github: jwveldhuis) Also-by: Jeroen Idserda (github: idserda)

kaikreuzer

Excellent, thanks for your updates!

@jwveldhuis

* Initial contribution for new Denon-Marantz 2.0 binding Also-by: Jeroen Idserda (github: idserda) Signed-off-by: Jan-Willem Veldhuis <[email protected]> (github: @jwveldhuis)

@jwveldhuis

* Initial contribution for new Denon-Marantz 2.0 binding Also-by: Jeroen Idserda (github: idserda) Signed-off-by: Jan-Willem Veldhuis <[email protected]> (github: @jwveldhuis)

grzegorz914 · 2018-07-11T16:19:48Z

Hi,

I use Your binding wit OH2.4 and AVR-X6300H, all working OK except "surroundProgram", this tag show nothing, connection HTTP or Telnet make no differences. Look on screenshot.

Thanks
Grzegorz

New issue #3724

jwveldhuis · 2018-07-11T17:44:41Z

@grzegorz914 please create a new issue for this. Please don’t add comments to closed issues.

@jwveldhuis

* Initial contribution for new Denon-Marantz 2.0 binding Also-by: Jeroen Idserda (github: idserda) Signed-off-by: Jan-Willem Veldhuis <[email protected]> (github: @jwveldhuis)

* Remove duplications * Better null handling * Use constants * Use default waitForAssert timeout (10s) This may help to get the test more stable (openhab#1089). Signed-off-by: Wouter Born <[email protected]>

jwveldhuis mentioned this pull request Nov 19, 2017

[WIP] Denon/Marantz 2.0 Binding #2204

Closed

martinvw added the new binding If someone has started to work on a binding. For a new binding PR. label Nov 24, 2017

jwveldhuis changed the title ~~[denonmarantz][WIP] new Denon / Marantz 2.0 binding~~ [denonmarantz] new Denon / Marantz 2.0 binding Dec 30, 2017

triller-telekom reviewed Jan 19, 2018

View reviewed changes

sjsf mentioned this pull request Feb 1, 2018

Improve ThingHandler API to allow easy addition of Channels eclipse-archived/smarthome#4950

Closed

triller-telekom reviewed Feb 5, 2018

View reviewed changes

triller-telekom mentioned this pull request Feb 6, 2018

Magic bundle: Added thing that schedules a job to go ONLINE after a delay eclipse-archived/smarthome#5037

Merged

triller-telekom reviewed Feb 9, 2018

View reviewed changes

jwveldhuis added 7 commits April 29, 2018 20:53

Initial contribution for new Denon-Marantz 2.0 binding

26cadf2

Signed-off-by: Jan-Willem Veldhuis <[email protected]> (github: @jwveldhuis)

Processed review comments. Added some additional fixes.

76f582b

Signed-off-by: Jan-Willem Veldhuis <[email protected]> (@github: jwveldhuis)

Processed review comments. Added some additional fixes.

df026e3

Signed-off-by: Jan-Willem Veldhuis <[email protected]> (github: jwveldhuis)

Fixed log message in handleCommand. Removed @nonnull annotation.

1a8aae7

Signed-off-by: Jan-Willem Veldhuis <[email protected]> (github: jwveldhuis)

Change log level of message about unsupported discovered devices

b217d1e

Signed-off-by: Jan-Willem Veldhuis <[email protected]> (github: jwveldhuis)

Trim leading spaces when parsing Volume from XML.

e381dc7

Signed-off-by: Jan-Willem Veldhuis <[email protected]>

Used Channel Groups for the common channels for Main Zone, Zone2 and

9d03b12

Zone3. Fixed comments according to code analysis report. Signed-off-by: Jan-Willem Veldhuis <[email protected]> (github: jwveldhuis)

Fix channel-groups definition.

35106bc

Signed-off-by: Jan-Willem Veldhuis <[email protected]> (github: jwveldhuis)

kaikreuzer requested changes May 1, 2018

View reviewed changes

Processed review comments.

c6625a0

Signed-off-by: Jan-Willem Veldhuis <[email protected]> (github: jwveldhuis) Also-by: Jeroen Idserda (github: idserda)

kaikreuzer approved these changes May 14, 2018

View reviewed changes

kaikreuzer merged commit 5bf4630 into openhab:master May 14, 2018

jwveldhuis deleted the denon-marantz-binding branch May 14, 2018 07:53

jwveldhuis restored the denon-marantz-binding branch May 14, 2018 08:03

jwveldhuis deleted the denon-marantz-binding branch May 14, 2018 08:03

jwveldhuis restored the denon-marantz-binding branch May 14, 2018 08:03

martinvw added this to the 2.3 milestone May 25, 2018

		@@ -0,0 +1,86 @@
		/**
		* Copyright (c) 2010-2017 by the respective copyright holders.

		@@ -57,26 +62,30 @@ The DenonMarantz AVR supports the following channels (some channels are model sp
		\| mainVolume \| Dimmer \| Main zone volume


		logger.debug("IP Address: {}", host);

		// try a HTTP request to autoconfigure the HTTP / Telnet parameter

[denonmarantz] new Denon / Marantz 2.0 binding #2859

[denonmarantz] new Denon / Marantz 2.0 binding #2859

Conversation

jwveldhuis commented Nov 19, 2017

jwveldhuis commented Nov 19, 2017 • edited Loading

jwveldhuis commented Nov 20, 2017

jwveldhuis commented Dec 30, 2017

jwveldhuis commented Jan 1, 2018 • edited Loading

jwveldhuis commented Jan 3, 2018

triller-telekom 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

jwveldhuis commented Jan 26, 2018

jwveldhuis commented Feb 1, 2018 • edited Loading

triller-telekom commented Feb 2, 2018

jwveldhuis commented Feb 3, 2018 • edited Loading

triller-telekom 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

jwveldhuis Feb 6, 2018 • edited Loading

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

jwveldhuis commented Feb 5, 2018 • edited Loading

jwveldhuis commented Feb 5, 2018 • edited Loading

triller-telekom commented Feb 6, 2018

triller-telekom commented Feb 8, 2018

jwveldhuis commented Feb 8, 2018

jwveldhuis commented Feb 8, 2018 • edited Loading

triller-telekom commented Feb 8, 2018

jwveldhuis commented Feb 8, 2018

triller-telekom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwveldhuis commented Feb 9, 2018

triller-telekom commented Feb 9, 2018

triller-telekom commented Feb 9, 2018

jwveldhuis commented Feb 9, 2018

kaikreuzer commented Feb 13, 2018

jwveldhuis commented Apr 26, 2018

kaikreuzer commented Apr 26, 2018

jwveldhuis commented Apr 26, 2018

jwveldhuis commented Apr 29, 2018

kaikreuzer commented Apr 30, 2018

jwveldhuis commented Apr 30, 2018

kaikreuzer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwveldhuis commented Nov 19, 2017 •

edited

Loading

jwveldhuis commented Jan 1, 2018 •

edited

Loading

jwveldhuis commented Feb 1, 2018 •

edited

Loading

jwveldhuis commented Feb 3, 2018 •

edited

Loading

jwveldhuis Feb 6, 2018 •

edited

Loading

jwveldhuis commented Feb 5, 2018 •

edited

Loading

jwveldhuis commented Feb 5, 2018 •

edited

Loading

jwveldhuis commented Feb 8, 2018 •

edited

Loading

jwveldhuis May 2, 2018 •

edited

Loading

triller-telekom May 3, 2018 •

edited

Loading