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.

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

feat: Support GCM encryption for Gree devices #92

Merged
merged 15 commits into from
Aug 5, 2024
Merged

Conversation

cmroche
Copy link
Owner

@cmroche cmroche commented Jul 6, 2024

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:

  • Finish Cipher AES-GCM support
  • Reduce 120 timeout on binding wait only
  • Add tests
  • Verify in Home Assistant

Copy link

codecov bot commented Jul 6, 2024

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 ⚠️
greeclimate/device.py 96.87% 1 Missing ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

greeclimate/device.py Outdated Show resolved Hide resolved
greeclimate/network.py Outdated Show resolved Hide resolved
greeclimate/device.py Outdated Show resolved Hide resolved
@cmroche
Copy link
Owner Author

cmroche commented Jul 13, 2024

@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
Copy link
Contributor

engrbm87 commented Jul 14, 2024

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

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

cmroche commented Jul 29, 2024

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 cmroche closed this Jul 29, 2024
@cmroche cmroche reopened this Jul 29, 2024
@cmroche
Copy link
Owner Author

cmroche commented Jul 29, 2024

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 cmroche marked this pull request as ready for review August 3, 2024 21:59
@cmroche
Copy link
Owner Author

cmroche commented Aug 3, 2024

@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
Copy link
Contributor

engrbm87 commented Aug 5, 2024

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

cmroche commented Aug 5, 2024

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

cmroche commented Aug 5, 2024

@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
Copy link
Contributor

engrbm87 commented Aug 5, 2024

@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.

@cmroche cmroche merged commit 7122cdd into master Aug 5, 2024
6 checks passed
@cmroche cmroche deleted the gcm-encryption branch August 5, 2024 21:24
github-actions bot pushed a commit that referenced this pull request Aug 5, 2024
# [2.1.0](v2.0.0...v2.1.0) (2024-08-05)

### Features

* Support GCM encryption for Gree devices ([#92](#92)) ([7122cdd](7122cdd))
Copy link

github-actions bot commented Aug 5, 2024

🎉 This PR is included in version 2.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants