Skip to content
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

"On the Fly" code generation #31

Open
rlarranaga opened this issue Jun 8, 2022 · 16 comments
Open

"On the Fly" code generation #31

rlarranaga opened this issue Jun 8, 2022 · 16 comments

Comments

@rlarranaga
Copy link

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

themillhousegroup commented Jun 21, 2022 via email

@rlarranaga
Copy link
Author

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
Copy link
Owner

themillhousegroup commented Jul 8, 2022 via email

@rlarranaga
Copy link
Author

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
Copy link
Owner

themillhousegroup commented Jul 8, 2022 via email

@rlarranaga
Copy link
Author

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.

  1. 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
Copy link
Author

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
Copy link
Author

According to this:

pmd/pmd#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
Copy link
Owner

themillhousegroup commented Jul 10, 2022 via email

@rlarranaga
Copy link
Author

rlarranaga commented Jul 10, 2022 via email

@rlarranaga
Copy link
Author

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
Copy link
Author

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

@rlarranaga
Copy link
Author

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

@themillhousegroup
Copy link
Owner

themillhousegroup commented Jul 18, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@themillhousegroup @rlarranaga