I added my changes from your rm4pros branch. You will basically find the following:
-Properties files have been replaced by OpenHAB StorageService.
-Modified (or created, when necessary) lookup, add, replace and remove functions for IR and RF codes. These functions now use the StorageService too.
Modified the README.md file to include commentary on the new storage, the difference with the old storage, and also python script that will migrate an old format file to the new format.
I have 3 things pending addition to this branch:
-Need to fix some minor compile-time warnings
-I am currently playing with the channels, deciding what is the best way to provide an interface to add, remove and modify codes. The main difficulty here is that OpenHAB does not have (that I now of) a channel that will render into free-form text field in the interface, and thus, it is difficult to input the Label you want to use when you are learning a new code.
-When i integrated the StorageService libraries, the unit tests stopped working, so i temporarily removed. I think this is because now that we are using StorageService, these tests have become integration tests and need to be moved to the itest directory. Haven’t tested this yet though.
I will give you a shout when i update those changes.
Hi @ricardol great to hear about all the changes you are making! With respect to the channels, I have been thinking about it as well. For now I choose the easy option by making it just a property of the thing for the command to learn and then use a set in the UI to select the learned command.
However, a better solution, but (much) more work, is to use a system similar to the generic MQTT binding. It allows for the dynamic creation of channels through the UI. Install the add-on, play around with it a bit and let me know what you think of it.
It would be good to first clean up the code (compile warnings, code style stuff etc.) and fix the unit tests. Before we delve into the complexity of getting the channels to work nicely. Could you do a pull request so I can merge in your changes and contribute to the above?
Hi @Bernd_Ritter what name would you propose? I have been thinking about it for a while and I can’t get a better name, so maybe you have some good suggestions
I believe during the last PR I opened, there was a discussion about removing broadlinkthermostat altogether from OpenHAB…It relies on EOL and unmaintained libraries.
I am currently giving it a try to get the tests working again. I need to admit (i haven’t developed in a long time). I am not sure what I am doing, and i haven’t managed to get it to compile.
I will try to get it working this week, so i can make that pull request as clean as possible. If i cannot do that, I might just to the PR without them, and let you know. The compile warnings, etc are simple enough to clean, so i will have that done for sure.
Just updating the thread for progress. I managed to compile the tests successfully, and clean up the warnings. I still have some work to do removing things that are not used anymore and i am away from my OpenHAB instance so i can’t test. The pull request ill be delayed until next weekend, but almost there!
Hey @AntonJansen
While i am testing the binding, i also pushed to the previously mentioned branch my lastest (and hopefully, last) changes before this gets PR’d to openHAB.
Currently, i am having a number of warnings that i cannot resolve:
[WARNING] openhab2-addons/openhab-addons/bundles/org.openhab.binding.broadlink/src/main/java/org/openhab/binding/broadlink/internal/handler/BroadlinkRemoteModel4ProHandler.java:[120,29] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] openhab2-addons/openhab-addons/bundles/org.openhab.binding.broadlink/src/main/java/org/openhab/binding/broadlink/internal/handler/BroadlinkRemoteModel4ProHandler.java:[198,25] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] openhab2-addons/openhab-addons/bundles/org.openhab.binding.broadlink/src/main/java/org/openhab/binding/broadlink/internal/handler/BroadlinkRemoteModel4ProHandler.java:[243,25] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] openhab2-addons/openhab-addons/bundles/org.openhab.binding.broadlink/src/main/java/org/openhab/binding/broadlink/internal/handler/BroadlinkRemoteModel4ProHandler.java:[270,9] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] openhab2-addons/openhab-addons/bundles/org.openhab.binding.broadlink/src/main/java/org/openhab/binding/broadlink/internal/handler/BroadlinkRemoteHandler.java:[128,17] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] openhab2-addons/openhab-addons/bundles/org.openhab.binding.broadlink/src/main/java/org/openhab/binding/broadlink/internal/handler/BroadlinkRemoteHandler.java:[151,17] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] openhab2-addons/openhab-addons/bundles/org.openhab.binding.broadlink/src/main/java/org/openhab/binding/broadlink/internal/handler/BroadlinkRemoteHandler.java:[167,9] Potential null pointer access: this expression has a '@Nullable' type
I think 3 of these where already there in your rm4pros branch. They they can be overriden with:
@SuppressWarnings("null")
But i have left them there for now, so we can discuss if there if a better way to handle them. It looks like a compiler-check error to me.
The code also generates A BUNCH of these:
...
[INFO] /openhab2-addons/openhab-addons/bundles/org.openhab.binding.broadlink/src/test/java/org/openhab/binding/broadlink/internal/handler/BroadlinkSocketModel3SHandlerTest.java:[102,9] Unsafe interpretation of method return type as '@NonNull' based on substitution 'T=org.openhab.core.thing.binding.@NonNull ThingHandlerCallback'. Declaring type 'org.mockito.Mockito' doesn't seem to be designed with null type annotations in mind
[INFO] /openhab2-addons/openhab-addons/bundles/org.openhab.binding.broadlink/src/test/java/org/openhab/binding/broadlink/internal/handler/AbstractBroadlinkThingHandlerTest.java:[51,44] Unsafe interpretation of method return type as '@NonNull' based on substitution 'T=org.openhab.binding.broadlink.internal.socket.@NonNull RetryableSocket'. Declaring type 'org.mockito.Mockito' doesn't seem to be designed with null type annotations in mind
[INFO] /openhab2-addons/openhab-addons/bundles/org.openhab.binding.broadlink/src/test/java/org/openhab/binding/broadlink/internal/handler/AbstractBroadlinkThingHandlerTest.java:[52,56] Unsafe interpretation of method return type as '@NonNull' based on substitution 'T=org.openhab.binding.broadlink.internal.socket.@NonNull NetworkTrafficObserver'. Declaring type 'org.mockito.Mockito' doesn't seem to be designed with null type annotations in mind
...
Those, i have no idea how to deal with. The are just INFO level, but because there are a lot of them, it kind of muddies the compile output.
Let me know what your thoughts are before I send you the PR request.
Hi @ricardol, I think we should try to resolve these issues quickly, so I can merge it with the open PR that I have running with the main branch of openhab. This way we will have very little legacy in the field with the old mapping files and have to deal with it’s migration.
As you can tell, the code in the rm4pros branch was very rough and not cleaned up yet. I hold back on it while you were working on it, as to reduce potential merge conflicts.
I checked out your code and indeed some supressWarnings does the trick for the compiler warnings. There are also some style warnings we need to take care off. For the Mockito test INFO warnings, I have no idea It seems reading the documentation that Mockito does not use null values, but rather has default return types (e.g. empty list, false, etc.).
Moving forward, I suggest the following approach. You create a pull request to me and I merge it with my rm4pro branch. I added you as a contributor to my repository, so we can then work together on the same branch (rm4pro) in my repo. Focus is on getting the errors / warning / style issues resolved so we can merge it with the pending PR.
I checked out your code and indeed some supressWarnings does the trick for the compiler warnings.
got rid of these in one of my last commits
There are also some style warnings we need to take care off.
I looked at this, but there is something wrong with my summary file for errors. Not only it appears not to change, it also appears inaccurate. for example, one of the warnings i get is:
checkstyle 2 98 style MarkdownCheck The line after code formatting section must be empty.
in README.md
I am pretty sure i solved that already, but the error still shows up. That happened with other errors too, so i could not advance much on this.
For the Mockito test INFO warnings, I have no idea
Me neither, but i browsed around the forum, and it looks like we should be ok ignoring them.
You create a pull request to me and I merge it with my rm4pro branch.
Done.
I added you as a contributor to my repository,
I accepted the invite.
I would recommend that we put ourselves a deadline of end of this week to resolve these. We can also shoot a message in the PR letting them know we are mergint this this week. that way they do not waste time reviewing. Do you happen to know when does the window for 4.2 close?
I have merged the PR. I will not have time today to work on this. I will spend some time tomorrow evening / the weekend. Hopefully, we can then merge it with the outstanding PR. I have already made comment about this change coming, so the reviewers of the PR should not be surprised.
I got rid of them using supresswarnings. After spending a fair bit of time looking into it, i think the checking mechanism is kind of broken. All the functions are declared as null-able, so i could not find a reason why the compiler would throw a warning there.
I will try to finish my testing today. There is a couple of things that i would also like to get to, depending on time, before Friday:
-Currently, there are 2 sets of functions to store/modify/lookup/delete code, one for IR and one for RF. They are identical except for the storage they write to, so i will try to consolidate them, so there less duplicated code to maintain.
-It would be great if after any successful modification of the storage, the parameter that holds the command label gets erased, as to make sure no unwanted overwrites happen. Not even sure if that can be done though.
-A bit of a nitpick. i do not like the “Check and save” name used to add a code…would it be better to just say “Save new code”?
-I own 1 RM3, and 2 different types of RM4. I noticed that for the RM3 and one of the RM4, the process to add a thing is different ( for one, the interface only asks for a thing name, for the other one, there is a couple of options present to add as thing, etc etc). I will try and see if i canmake the behaviour the same for all of them.
-I notice the handler functions used to learn codes are quite different between RF and IR. They even have different names. i would your input to understand if we should consolidate names and if there is any chance they can be consolidated.
-I have not had time to look at dynamic channels. But i still think that can come after the binding is merged.
Thanks @AntonJansen for the binding!
I’m trying to connect a RM4mini, managed to see it in the things and have four channels. Learning of the code, gave me the code in the openhab.log, but now I’m stuck. I’m not able to switch on the aircon.
This is my item:
Switch Air_cond_on "Aircon ON 25°C" { channel="broadlink:rm4mini:2747c1ce9c:Aircon_ON" }
It seems reading the documentation that Mockito does not use null values, but rather has default return types (e.g. empty list, false, etc.).