cmroche / greeclimate

Python package for controlling Gree based minisplit systems
GNU General Public License v3.0
44 stars 22 forks source link

feat: Support GCM encryption for Gree devices #92

Closed cmroche closed 3 months ago

cmroche commented 4 months ago

Adds support for GCM encryption used on some (newer) Gree devices. Since it's not clear yet how to identify these devices by their announce information, the older AES ECB key will be tried first, then attempt the AES GCM key.

Changes:

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 97.81022% with 3 lines in your changes missing coverage. Please review.

Project coverage is 95.74%. Comparing base (4587984) to head (f595eab). Report is 1 commits behind head on master.

Files Patch % Lines
greeclimate/network.py 94.44% 2 Missing :warning:
greeclimate/device.py 96.87% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #92 +/- ## ========================================== + Coverage 94.93% 95.74% +0.81% ========================================== Files 7 8 +1 Lines 651 729 +78 ========================================== + Hits 618 698 +80 + Misses 33 31 -2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

cmroche commented 3 months ago

@engrbm87 Are you comfortable to test this branch with your HA to confirm? While I have a bit more work to do for tests to pass, I think this should be functional now.

engrbm87 commented 3 months ago

@engrbm87 Are you comfortable to test this branch with your HA to confirm? While I have a bit more work to do for tests to pass, I think this should be functional now.

Nice work, I noticed one thing still missing. The initial binding timeout. It is still waiting for 120 seconds.

Another thing I noticed in the bind function, you have included the cipher_type attribute. Can I know the use for this attribute. If I understand correctly, the binding process that happens during discovery will attempt both Cipher versions and store the one that works. I can see that someone might use this library and do the binding manually, in such case he may pass the cipher if he is sure which one is compatible with his device. If that is the case, may I suggest that the function attribute be an instance of the cipher instead of a type. This way, the user can initialize the cipher with/without a key and pass it to the bind function. We will still check if the key provided (assuming this is the device key that the user might have obtained somehow). If provided we can simply update the passed cipher instance and save it as device_cipher.

angelitoo776 commented 3 months ago

Hi!

I was doing some work to include GCM encryption support and I see here it almost done (and much better implemented). There is something that I can help on this?

cmroche commented 3 months ago

Hi!

I was doing some work to include GCM encryption support and I see here it almost done (and much better implemented). There is something that I can help on this?

Testing this PR if you are up for it please. Otherwise I will aim to finish up and merge this week.

cmroche commented 3 months ago

Nice work, I noticed one thing still missing. The initial binding timeout. It is still waiting for 120 seconds.

Yup, wanted to get the cipher stuff out of the way, I'll look at this ridiculous timeout in the next day or two.

cmroche commented 3 months ago

@engrbm87 @angelitoo776 I'm getting ready to release this into main, perhaps you could do some quick testing and confirm. The bind timeout for V2 has been adjusted down to 10 seconds now too.

engrbm87 commented 3 months ago

Hello @cmroche , thanks for the updated bind timeout. One thing I noticed during testing is that the device.update_state() is not awaited correctly. Meaning that after the update request packet is sent we are not waiting for the response before executing other functions. To clarify what is happening in HA. The coordinator is requesting a refresh which triggers a device update. The coordinator is not waiting for the response to be processed and considers that the coordinator data update is completed. When the climate entity is initialized without the response date being processed, the temperature unit conversion is not calculated correctly (getting negative temperature). I am not sure where exactly is the library waiting for the response before finishing the update_state() function and returning.

cmroche commented 3 months ago

sure where exactly is the library waiting for the response before finishing the update_state() function and returning.

This is by design. The goal is to stop the lock-step approach to device comms over the socket. Since networking is inherently unreliable this should lead to a more robust overall design.

That said, the issue is one I have not seen likely because of my ideal network conditions. BUT, I think it would be reasonable to mark the device as unavailable until the first state update, or to block in the binding process until there is an initial state.

cmroche commented 3 months ago

@engrbm87 Does the negative temperature resolve itself once an update is received, or is it persistent? I believe it should resolve once the update comes in, since the correction is computed on the incoming state update from the device.

engrbm87 commented 3 months ago

@engrbm87 Does the negative temperature resolve itself once an update is received, or is it persistent? I believe it should resolve once the update comes in, since the correction is computed on the incoming state update from the device.

I just noticed PR# 121041. I updated my test component based on that and now the temperatures are showing correct. I still think the coordinator should wait until the response is processed during initial setup.

github-actions[bot] commented 3 months ago

:tada: This PR is included in version 2.1.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: