[mideaac] Initial Contribution

[apella12]

new midea ac binding - Had to delete old one due to rebase malfunction (operator error)
Signed-off-by: Bob Eckhoff [email protected]

I’d like to put this out for new binding review.
This binding was started by the original author as a forum topic, but has since had other contributors. I picked it up when I installed a mini-split AC that used the Midea protocol earlier this year, originally to allow for longer polling frequencies. Then as a retirement challenge (no formal java training), decided to attempt to clean it up for official status. I cleaned up the summary report from over 150 items and believe I conformed to current developer guidelines (Java docs, naming, tests, etc.). Along the way added some additional functionality and corrected some code issues.
The discovery and security classes remain largely unchanged from the original author (who I haven’t been able to contact), and work well. The other classes I understand pretty well at this point and hopefully can address any concerns.
As a note, to conform to the guidelines there are breaking changes in naming from the version I published on the forum thread.

[lsiepel]

Thank you for creating this PR to contribute this binding.
To manage expectations; review capacity is somewhat limited so don’t expect an immediate full review. To speed up the process you can already perform a self-review by checking this list: https://github.com/openhab/openhab-addons/wiki/Review-Checklist

Have not looked at this PR yet, it may be of very good quality. But common review comments for new bindings are about thing structure (xml), naming convention and log levels. So you might double check those areas.

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/best-air-conditioner-for-openhab-integration-in-2023/145852/65

[lsiepel]

Thank you for contributing. This seems to be a binding that has been used for a while. I left some initial comments to specific parts. Have not covered everything yet as i think some comments woudl otherwise be stacked. While looking at the comments you might also look at the bindingt structure, the discovery/handler/security folders seem to have some kind of overlap.
Personally i prefer to have a handler folder, and a api or servcie folder with everything related to the remote service. Usually the DTO-classes go in a api/dto subfolder. This is meant to give some guidance and not necesairly as a strict rule.

[apella12]

I still have several suppress "null" generally related to this variable on line 677 in the MideaACHandler config = getConfigAs(MideaACConfiguration.class); for instance near the bottom (line 1455) if I remove the suppress null, I get the yellow line under long frequency = config.pollingTime; Also how do you get the file extracts with the line numbers?

I think I will add a commit to try to close off some of the questions (narrow down the problems)

Edit: Last night, with your insight that the Suppress("null") doesn't affect the compiler, I'll take them all out and see what happens. As noted above, I have struggled, and it is like the carnival game "Whack-a-mole". Fixing one causes another to popup. The version committed last night has no warnings during the compile, so it is a good base to start from.

[apella12]

I'm going to need a few days to do some testing with all the changes to address some of the null warnings.

[apella12]

Addressed most suppressed null issues. In VSC the remaining ones are marked as "info" (not "warning') or not highlighted at all. However, in the compile process they are still shown as warnings. I tried a number of things (and some AI suggestions), but nothing worked. Also made some changes to address the first review topics more completely.

[lsiepel]

Thank you for addressing most comments. There are some new comments, made some good progress and allmost covered all files. I expect 1 or 2 more passes left, also depends on the follow up offcourse ;-)

[lsiepel]

Not sure if this makes sense, but this could be: "Not set (pre V3)" or something similar to indicate this is not used.

[apella12]

I thought I responded to this. There a couple of concerns, one is that "" is the default to decipher the discovery message. And to get the AC on the LAN one of the apps needed to be used, so a V2 user could either leave blank after discovery or add one of the three defined options. I think it would be confusing to have a default.

[lsiepel]

Not sure if i understand it. But the value can still remain empty, but the option needs a label so that it works nice in the UI.
Example:
<option value="">Not set (V2) or auto detect</option>

[apella12]

Ok misunderstood. Just the description

[apella12]

I think the only major item left is the ConnectionManager class, but let me know

[apella12]

Experimenting with extracting the ConnectionManager as a separate class. The first issue (and in a lot of places) appears that it was using the fact that MideaACHandler extended the BaseThingHandler. All the updateStatus and editConfiguration (for trouble with connecting or missing token/key don't work. I tried mideaACHandler.updateStatus(ThingStatus.OFFLINE, mideaACHandler.getDetail(), message); but it says it is not visible from the BaseThingHandler (BTH) & if I try to extend the BTH it wants a handleCommand() and initialize(). I have the BTH imported, but that doesn't seem to be enough.

EDIT: Another option is to eliminate the ConnectionManager class completely. I've never been a fan (it was in the code when I got it). In essence the poll could be replaced with a simple cron rule using a channel refresh (I've done this test- rule "Get readings from AC" when Time cron "0 * * ? * * *" then MBR_Midea_AC_Indoor_temperature.sendCommand ("REFRESH") end). That way it is all linear. Poll or Command --> Connect -->write -->response -->update channels. The issues would be 1) class is still long code-wise (my gut says that is your concern, not the separation) 2) either this is completely left out of the binding (probably a hassle for users) or need some way to trigger a refresh on the handleCommand() method (Required with extended BTH anyway) without knowing the specific channelUID. Could be easier and more straightforward. WDYT?

[lsiepel]

IT: Another option is to eliminate the ConnectionManager class completely. I've never been a fan (it was in the code when I got it). In essence the poll could be replaced with a simple cron rule using a channel refresh (I've done this test- rule "Get readings from AC" when Time cron "0 * * ? * * *" then MBR_Midea_AC_Indoor_temperature.sendCommand ("REFRESH") end). That way it is all linear. Poll or Command --> Connect -->write -->response -->update channels. The issues would be 1) class is still long code-wise (my gut says that is your concern, not the separation) 2) either this is completely left out of the binding (probably a hassle for users) or need some way to trigger a refresh on the handleCommand() method (Required with extended BTH anyway) without knowing the specific channelUID. Could be easier and more straightforward. W

The ConnectionManager should not know anything about the Thing or its state. It should handle the connection and nothing more.

A typicall pattern is that the ConnectionManager keeps track of the underlying connection, has jobs to keep alive, might have caching machnisms etc. But it only exposes a few public methods that enable to set or get data. Usually this ConnectionManager informs the caller by using custom exceptions like AuthenticationException or TransientNetworkErrorException when one of the public methods is unable to return a value or perform a coimmand. The caller (in this case the Handler) can route that to updateStatus() easily.

[apella12]

Out of the gate the binding no longer works. I put back part of the old initialize() and got the configs back, but the cloud retrieval of token and key doesn't work. Looks like the parts were commented out. I tried to move this back to the initialize() (I think it makes more sense to get everything correct first) but is still failed. I committed my work in progress, but probably need to drop this. We'll see how I feel in the morning. I'll convert to draft since it is no longer working.

[lsiepel]

Out of the gate the binding no longer works. I put back part of the old initialize() and got the configs back, but the cloud retrieval of token and key doesn't work. Looks like the parts were commented out. I tried to move this back to the initialize() (I think it makes more sense to get everything correct first) but is still failed. I committed my work in progress, but probably need to drop this. We'll see how I feel in the morning. I'll convert to draft since it is no longer working.

Yes this was part of the TODO I mentioned. Handling the token should be done from the connection manager, also have to look how it works together with the discovery part. I’ll look into it today.

[apella12]

My thought was to get all the configurations set in the initialize() method, not do part there and the other in the connectionManager.

[apella12]

Thanks for all your time. Sorry it couldn't work out. You are obviously a skilled developer. I'll update the forum version with the changes up to closing this PR.

[lsiepel]

Thanks for all your time. Sorry it couldn't work out. You are obviously a skilled developer. I'll update the forum version with the changes up to closing this PR.

There is no need to rush, why close it? There where some things on the TODO list that i can fix. Only needed a little more time as i also have other things todo.
What if i try to fix it and i'll just send you the jar so you can test?

[apella12]

This was a test to see if I could update the original code to pass OH official review. I had looked through most of the requirements before starting, but it appears the separation of the ConnectionManager from the MideaACHandler was way beyond my ability. In a nutshell I am unsure I can support your revisions if any problems arise.

That said I do think the OH community would benefit from this binding. There is quite an active effort with HA. I would be happy to test anything you develop, but the support issue still looms. I was hoping there would be a much simpler way to update the Thing status in the ConnectionManager.