[broadlink] Initial contribution - #11049
Conversation
Signed-off-by: John Marshall <john.marshall.au@gmail.com>
fwolter
added
the
new binding
|
Is there any relationship to the broadlinkthermostat binding? |
They may be using the same communications protocol underneath. Beyond that I don't know. |
|
It doesn't make much sense to have two bindings for the same system, speaking the same protocol. @flo-02-mu Is there any reason why you suffixed your binding with "thermostat"? Is that another subset of devices or different protocols or is it even the same vendor? |
|
I took a look and the other already merged binding uses the library linked below, which does support RM2 and SP1 and SP2 devices. Sorry I cant comment on if the RM4 is different to a RM2 protocol and if it would work. However the library does not seem to get updated very often. |
kaikreuzer
added
rebuild
Skinah
added
rebuild
|
I understand the reluctance to accept a binding so similar to one that's already merged; however as mentioned by @Skinah the protocol library used by the Due to the nature of the Broadlink device discovery mechanism, each supported device still has to be explicitly added to this binding anyway. |
|
Can you add yourself to the codeowners file please? Would be good to have this showing up with a green tick at passing the build. These 3 are described here. https://www.openhab.org/docs/developer/bindings/#include-the-binding-in-the-build Looking at the other library used in broadlinkthermostat binding in more depth I have concerns over it not only due to the last time it got any updates, but because it does not work on Java 11 and the PR to get this resolved has had no progress since April last year. Any PR would likely not progress = dead library. The forum has a post on this here: My opinion is we should merge this binding after going through review. |
…emillhousegroup/openhab2-addons into openhab-3.2-broadlink-binding Signed-off-by: John Marshall <john.marshall.au@gmail.com>
…emillhousegroup/openhab2-addons into openhab-3.2-broadlink-binding Signed-off-by: John Marshall <john.marshall.au@gmail.com>
…emillhousegroup/openhab2-addons into openhab-3.2-broadlink-binding Signed-off-by: John Marshall <john.marshall.au@gmail.com>
|
I'm still trying to gather the facts. The forum post linked by @Skinah states that the library is not compatible with Java 11, but the OH 3.x addon uses exactly this library: So, it must be Java 11 compatible.I would have no problem replacing the existing @flo-02-mu as the owner of the |
|
The java 11 compatibility of the bl-library was a missunderstanding: For building the bl-library the jaxb dependency needed to be there when building with java 11. The usage of the assembled library is not an issue. |
|
|
||
| The Broadlink RM family of devices can transmit IR codes. | ||
| The map file contains a list of IR command codes to send via the device. | ||
| The file uses the [Java Properties File format](https://en.wikipedia.org/wiki/.properties) and is stored in the `<OPENHAB_CONF>/transform` folder. |
There was a problem hiding this comment.
I'm pretty sure the default should not be the conf/transform folder. From having other bindings get merged, I was told the correct place is this using the binding name as a sub folder.....
<OPENHAB_USER_DATA>/broadlink/<UID>.map
or
<OPENHAB_USER_DATA>/broadlink/<UID>/broadlink.map
This location will have write permissions on mac/win/linux
Also consider what happens if someone has multiple things, hence the suggestion to use the Unique ID of the thing in the filename or a sub folder, whatever makes sense.
Use this code to get the location...
OpenHAB.getUserDataFolder() + "/broadlink/" + this.thing.getUID().getId() + "/"
I like the idea of it in a file so you can share the files to others easily.
If you made the change suggested above, then you could remove this as a config and it would be handled automatically. I suspect leaving it as is, people that own two devices will forget to name the map to something different on the second device and that will cause it to wipe the file from the first. This is where having the UID will stop that occurring.
| props.stringPropertyNames().forEach(k -> { | ||
| commandMap.put(k, props.getProperty(k)); | ||
| }); | ||
| logger.info("Read {} commands from {}", commandMap.size(), mapFilePath); |
There was a problem hiding this comment.
info should rarely be used to keep logs clean, suggest this is debug.
|
|
||
| public BroadlinkDiscoveryService() { | ||
| super(BroadlinkBindingConstants.SUPPORTED_THING_TYPES_UIDS_TO_NAME_MAP.keySet(), 10, true); | ||
| logger.info("BroadlinkDiscoveryService - Constructed"); |
| logger.trace("Comparing with desired mac: {}", missingThingConfig.getMacAddressAsString()); | ||
|
|
||
| if (missingThingConfig.getMacAddressAsString().equals(remoteMAC)) { | ||
| logger.info("We have a match for target MAC {} at {} - reassociate!", remoteMAC, remoteAddress); |
| logger.warn("Error while receiving", e); | ||
| } | ||
| } | ||
| logger.info("Receiver thread ended"); |
There was a problem hiding this comment.
Debug seems a better fit, or remove it to give more performance.
| socketReceiveThread.interrupt(); | ||
| } | ||
| if (socket != null) { | ||
| logger.info("Socket closed"); |
There was a problem hiding this comment.
Debug seems a better fit. If this happens a lot, then consider removing this to improve performance. There is a cost each time Java has to check if the logger is set to debug/info etc...
| } | ||
|
|
||
| public void onDeviceRediscovered(String newIpAddress) { | ||
| logger.info("Rediscovered this device at IP {}", newIpAddress); |
|
I think the way to go is to ship both bindings, so users of the broadlinkthermostat binding are not fooled if their binding is removed from the distribution. @themillhousegroup Are you going to address @Skinah's review comments? |
fwolter
added
the
awaiting feedback
|
This PR seems to be broken and modifies too many files outside of the binding. Someone here can probably offer help to fix - normally happens when you have opened PR a long time ago and you pull in new changes (has happened on my PRs and others before). If fixing is not feasible then you might want to close and re-open a new PR without the issues - just make sure you sort out all the remaining change requests first. |
|
There's also a ton of noise from what looks like an auto-formatter changing the first couple of lines in every file.
|
|
Yep I'm going to kill this PR and try again. It feels like trying to nail jelly to a moving train getting this PR landed :-( |
|
@themillhousegroup john - it seems you never re-opened your PR for this. Does this mean your project is now abandoned? Your version of the Broadlink binding seems to support RF devices, while the others do not sadly. |
|
Hi @alackmann yes I'm afraid I no longer have the time to give this binding the attention it requires; general maintenance tasks once in the main codebase might be possible, but unfortunately getting it merged has just proved to be too much. Some months ago I did have a conversation with @rlarranaga regarding him trying to steer a fresh merge PR into openHAB, but I'm not sure what became of that. |
|
Hello, I started work on the PR and managed to get a couple of things done, i.e: text header updates, upgrading junit, change log level of messages, all compiling and running succesfully on OH 3.3. However, I have been trying to troubleshoot an issue where I get random BIG delays when sending commands (10+ seconds, most noticeable when trying to control a TV). I upgraded OH to 3.4 though, and caught wind of the fact that the broadlinkthermostat add-in also supports RM Mini 3 (I have mostly RM mini 4s, but one 3. Delay was showing in all of them). with the broadlinkthermostat add-in, these delays appear not to be present. Currently, i am playing around with a modified version of broadlink-java-api, trying to add RM4 mini support to confirm if the pesky delay is gone. beoadlinkthermostat's dependency on broadlink-java-api is an issue though. Not only it appears to be abandoned, but some of the components it uses (javax) need to be replaced soon too. To this, you can add the fact that I haven't developed code in years, and work is currently not leaving much free time. So progress is slow, to say the least. I would love to give a better update, with an ETA for submitting a PR, but I am afraid I am going to have to disappoint. |
|
Hi again @alackmann , @themillhousegroup @flo-02-mu , I decided to try one more time and compile the binding against OH 3.4.3 I am currently stuck with six errors of the same tipe that have to do with the handling of Nulls: Any idea how to handle these? Additionally, i have around 25 warnings of the following type:
I have seen the "Nullable Annotations" discussion in the OpenHAB forum. but i am not sure i understand it enough. Can anyone point me towards a good basic resource to figure those out? Lastly, i am currently trying to compile with:
but on the logs, i see:
I am pretty sure i am missing a lot of code checking there. Any ideas what can be wrong with that? Thanks |
|
Hi again @alackmann , @themillhousegroup @flo-02-mu , It looks like the errors i posted above stem from the fact that most of the methods are declared as @nullable (and in fact, explicitly return nulll), but the result of the method is being assigned to a byte array type. Since i cannot really annotate the methods as @nonnull, is there a way to define a byte array as @nullable? An example of this is:
In this case, sendAndReceiveDatagram is @nullable, and that is why the compiler throws an error when trying to assign its result to response. It does not seem i can annotate response[] as @nullable though. Thanks! |
|
Hello @alackmann @themillhousegroup @flo-02-mu I have managed to compile this binding with openhab 3.4.3 and am currently testing it. I have made all modification @flo-02-mu suggested except the change in the configuration file name. The binding uses a service model for retrieving the codes from the files that is not aware of thing instances. It is non trivial to make the change, so i won't be able to modify it in the short term. In the mean time, i have a couple of warnings when compiling that i would like to know if you have suggestions on how to clean up: The following come from mvn clean install: The following come from the report summary:
Not sure how to handle either of these. You can look at the code at the following fork: https://github.com/rlarranaga/openhab-addons I believe if we can knock these off, we are very close to a PR. Thanks! |
|
Hello @alackmann , @themillhousegroup , @flo-02-mu Thanks |
11 participants
A binding that can communicate with the Broadlink family of WiFi-addressable devices, currently comprising:
A1 environmental sensor
RMx Remote IR blasters
SPx WiFi-controlled power switches
MPx WiFi-controlled multi-outlet power strips
Development of this binding was begun some four (!) years ago by one Cato Sognen, who unfortunately seems to have stopped development a few months after posting about it here:
https://community.openhab.org/t/broadlink-binding-for-rmx-a1-spx-and-mp-any-interest/22768
I reverse-engineered the binding's JAR file and gradually built up an understanding of its operation, fixing a few bugs and adding support for more devices along the way, including bringing it up through the 2.x and 3.x upgrade paths.
There has been significant interest and help provided by interested parties, as evidenced by the enormity of the above thread! It would be fantastic to get this binding into the official release to reward their patience :-)
Thanks also to @Skinah who provided invaluable guidance in previous PRs for this binding.
closes #322
Signed-off-by: John Marshall john@themillhousegroup.com