[mideaac] Initial contribution

[apella12]

This is a second attempt. The first attempt is closed #17395. Essentially all of the previous comments have been addressed. The main new change is the separation of the connection manager and the MideaACHandler, (largely rewritten by @lsiepel) these two files are basically new. I have worked and tested with this version (made some modifications) for several weeks and it seems to work as well as the first attempt now. Also there were a number of new supporting files added by @lsiepel.

For reference the previous intro is here.
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]

This is not forgotten, the time I have to spare is consumed by the ZUI binding at the moment. So don’t wait for me.

[apella12]

@lsiepel Well I'm not really waiting. I'm working on other things too. There shouldn't be much to review after you rewrote the connection manager. I did a few things to make it more robust to run without the A/C connected to the cloud and fixed some things that you left hanging. I have had it polling for months without issues. It is not exactly A/C season, so no one is exactly clamoring for an update. Hopefully you or someone else can have a look in a couple of months as we get towards spring. EDIT: I did update the version to OH5 and change the date to 2025 in the headers

[lsiepel]

Forgot about this one, sorry. I might have some time to review in the next days.
It was tested and all works as expected?

[apella12]

Yes, it is working fine. I have been using it since the last commit. Decided to rebase today in case something in the OH5.0 causes a problem, but it passed checks

[lsiepel]

Thanks for not leaving this PR/binding. It has improved, i also see more room for improvement. Besides some new minor issues that i probably missed before, i think the DTO and connection structure can benefit from some more work and thinking.

[lsiepel]

Marked some previous comments as resolved. Found some new ones, but nothing big. Let's slowly continue and narrow the remaining issues down and fix them together.

[apella12]

In no particular order, 1) I'm not sure how to use the OH standard conversions and where to put the .toLowerCase since I do not see that in the documentation. 2) The exception in the retry I'm trying to address is simply a timeout. 3) Elsewhere, I was trying to avoid breaking something that I fixed earlier would cause me to go back and fix again. It has been months and I'm not recalling all the issues I had getting to the fix. 4) I also attached a log of the DTO actions, it appears to me everything is needed. The only logic is to pull from the cloud responses to compose the next message.

EDIT: Regaring other comments;
First about the blocking method in the initialize(). The issue is that V.3 discovery is two steps. First step is to get the IP, port, deviceId and set the defaults. The V.3 initialization starts but hits a wall until the cloud provider, email and password are entered on the UI page (V.2 OK). That springs the method to get the token and key. We could force textual configuration and the use of a third party platform to obtain the token and key, but that doesn’t seem to fit the OH model.

Second I retested a change to remind myself of the problem. I dropped the return after getTokenKeyCloud(cloudProvider); (line 218) and the initialize() after logger.debug("Token and Key obtained from cloud, saving, back to initialize"); (line 372). The token and key are filled in on the UI, the connection manager is started, but chokes on the authorization. Resaving the thing with a change corrects the problem (I just added a location), but that is messy. With the current code everything works seamlessly when deleting the token and key when the cloud provider, password and email are in place.

[apella12]

Figured out how to leverage the OH core util hex/byte and separated the socket timeout exception from anything else. Seems to still work, but with the magnitude of changes am testing overnight. Have tested scan, retrieve key/token, normal polling and temperature changes. Had one timeout exception that worked as expected. 9 seconds = 4 for timeout and 5 delay for next try.

2025-02-21 17:19:56.320 [DEBUG] [nternal.connection.ConnectionManager] - Disconnecting from device at 192.168.0.246
2025-02-21 17:20:05.326 [DEBUG] [nternal.connection.ConnectionManager] - Socket retry count 1, Socket timeout connecting to 192.168.0.246: Connect timed out
2025-02-21 17:20:05.329 [DEBUG] [nternal.connection.ConnectionManager] - Device at IP: 192.168.0.246 requires authentication, going to authenticate

Normal connection is less that 10ms.

2025-02-21 17:09:53.751 [DEBUG] [nternal.connection.ConnectionManager] - Disconnecting from device at 192.168.0.246
2025-02-21 17:09:53.757 [DEBUG] [nternal.connection.ConnectionManager] - Device at IP: 192.168.0.246 requires authentication, going to authenticate

[lsiepel]

Ok, so i'll try to reboot this review proces, where are we with this PR? Previously we tried to seperate the handler and the connection manager, how did that work out?

[Copilot]

Pull Request Overview

Initial contribution of the MideaAC binding, introducing the core implementation, tests, and OpenHAB metadata for discovery, configuration, and channels.

• Added new binding module and registered it in the parent POM.
• Implemented security, packet/response handling, capability parsing, connection manager, handler logic, and cloud integration.
• Provided comprehensive unit tests and updated thing-types, i18n, and addon metadata.

Reviewed Changes

Copilot reviewed 46 out of 46 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
bundles/pom.xml	Register org.openhab.binding.mideaac module
src/main/java/.../internal	Add core binding implementation (handler, security, parser, packet, response, energy)
src/test/java/.../internal	Add unit tests for security, parser, response, energy, discovery, cloud, configuration
src/main/resources/OH-INF/thing	Define thing-types (channels, configuration)
src/main/resources/OH-INF/i18n	Provide localization properties
src/main/resources/OH-INF/addon	Add addon metadata for discovery and binding

Comments suppressed due to low confidence (2)

bundles/org.openhab.binding.mideaac/src/main/resources/OH-INF/thing/thing-types.xml:275

• The pattern="%d%%" implies a percentage but this channel reports energy in kWh; update to a more appropriate numeric or unit-aware pattern (e.g., %.2f %unit%).

		<state readOnly="true" pattern="%d%%"/>

bundles/org.openhab.binding.mideaac/src/test/java/org/openhab/binding/mideaac/internal/handler/capabilities/CapabilityParserTest.java:152

• [nitpick] Method name testParseWithtemperature does not follow camelCase; rename to testParseWithTemperature for consistency.

    void testParseWithtemperature() {

[apella12]

@lsiepel I didn't see your reopening comment. Must not have been notified (or thought it was related to my ZUI nofications)...

To recap; you wrote code to separate the connection manager and the MideaAC handler and I got it working again and addressed all the comments from February 21 and before. Since then I extracted several features from the HA version (and with the help of Copilot in VSC converted them to java). Also there have been a couple of new users of the forum version that had new messages that I incorporated using some Lua code references. Also because of the new features, reworked some part of the code. All that is complete now, but the binding is 25% longer. I was just making a final pass to ensure the ReadMe matched with the additional capabilities.

[apella12]

@lsiepel Since you are busy with the zwave-js, possibly someone else can review this. Coming up on 1 year anniversary.

[lsiepel]

Sorry i had you waiting this long. I got distracted many times and needed some focus time to finish this review.
Finished all files. The handler might need some refactoring to create logical units that do a strict amount of work. especially around the initialize method and how it is called from other places.

Anyway, i think that depending on the follow-up there might be one more review, but as i have covered everything, it will not take as long as before.

[lsiepel]

Please use i18n custom keys when setting the thing state. This would make it possible to add translations.
For reference: https://next.openhab.org/docs/developer/utils/i18n.html#using-custom-keys

(applies to all occurences of updateStatus with a thingstatusdetail)

[lsiepel]

To be fixed

[apella12]

Just to check. I have run mvn i18n:generate-default-translations -pl :org.openhab.binding.mideaac, but need to manually add the thing status details in the MideaACHandler to that file? First follow-up question, what about when the text is e.getMessage() which apparently could be null?

Second followup question what about duplicates with different messages? Do I need to massage them to be different?

# thing status descriptions

unknown.configuration_pending = Configuration not valid.
unknown.configuration_pending = Configuration missing, discovery needed. Discovering...
offline.configuration_error = Discovery failure. Check IP Address.
offline.configuration_error = Invalid MideaAC config. Check IP Address.

[lsiepel]

They keys are custom. So you are also able to use

offline.configuration_error_discovery are whatever you think is right. Obvously the key in the properties file needs to match the key in the updatestate string.

[apella12]

I think I have this sorted out. With the limited slate of states and descriptions did some consolidation and removed some unnecessary ones. I have assumed that e.getMessage() override the content in the i18n.

[lsiepel]

A follow up to the previous fixes.

[lsiepel]

To be fixed

[lsiepel]

I think almost all comments are addressed, my main concern is the initialize method.
The openHAB model seems stable, that is one of the most important parts.

[lsiepel]

This initialize method needs some refactoring. I'm happy to help, but you should also need to understand what is going on here.

The initialize() method currently performs multiple responsibilities in one place:

• Validating configuration
• Attempting discovery or token/key retrieval
• Establishing device communication
• Fetching capabilities
• Starting scheduled background tasks

This makes the method long and a bit difficult to follow. The logic is solid, but readability and maintainability can be improved by splitting the flow into smaller, clearly named helper methods.

Together with early returns the method would be short, straight forward and simple to adapt to prevent multiple status updates. in a row.

[apella12]

TBH last time you helped I almost decided to give up.
All these things need to be done and with the async threads it is happening fast.
I'd like to hold on this for now.

[lsiepel]

Íf i have time in the next days i'll create a PR against your branch. It is mainly re-ordering the code, so i guess this won;t be difficult without many device details.

[apella12]

I enhanced the documentation in the code to hopefully avoid this, but whatever.

[lsiepel]

Created the PR here: apella12#2

Please test and see if all is still working as expected, if so, merge it and we can finish this PR.

[apella12]

Ran some tests to exercise the initialization. Seems to work as before. (Couldn't resist, could you?)
Edit; Personally, I prefer grouped code as easier to follow, rather than having to jump around to find the linked methods. (Ok this is false, now where do I find what happens next?) Also about 100 more lines.

[lsiepel]

They keys are custom. So you are also able to use

offline.configuration_error_discovery are whatever you think is right. Obvously the key in the properties file needs to match the key in the updatestate string.

[apella12]

I'll have to tackle the i18n tomorrow. Thanks

[lsiepel]

I think we are ready. @apella12 do you need some additional time to test / verify different scenario's after latest changes?

[lsiepel]

Thanks, LGTM

[apella12]

I have the latest version running. No more testing really needed IMO. Also, the underlying code looked the same, materially, just dispersed around the MideaACHandler class so the initialize() just had method calls. Thanks for approving !

[lsiepel]

o is here.
I’d like to put this out for new

The thingstatus handling did change a bit but the logic itself was not changed as that was solid. Anyway.

Now, you could add the binding's logo to the openHAB website. See https://www.openhab.org/docs/developer/addons/#add-your-add-on-s-logo-to-the-openhab-website-and-the-ui