[Russound] Added discovery of new devices #2135

tmrobert8 · 2017-04-07T13:30:18Z

This pull request adds auto discovery of Russound devices (and all controllers/sources/zones on the device itself). Likewise, I've added all the media management functionality (to support UIs - like the HABPanel widget I've developed for this). Removed/rearranged alot of functionality as well.

Let me know what I've done wrong ;)

mention-bot · 2017-04-07T13:30:19Z

@tmrobert8, thanks for your PR! By analyzing the history of the files in this pull request, we identified @kaikreuzer and @martinvw to be potential reviewers.

martinvw · 2017-04-12T14:25:02Z

@tmrobert8 can you take a look at the errors reported by the build?

tmrobert8 · 2017-04-12T19:05:09Z

Martin - interesting decision enforcing IAE over NPE - but did the change...

martinvw · 2017-04-12T19:40:21Z

@tmrobert8 I did not look at the errors, what was the exact error? Was it that you should not throw your own NPE?

I think I agree with this answer: http://stackoverflow.com/questions/3322638/is-it-okay-to-throw-nullpointerexception-programmatically#answer-3323006

tmrobert8 · 2017-04-12T19:49:26Z

Yep - I personally don't agree with that but I don't feel strongly enough to really care that much (nor care to waste my time arguing one way or the other)

;)

At any rate, changed them all to IAE and it passes now.

martinvw

Thanks for the big update of your/this binding. Great to see bindings being actively maintained by their authors!

I see that you reverted some changes which have been made in the meanwhile I don't know whether this was on purpose or not, but please keep them intact :-)

So the prefered order of modifiers final static
Loggers should be non-static and called logger.

I think that the code guidelines might have been updated since this binding was first developed. But it would be great if we could make your binding follow the current guidelines, if you really want we could make that a separate PR (before or after this one) but I prefer not to add new 'violations'.

So can you also please check the full report, you can find it in:

openhab2-addons\addons\binding\org.openhab.binding.russound\target\code-analysis\report.html

Besides that I added some other comments (but most of them are about the (current) guidelines). Questions? Please let me know I'll try to answer them ASAP.

martinvw · 2017-04-12T20:05:50Z

...ing.russound/src/main/java/org/openhab/binding/russound/internal/RussoundHandlerFactory.java

@@ -33,11 +35,10 @@
 * @author Tim Roberts
 */
 public class RussoundHandlerFactory extends BaseThingHandlerFactory {
+    private final static Logger logger = LoggerFactory.getLogger(RussoundHandlerFactory.class);


loggers should non-static, can you check all of them?

martinvw · 2017-04-12T20:07:18Z

...ing.russound/src/main/java/org/openhab/binding/russound/internal/RussoundHandlerFactory.java

+        logger.trace("Try to register Discovery service on BundleID: {} Service: {}",
+                bundleContext.getBundle().getBundleId(), DiscoveryService.class.getName());
+
+        Hashtable<String, String> prop = new Hashtable<String, String>();


Hashtable should not be used anymore, see the following remark

If a thread-safe implementation is not needed, it is recommended to use HashMap in place of Hashtable. If a thread-safe highly-concurrent implementation is desired, then it is recommended to use ConcurrentHashMap in place of Hashtable.

Source: https://docs.oracle.com/javase/7/docs/api/java/util/Hashtable.html

Unfortunately the bundleContext.registerService call requires a Dictionary<String, ?> - of which Hashtable is the only Dictionary implementation (there might be something in apache utils that I'm unaware of). Ignoring this one

Pity, sorry for the false alarm 🙁

martinvw · 2017-04-12T20:08:26Z

...in/java/org/openhab/binding/russound/internal/discovery/RioSystemDeviceDiscoveryService.java

+    private final RioSystemHandler sysHandler;
+
+    /** Pattern to identify controller notifications */
+    private final Pattern RSP_CONTROLLERNOTIFICATION = Pattern.compile("(?i)^[SN] C\\[(\\d+)\\]\\.(\\w+)=\"(.*)\"$");


All caps are reserved for constants. If this is one, can you then make it private final static

Pretty sure I got them all (over about 7 classes). Let me know if I didn't on the next commit

martinvw · 2017-04-12T20:08:57Z

...in/java/org/openhab/binding/russound/internal/discovery/RioSystemDeviceDiscoveryService.java

+            _listener = null;
+        }
+
+        // sysHandler.unregisterDiscoveryService();


Please remove this commented line of code

I did this - however, just food for thought. I fully disagree with this standard. Commented code has alot of value to someone reading the code (including the original author) since it's like code that was commented out either for a specific reason or for code that could potentially be used in the future. I see it as very much like documentation. But again, don't really care about this one - so I did it (any any others that I coudl find)

martinvw · 2017-04-12T20:10:33Z

...in/java/org/openhab/binding/russound/internal/discovery/RioSystemDeviceDiscoveryService.java

+    /**
+     * The {@link SocketSession} that will be used to scan the device
+     */
+    private SocketSession _session;


Please follow the java naming convention instance fields are camelCased and do not have underscores in them.
See bullat A.1 @ http://docs.openhab.org/developers/development/guidelines.html#a-code-style

martinvw · 2017-04-12T21:13:01Z

...ing.russound/src/main/java/org/openhab/binding/russound/internal/rio/RioPresetsProtocol.java

+    /**
+     * The minimum timespan between updates of source presets via {@link #refreshPresets(Integer)}
+     */
+    private static final long UPDATETIMESPAN = 60000;


Minor: I think the correct name would be UPDATE_TIME_SPAN.

Just a suggestion did you know java allows underscore in number to be able to properly display large value, so you could do this:

private static final long UPDATE_TIME_SPAN_MS = 60_000;

martinvw · 2017-04-12T21:17:37Z

...g.russound/src/main/java/org/openhab/binding/russound/internal/rio/zone/RioZoneProtocol.java

+    /**
+     * Sets the zone favorites to what is currently playing
+     *
+     * @param favJsona possibly null, possibly empty json for favorites to set


Minor: favJsona => missing space in docblock

martinvw · 2017-04-12T21:25:12Z

...in/java/org/openhab/binding/russound/internal/discovery/RioSystemDeviceDiscoveryService.java

+
+                    final DiscoveryResult discoveryResult = DiscoveryResultBuilder.create(thingUID)
+                            .withProperty(RioZoneConfig.Zone, z).withBridge(controllerUID)
+                            .withLabel((StringUtils.isEmpty(name) || name.equalsIgnoreCase("null") ? "Zone" : name)


You just checked name StringUtils.isNotEmpty(name) on line 218. And can it really equals the string "null", then this should suffice: name.equalsIgnoreCase("null") ? "Zone" : name

Remove the redundant isnotempty name - unfortunately Russound will put out a 'null' name

martinvw · 2017-04-12T21:29:34Z

...ssound/src/main/java/org/openhab/binding/russound/internal/discovery/RioSystemDiscovery.java

+                for (int i = 1; i < 255; i++) {
+                    addresses[i - 1] = subnet + i;
+                }
+                for (int i = 0; i < addresses.length; i++) {


Why do you iterate twice? You should only need the first one with the body of the second one, does this work?

for (int i = 1; i < 255; i++) { final String address = subnet + i; scheduler.execute(new Runnable() { @Override public void run() { scanAddress(address); } }); }

lol - use to be a ton of code between those statements and didn't notice that when I removed it all!

martinvw · 2017-04-12T21:32:28Z

...ssound/src/main/java/org/openhab/binding/russound/internal/discovery/RioSystemDiscovery.java

+                logger.trace("Starting scan of {}", subnet);
+
+                final String[] addresses = new String[254];
+                for (int i = 1; i < 255; i++) {


Are you aware that subnets can also be bigger?

You can have this complete range in a single home network: 192.168.0.0 - 192.168.255.255 or even bigger.

Besides that maybe a warning should be added before starting discovery like this, because some (company) networks might not like this port-scan. And if someone is connected to a vpn that network will be scanned as well.

In reverse - is there a way to 'warn' someone before starting a discovery? Not aware but let me know if there is one.

Of course they can be bigger, but I'm looking at a normal use case (ie home network) which, 99.9% of the time, will be a simple /24. Would be too onerous to try to do a full scan. For those people that fall into the .1% with complex HOME networks - they can simply define the system bridge directly (they'd certainly not need a discovery process).

Note that the Network binding does the same, so most likely this is not a problem at all 👍 . I just happened to see that the Network binding uses a subnet calculation via an exernal jar maybe you could check it out too?

The only plausible ways to add a warning are:

the Readme (most likely only read after any problem)

or optionally in description of the binding it's displayed if you click from the PaperUI inbox to discover new devices, an impression of how this could look like.

Its up to you :-)

Didn't know about subnetutils - incorporated it (and the executor as well)

martinvw · 2017-04-12T22:02:02Z

...ound/src/main/java/org/openhab/binding/russound/internal/rio/RioSystemFavoritesProtocol.java

+     */
+    @Override
+    public void responseReceived(String response) {
+        if (response == null || response == "") {


You cannot compare a string like this, maybe you should use commons stringutils I saw that it was used in other places of this binding as well.

You might have other occurrences of this same line in other places as well, can you search for it?

lol - I work with too many languages and that occasionally slips through!

martinvw · 2017-04-13T05:25:38Z

Yep - I personally don't agree with that but I don't feel strongly enough to really care that much (nor care to waste my time arguing one way or the other)

;)

At any rate, changed them all to IAE and it passes now.
Note that there is another option Objects.requireNonNull(gson, "gson cannot be null"), this will change your 3 line check to single line, if you like you can check it out. And you can still throw NPE's from the precondition :-)

https://docs.oracle.com/javase/7/docs/api/java/util/Objects.html#requireNonNull(T,%20java.lang.String)

Although I do think that you should NOT throw NPE for things which non-null but empty.

martinvw · 2017-04-13T15:22:20Z

....russound/src/main/java/org/openhab/binding/russound/internal/rio/AbstractBridgeHandler.java

+    @SuppressWarnings("rawtypes")
+    public RioSystemFavoritesProtocol getSystemFavoritesHandler() {
+        final Bridge bridge = getBridge();
+        if (bridge != null && bridge.getHandler() instanceof AbstractBridgeHandler) {


Why the difference with line 110

Martin - btw just wanted to thank you for the time reviewing. I have no idea what you mean by this comment - would you mind explaining a bit more?

I would expect that since line 109 and 125 are the same and line 111 and 127 are doing more or less the same (calling a method on the bridge its handler). I would expected the lines 110 and 125 to be resilient to the same amount of null values. But I might miss some knowledge which would make it unnecessary.

tmrobert8 · 2017-04-13T18:46:33Z

Martin, I believe I've gotten all the changes (might have missed some). You'll see a few did change quite radically (mainly SocketChannelSession) because I took the opportunity to rewrite it a bit (to simplify) - always wanted to do that but never had the motivation to sit down and do it!

Let me know if you see anything I missed

martinvw

I'm glad you're okay with my review. Great that you were able to fix all of it in such a short time!

I owed you an explanation of an earlier remark so I gave it. And you missed one thing, the order of some of the modifiers maybe you can search and replace all?

Did you btw also see my message about requireNonNull?

martinvw · 2017-04-14T05:48:55Z

...russound/src/main/java/org/openhab/binding/russound/internal/rio/source/RioSourceConfig.java

@@ -15,6 +15,11 @@
 */
 public class RioSourceConfig {
    /**
+     * Constant defined for the "source" configuration field
+     */
+    public final static String Source = "source";


It seems that you missed this one

martinvw · 2017-04-14T05:51:20Z

....russound/src/main/java/org/openhab/binding/russound/internal/rio/AbstractBridgeHandler.java

+    @SuppressWarnings("rawtypes")
+    public RioSystemFavoritesProtocol getSystemFavoritesHandler() {
+        final Bridge bridge = getBridge();
+        if (bridge != null && bridge.getHandler() instanceof AbstractBridgeHandler) {


I would expect that since line 109 and 125 are the same and line 111 and 127 are doing more or less the same (calling a method on the bridge its handler). I would expected the lines 110 and 125 to be resilient to the same amount of null values. But I might miss some knowledge which would make it unnecessary.

martinvw · 2017-04-14T05:54:53Z

...b.binding.russound/src/main/java/org/openhab/binding/russound/internal/rio/RioConstants.java

@@ -19,118 +19,134 @@
 public class RioConstants {

    // BRIDGE TYPE IDS
-    public static final ThingTypeUID BRIDGE_TYPE_RIO = new ThingTypeUID(RussoundBindingConstants.BINDING_ID, "rio");
-    public static final ThingTypeUID BRIDGE_TYPE_CONTROLLER = new ThingTypeUID(RussoundBindingConstants.BINDING_ID,
+    public final static ThingTypeUID BRIDGE_TYPE_RIO = new ThingTypeUID(RussoundBindingConstants.BINDING_ID, "rio");


Some public final static are still changed maybe you can do a replace-all 'final static' -> 'static final'

lol - I did a global replace on private static final and missed the public ones. Not sure why I did that! (must have been caught up in the moment!)

Ah - I see what you meant about the lines - don't know why they differ (probably copy/pasted and fixed one without the other). At any rate, I've updated to these latest comments and pushed.

Note: I've known about the requireNotNull and use it in current (other) bindings - but I didn't think about it when I made all these mods and tell you the truth - will probably leave it for a future date at this point (want to get this off my plate to get back to the sony binding)

martinvw

@tmrobert8 thanks!

@kaikreuzer since its not a small PR on an existing binding its yours :-)

A lot of 'warnings' from the code guidelines were resolved, new functions were added and the readme was extended. So LGTM

kaikreuzer

I think @tmrobert8 just wants to avoid any code reviews by producing more code than anyone would be willing to review ;-) Amazing work...

So I only managed to scan very superfluously over it and came up with a few very unimportant comments.

kaikreuzer · 2017-04-21T15:46:46Z

...in/java/org/openhab/binding/russound/internal/discovery/RioSystemDeviceDiscoveryService.java

+                discoverControllers();
+                discoverSources();
+            } catch (IOException e) {
+                logger.warn("Trying to scan device but couldn't connect: {}", e.getMessage(), e);


that might be expected, it the device is not reachable, right? So better reduce this to debug.

kaikreuzer · 2017-04-21T15:52:19Z

...ussound/src/main/java/org/openhab/binding/russound/internal/rio/system/RioSystemHandler.java

+                this.scheduler.execute(new Runnable() {
+                    @Override
+                    public void run() {
+                        logger.info("Starting device discovery");


please reduce this to debug

kaikreuzer · 2017-04-21T15:54:26Z