themillhousegroup / openhab2-addons

Add-ons for openHAB 2.x
Eclipse Public License 2.0
36 stars 17 forks source link

"On the Fly" code generation #31

Open rlarranaga opened 2 years ago

rlarranaga commented 2 years ago

Hello, I am using this add on with OpenHab, and got to the point where I am trying to control my Air Conditioners. The protocol for AC in general is a little more convoluted, as each button press sends all parameters of the AC.

The Broadlink Add-On uses a .map file to retrieve the codes that the devices will send. My problem is that if I want to either increase or decrease the temperature for example, I might need to build the IR code dynamically, and send it on the fly instead of having it pre packaged in a file.

Is there any way to send IR codes that are not contained in the .map file? Thanks

themillhousegroup commented 2 years ago

Hi, That's a really great question and I'm afraid the answer at this point in time is "No - we only support sending static commands from a .map file" - as you correctly point out. It would be a great feature to support some kind of variable substitution scheme but to be honest, I don't have the time to investigate how to achieve that as the IR strings are pretty "opaque" to the binding - it's really just telling the hardware to play back a string that was previously recorded on the hardware - and I still haven't succeeded in landing this binding in the main openHAB codebase, which needs to be my highest priority. Sorry for the bad news :-(

rlarranaga commented 2 years ago

Hello @themillhousegroup,

Understandable, thanks for the quick answer! What is pending for you to be able to land the binding in the main openHAB codebase? Do you need any help? I might look around and see if I can add this feature.

One thing I am a bit concerned re: binding performance is the delay in command (IR) execution. I was playing around with the binding vs the BroadLink app yesterday, and the app is way more responsive than the binding through openHAB, which is surprising, since it has a road trip to the cloud. When using a tv remote, and commanding the binding through a HABpanel touch screen, I get these random delays when pressing a button, that can take seconds to respond, and they kill the experience Have you experienced anything like this?

Thanks

themillhousegroup commented 2 years ago

Hi, To be honest, I'm not giving the binding the attention it deserves. I've been too busy with other "life" stuff to keep driving the merge-into-openHAB-main process, so it keeps falling behind and the merge conflicts just kill me. If you'd like to take ownership of landing it in openHAB I'd be eternally grateful.

I've heard previously about occasional lagginess in the IR commands. As my needs are extremely simple (playing back one of 4 fixed codes to 2 aircon units) I've never encountered the problem, but I wonder if there's a race condition somewhere if lots of keypress events are firing - possibly something gets mutated and so the device gets a mangled packet that fails the checksum? That's probably where I'd start looking. Would be a brilliant bug to fix.

Cheers, John

On Sat, 18 Jun 2022 at 03:05, rlarranaga @.***> wrote:

Hello @themillhousegroup https://github.com/themillhousegroup,

Understandable, thanks for the quick answer! What is pending for you to be able to land the binding in the main openHAB codebase? Do you need any help? I might look around and see if I can add this feature.

One thing I am a bit concerned re: binding performance is the delay in command (IR) execution. I was playing around with the binding vs the BroadLink app yesterday, and the app is way more responsive than the binding through openHAB, which is surprising, since it has a road trip to the cloud. When using a tv remote, and commanding the binding through a HABpanel touch screen, I get these random delays when pressing a button, that can take seconds to respond, and they kill the experience Have you experienced anything like this?

Thanks

— Reply to this email directly, view it on GitHub https://github.com/themillhousegroup/openhab2-addons/issues/31#issuecomment-1159074141, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJ7KA62HE7BHHHNX7X2L6DVPSV4BANCNFSM5YHCB7UA . You are receiving this because you were mentioned.Message ID: @.***>

rlarranaga commented 2 years ago

If I wanted to start looking into merging, should clone this repository, run the maven recommendations from OH, and the do a pull request?

themillhousegroup commented 2 years ago

That would be absolutely great. Thankyou so much in advance if you decide to go ahead.

Cheers, John

On Fri, 8 Jul 2022 at 05:50, rlarranaga @.***> wrote:

If I wanted to start looking into merging, should clone this repository, run the maven recommendations from OH, and the do a pull request?

— Reply to this email directly, view it on GitHub https://github.com/themillhousegroup/openhab2-addons/issues/31#issuecomment-1178147527, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJ7KA3ZR6U2TQ2G6JTUWATVS4YJVANCNFSM5YHCB7UA . You are receiving this because you were mentioned.Message ID: @.***>

rlarranaga commented 2 years ago

If have time, i will try to give it a go over the weekend. See how it goes. Would it be better to clone openhab-addons from the openhab repo (3.4) and copy the broadlink directory, and start from there? Thanks

themillhousegroup commented 2 years ago

Yes I’d say so. Thanks again!

On Fri, 8 Jul 2022 at 9:54 pm, rlarranaga @.***> wrote:

If have time, i will try to give it a go over the weekend. See how it goes. Would it be better to clone openhab-addons from the openhab repo (3.4) and copy the broadlink directory, and start from there? Thanks

— Reply to this email directly, view it on GitHub https://github.com/themillhousegroup/openhab2-addons/issues/31#issuecomment-1178902204, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJ7KA22U454QNPNCA7ONJLVTAJIVANCNFSM5YHCB7UA . You are receiving this because you were mentioned.Message ID: @.***>

rlarranaga commented 2 years ago

Hey,

I ran mvn clean install against the current openhab-addons master. there were under 50 coding guideline errors. Most of them were easily fixable, having to do with headers, empty lines, etc.

There is currently only 13 errors left that are divided in 2 categories:

1)The build process complains about junit.test and junit.assert being used (The error message just says "Junit.test should not be used" ). There is 11 instances of this issue throughout the code. and it is a priority 2 error.

2) There is 2 files (ModelMapperTest.java and BroadlinkSocketModel3SHandler.java) where the build process detects two loggers in the same class. I do not see that in the file, so i am wondering if it is a false positive. This is a priority 3 error

It seems to me that the bulk of the remaining work here is replacing junit in the unit tests. I will give it a think if i have time throughout the weekend, but wanted to drop a note in case you have any suggestion that could indicate a fast drop-in replacement.

Besides that, there is a number of warnings in the code, most notably initMocks(java.lang.Object) method being deprecated and Potential null pointer access: this expression has a '@Nullable' type . I am not really looking at these at these point, as i have seen other modules throw the same warnings.

Let me know if you have any thoughts on this. Thanks!

rlarranaga commented 2 years ago

Just as an update, it looks like i managed to switch to Junit 5, and that cleared the 11 instances of the Junit package warning....only the 2 logger issues left.

rlarranaga commented 2 years ago

According to this:

https://github.com/pmd/pmd/issues/3860

The logger warnings are a false positive.....It looks like we might be ready.

@themillhousegroup , how would you prefer to do the pull request?

I cloned the current openHAB add-ons master from openHAB's github. From there, i copied the org.openhab.binding.broadlink directory from the 3.2.0 branch to the clone, changed and made the modifications there.

I see that your add-on's branch for 3.2.0 is quite behind the current openHAB-addons master branch. Do you want to create a new branch for 3.4.0 and i can try do a pull request there? Thanks!

themillhousegroup commented 2 years ago

Sounds like you've done a nice job. Yep I'd definitely target the latest openHAB-addons version - the main trouble I encountered with the PR process was that I got given nice suggestions/improvements and/or requests to use new(er) features available in the core libraries; which I'd do, but it would take some time, by which point I'd be hopelessly behind and facing enormous conflicts just to get back to a mergeable point.

I've created branch openhab-3.4-broadlink-binding - go right ahead!

Cheers, John

On Sun, 10 Jul 2022 at 02:05, rlarranaga @.***> wrote:

According to this:

pmd/pmd#3860 https://github.com/pmd/pmd/issues/3860

The logger warnings are a false positive.....It looks like we might be ready.

@themillhousegroup https://github.com/themillhousegroup , how would you prefer to do the pull request?

I cloned the current openHAB add-ons master from openHAB's github. From there, i copied the org.openhab.binding.broadlink directory from the 3.2.0 branch to the clone, changed and made the modifications there.

I see that your add-on's branch for 3.2.0 is quite behind the current openHAB-addons master branch. Do you want to create a new branch for 3.4.0 and i can try do a pull request there? Thanks!

— Reply to this email directly, view it on GitHub https://github.com/themillhousegroup/openhab2-addons/issues/31#issuecomment-1179566953, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJ7KA34WQGIKCATVMNEO43VTGPMTANCNFSM5YHCB7UA . You are receiving this because you were mentioned.Message ID: @.***>

rlarranaga commented 2 years ago

Hey, Are you sure the new branch is aligned with the current openhab-addons branch? there is bindings missing in it (i.e.: org.openhab.bindings.broadlinkthermostat) Thanks

On Sun, Jul 10, 2022, 7:46 AM themillhousegroup @.***> wrote:

Sounds like you've done a nice job. Yep I'd definitely target the latest openHAB-addons version - the main trouble I encountered with the PR process was that I got given nice suggestions/improvements and/or requests to use new(er) features available in the core libraries; which I'd do, but it would take some time, by which point I'd be hopelessly behind and facing enormous conflicts just to get back to a mergeable point.

I've created branch openhab-3.4-broadlink-binding - go right ahead!

Cheers, John

On Sun, 10 Jul 2022 at 02:05, rlarranaga @.***> wrote:

According to this:

pmd/pmd#3860 https://github.com/pmd/pmd/issues/3860

The logger warnings are a false positive.....It looks like we might be ready.

@themillhousegroup https://github.com/themillhousegroup , how would you prefer to do the pull request?

I cloned the current openHAB add-ons master from openHAB's github. From there, i copied the org.openhab.binding.broadlink directory from the 3.2.0 branch to the clone, changed and made the modifications there.

I see that your add-on's branch for 3.2.0 is quite behind the current openHAB-addons master branch. Do you want to create a new branch for 3.4.0 and i can try do a pull request there? Thanks!

— Reply to this email directly, view it on GitHub < https://github.com/themillhousegroup/openhab2-addons/issues/31#issuecomment-1179566953 , or unsubscribe < https://github.com/notifications/unsubscribe-auth/AAJ7KA34WQGIKCATVMNEO43VTGPMTANCNFSM5YHCB7UA

. You are receiving this because you were mentioned.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/themillhousegroup/openhab2-addons/issues/31#issuecomment-1179703150, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABW5CX7BUKVMIDQE6HICPJ3VTKSXBANCNFSM5YHCB7UA . You are receiving this because you authored the thread.Message ID: @.***>

rlarranaga commented 2 years ago

Disregard my previous comment. Pull request has been submitted. Re: Improvements, suggestions and request, lets hope there isn't many, and that since we are submitting into 3.4.0 Early in the development cycle, we will have enough time to finish any changes.

Thanks!

rlarranaga commented 2 years ago

Hey, Just let me know if I can help with anything else on this. Thanks

rlarranaga commented 1 year ago

Hey @themillhousegroup
Would you prefer me to try and merge this from my repo? Thanks

themillhousegroup commented 1 year ago

Yes please! I suspect it’ll be easier for you too. Best of luck :-)

On Tue, 19 Jul 2022 at 3:40 am, rlarranaga @.***> wrote:

Hey @themillhousegroup https://github.com/themillhousegroup Would you prefer me to try and merge this from my repo? Thanks

— Reply to this email directly, view it on GitHub https://github.com/themillhousegroup/openhab2-addons/issues/31#issuecomment-1187907064, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJ7KA7MRC66EJQPOQCVBP3VUWJKPANCNFSM5YHCB7UA . You are receiving this because you were mentioned.Message ID: @.***>