New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[sony] New binding #6884

Closed

tmrobert8 wants to merge 3 commits into openhab:2.5.x from tmrobert8:1234-Sony

tmrobert8 commented Jan 21, 2020

After 4 years of development - finally asking for a merge on the sony addon. This is the most complete sony implementation to date and will be one of the largest bindings to review (be prepared).

A few notes:

This started as a 2.1 binding and I may have missed things that are now required (like I recently found out you need to set a status going out of a
There are a number of standard conventions that I've purposely ignored because they don't make sense for this large of a binding and were detrimental. The biggest would be constants - binding wide constants are in SonyBindingConstants, service wide constants or constants that are referenced by more than one file would be in either a service constants file or in a higher level file, constants that affect only a specific file are in that file itself.
I've left a number of todos, commented out code and comments in the code - these are important to keep as they have documented a number of things to remind me. The TODOs are generally things I need to find an example of. Commented out code generally was experimental code that may eventually go production or was an attempt that failed (but was important enought to document).
Logging crazy - these binding has extreme amount of logging in it and defines both debug/trace levels as needed. I cannot reduce the amount of logging in that it's vital to debugging someone's system remotely.
I already have a large user base - so please be aware of that when reviewing (like I'm really against changing channel names at this point or some very low level approach unless you find some drastic error with it).

Good luck and let me know what you'll need from me!

tmrobert8 added the new binding label

tmrobert8 mentioned this pull request

[sony][WIP] Sony Binding initial contribution #1249

Closed

TravisBuddy commented Jan 21, 2020

Travis tests were successful

Hey @tmrobert8,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

tmrobert8 force-pushed the 1234-Sony branch from 6d04459 to 211ec34 Compare

January 21, 2020 14:45

tmrobert8 requested review from 3cky, BobRak, caffineehacker, cdjackson, Cossey, cschneider, cweitkamp, davidgraeff, digitaldan, dominicdesu, fa2k, FlorianSW, fruggy83, gbicskei, gedejong, gerrieg, Hilbrand, J-N-K, jwveldhuis, kaikreuzer, kgoderis, klocsson, kubawolanin, limdul79, lolodomo and Markinus as code owners

January 21, 2020 14:45

cpmeister approved these changes

View reviewed changes

Contributor

cpmeister left a comment

LGTM

cpmeister reviewed

View reviewed changes

bundles/org.openhab.binding.sony/pom.xml Outdated Show resolved Hide resolved

TravisBuddy commented Mar 20, 2020

Travis tests were successful

Hey @tmrobert8,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

cweitkamp requested changes

View reviewed changes

...ny/src/main/java/org/openhab/binding/sony/internal/providers/SonyDefinitionProviderImpl.java Outdated Show resolved Hide resolved

mhilbush reviewed

View reviewed changes

...ny/src/main/java/org/openhab/binding/sony/internal/providers/SonyDefinitionProviderImpl.java Outdated Show resolved Hide resolved

TravisBuddy commented Mar 31, 2020

Travis tests were successful

Hey @tmrobert8,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

tmrobert8 requested a review from a team as a code owner

April 13, 2020 18:13

TravisBuddy commented Apr 13, 2020

Travis tests were successful

Hey @tmrobert8,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

2 similar comments

TravisBuddy commented Apr 17, 2020

Travis tests were successful

Hey @tmrobert8,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

TravisBuddy commented Apr 20, 2020

Travis tests were successful

Hey @tmrobert8,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

AlejandroImagineInc commented Apr 25, 2020

Hi, I have STR DN1030 sound receiver
Could you tell me where i can get an instruction to install "org.openhab.binding.sony-2.5.5-SNAPSHOT" in openhab?
Thank you very much

Contributor

lsiepel commented May 2, 2020 •

edited

Loading

Hi, I have STR DN1030 sound receiver
Could you tell me where i can get an instruction to install "org.openhab.binding.sony-2.5.5-SNAPSHOT" in openhab?
Thank you very much

I guess those questions belong here: https://community.openhab.org/t/sony-devices-binding/14052

Edit: All review comments are done right? 4 years of development wauw!

kaikreuzer self-assigned this

kaikreuzer force-pushed the 2.5.x branch from 536fc42 to 3ab1af1 Compare

June 21, 2020 16:12

kaikreuzer requested review from Hilbrand and cweitkamp

August 3, 2020 19:29

Hilbrand requested changes

View reviewed changes

Member

Hilbrand left a comment

Ok. I think I've reviewed this 😭 This is a horribly large pull request. Please don't that again 😜
In general my comments can be summarized as follows:

The Apache commons library will be removed in openHAB 3. Therefor it should not be used. Please replace all usages of this library (I guess the fastest is to add some util in you binding to implement the most used you have).
We don't have the immediate = true on Components. Please remove them (I commented on them, but I'm not sure if I got them all)
For java libraries we require to add a copyright notice about the library in the NOTICE file. I would suggest you need to do this for the JavaScript libraries you have included too (See some other NOTICE file on how to do it, it's 4 lines per item)
You have included a number of fonts. These add 2.3 MB to the size of the distribution. We try to minimize size as much as possible because with a lot of bindings it otherwise becomes a huge package. So my suggestion is to remove those fonts and rely on the standard provided fonts.
There are several other readme pages here. Recently I saw a comment about a missing page on our documentation page, which was also another additional page to a binding. So it looks like only the readme itself is published on the documentation page. Therefor it might be those other pages will not be available. This is something that might need to be looked into.

bundles/org.openhab.binding.sony/pom.xml Outdated

+                <parent>
+                  <groupId>org.openhab.addons.bundles</groupId>
+                  <artifactId>org.openhab.addons.reactor.bundles</artifactId>
+                  <version>2.5.5-SNAPSHOT</version>

Member

Hilbrand Sep 4, 2020

Suggested change

      
                <version>2.5.5-SNAPSHOT</version>
          
                <version>2.5.9-SNAPSHOT</version>

...org.openhab.binding.sony/src/main/java/org/openhab/binding/sony/internal/AbstractConfig.java

Comment on lines +21 to +22

		import org.apache.commons.lang.StringUtils;
		import org.apache.commons.lang.Validate;

Member

Hilbrand Sep 4, 2020

You need to remove all usages of the apache commons library as that library will be removed from openHAB 3.0.

...enhab.binding.sony/src/main/java/org/openhab/binding/sony/internal/SonyBindingConstants.java

+                  public static final Integer RSP_WAIT_TIMEOUTSECONDS = 10;
+                  /** The user agent for communications (and identification on the device) */
+                  public static final String NET_USERAGENT = "OpenHab/Sony/Binding";

Member

Hilbrand Sep 4, 2020

Suggested change

      
                public static final String NET_USERAGENT = "OpenHab/Sony/Binding";
          
                public static final String NET_USERAGENT = "openHAB/Sony/Binding";

...openhab.binding.sony/src/main/java/org/openhab/binding/sony/internal/SonyHandlerFactory.java

+               * @author Tim Roberts - Initial contribution
+               */
+              @NonNullByDefault
+              @Component(immediate = true, service = ThingHandlerFactory.class, configurationPid = "sony.things")

Member

Hilbrand Sep 4, 2020

Suggested change

      
            @Component(immediate = true, service = ThingHandlerFactory.class, configurationPid = "sony.things")
          
            @Component(service = ThingHandlerFactory.class, configurationPid = "binding.sony")

bundles/org.openhab.binding.sony/src/main/java/org/openhab/binding/sony/internal/SonyUtil.java

+                          return new ArrayList<>();
+                      }
+                      return list.stream().filter(e -> e != null).collect(Collectors.toList());

Member

Hilbrand Sep 4, 2020

Suggested change

      
                    return list.stream().filter(e -> e != null).collect(Collectors.toList());
          
                    return list.stream().filter(Objects::nonNull).collect(Collectors.toList());

bundles/org.openhab.binding.sony/src/main/resources/ESH-INF/config/simpleip.xml Outdated

    
              			<default>eth0</default>

              			<advanced>true</advanced>

              		</parameter>

              		<parameter name="refresh" type="integer">

Member

Hilbrand Sep 4, 2020

Suggested change

      
            		<parameter name="refresh" type="integer">
          
            		<parameter name="refresh" type="integer" unit="s">

Same comment for other parameters with time.

...inding.sony/src/main/java/org/openhab/binding/sony/internal/upnp/models/UpnpServiceList.java Outdated

    
               *

               * <pre>

               * {@code

                   need example

Member

Hilbrand Sep 4, 2020

Remove empty example.

...es/org.openhab.binding.sony/src/main/java/org/openhab/binding/sony/internal/net/NetUtil.java

+                          System.arraycopy(macBytes, 0, bytes, i, macBytes.length);
+                      }
+                      // logger.debug("Sending WOL to " + ipAddress + " (" + macAddress + ")");

Member

Hilbrand Sep 4, 2020

Remove commented out code.

bundles/org.openhab.binding.sony/src/main/java/org/openhab/binding/sony/internal/SonyUtil.java

+                      final Map<String, String> lowerMap1 = map1.entrySet().stream()
+                              .map(s -> new AbstractMap.SimpleEntry<>(StringUtils.lowerCase(s.getKey()),
+                                      StringUtils.lowerCase(s.getValue())))
+                              .collect(Collectors.toMap(k -> k.getKey(), v -> v.getValue()));

Member

Hilbrand Sep 4, 2020

Doesn't toLowerCase not work (without using the additonal .map)?

Suggested change

      
                            .collect(Collectors.toMap(k -> k.getKey(), v -> v.getValue()));
          
                            .collect(Collectors.toMap(k -> k.getKey().toLowerCase(), v -> v.getValue().toLowerCase()));

...ny/src/main/java/org/openhab/binding/sony/internal/providers/SonyDefinitionProviderImpl.java

+                  @Override
+                  public boolean removeListener(final SonyModelListener listener) {
+                      return sources.stream().map(s -> s.removeListener(listener)).anyMatch(e -> e);

Member

Hilbrand Sep 4, 2020

Suggested change

      
                    return sources.stream().map(s -> s.removeListener(listener)).anyMatch(e -> e);
          
                    return sources.stream().map(s -> s.removeListener(listener)).anyMatch(Function.identity());

kaikreuzer added migrate to OH3 and removed migrate to OH3 labels

Collaborator

openhab-bot commented Sep 21, 2020

Automatic code migration to openHAB 3 succeeded.

The resulting code can be found at https://ci.openhab.org/job/openHAB-Addons-Migration/53/artifact/bundles/.

You can download the migrated code from there and create a new PR against the master branch of the openhab-addons repo to contribute it for being included in openHAB 3.x.

Please see this issue about the details on how to proceed with your existing PR.

tmrobert8 added 2 commits

December 1, 2020 10:16


          Redo to 2.5.11

6fb3882


          Added other files

b33b0d6

tmrobert8 force-pushed the 1234-Sony branch from 838e310 to b33b0d6 Compare

December 1, 2020 15:18


          Fix simpleip power loop bug, improve status updating

421ea97

kaikreuzer added migrate to OH3 and removed migrate to OH3 labels

Collaborator

openhab-bot commented Dec 1, 2020

Automatic code migration to openHAB 3 succeeded.

The resulting code can be found at https://ci.openhab.org/job/openHAB-Addons-Migration/115/artifact/bundles/.

You can download the migrated code from there and create a new PR against the main branch of the openhab-addons repo to contribute it for being included in openHAB 3.x.

Please see this issue about the details on how to proceed with your existing PR.

Member

kaikreuzer commented Jan 12, 2021

Closing this PR as it is not in a mergeable state. Please follow the instructions to port this PR to the main branch for openHAB 3.

kaikreuzer closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mhilbush mhilbush left review comments

Cossey Cossey left review comments

cpmeister cpmeister approved these changes

Hilbrand Hilbrand requested changes

kaikreuzer Awaiting requested review from kaikreuzer

cweitkamp Awaiting requested review from cweitkamp

Labels

cre new binding