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

Adding support for 42 inch smart tower fan #251

Merged
merged 6 commits into from
Oct 15, 2024
Merged

Conversation

mikeyyuen
Copy link
Contributor

@mikeyyuen mikeyyuen commented Aug 19, 2024

Adds basic support for the 42 inch smart tower fan, it supports more features than I have added here, but I'd like to get feedback on this approach first.

Follows recommendations from #242, tested on the EU version of the fan.

I am open this being done by not using _config_dict to override methods and modes and doing something else (perhaps a new fan subclass, tho I may need assistance in that case) if that is preferred.


Manually tested & confirmed the following work, tests updated to include them as well

  • Normal mode
  • Advanced Sleep Mode
  • Turbo Mode
  • Auto Mode
  • Off Mode
  • Fan Speed Changes
  • Switch Toggle on/off

Adds basic support for the 42 inch smart tower fan. Follows recommendations from webdjoe#242, tested on the EU version of the fan.
@mikeyyuen mikeyyuen marked this pull request as ready for review August 19, 2024 22:33
@webdjoe
Copy link
Owner

webdjoe commented Aug 29, 2024

Thank you for the PR! I know that the API calls are very similar to the VeSyncAirBasev2 class but this being a fan instead of a purifier, can you please create a separate class that inherits from the VeSyncAirBasev2? This way it is easier to maintain changes and edits as new devices with a similar API are added.

@mikeyyuen
Copy link
Contributor Author

No problem, yeh I can change that, give me a few days :)

@webdjoe
Copy link
Owner

webdjoe commented Sep 26, 2024

I created a separate branch if you want to merge the code with yours: https://github.com/webdjoe/pyvesync/tree/towerfan-separate-class

@mikeyyuen
Copy link
Contributor Author

Thank you, that's really helpful!

I'm taking a look, i'll re-run all my manual testing against the my devise and once it's all confirmed working i'll push all the changes here.

@mikeyyuen
Copy link
Contributor Author

@webdjoe I've pulled your class change in and tested it, everything looks good.

There are now pylint validation changes however these are in files that we've not changed here, potentially due to pylint adding them rather than the code being broken, I'm happy to address them if you like. Options are

  1. For now add # pylint: disable=too-many-positional-arguments for the lines that are failing validation
  2. Refactor the 4 functions that are flagged to take objects instead of positional arguments
  3. Change .pylintrc to disable too-many-positional-arguments completely

1 & 3 would be quick, if 2 then lets make a new PR for that change.

@spkesDE
Copy link

spkesDE commented Oct 5, 2024

Hey @mikeyyuen, did you managed to convert the Temperature to °c or °f?

@mikeyyuen
Copy link
Contributor Author

Hey @mikeyyuen, did you managed to convert the Temperature to °c or °f?

I didn't actually touch the temperature in this PR, it's coming back in what ever the default format is (which looks like f multipliled by 10) e.g. below being 65.6f or 18c would make sense for where my device was.

"temperature": 656,

I'm not super familiar with how this library expects to handle temperature but have assumed this is correct because it makes sense.

@spkesDE
Copy link

spkesDE commented Oct 5, 2024

Wow, that is something I didn't think of. I always thought it would be a sensor value that needed to be converted, and I didn't think it would just be Fahrenheit.

Thanks!

As identified in the comments above to improve readability ignore this new-ish lint
@webdjoe
Copy link
Owner

webdjoe commented Oct 15, 2024

@mikeyyuen Thank you for the PR, adding the line to pylintrc is fine.

Since this device is the only one I'm aware of that has temperature, I'd rather not include that functionality. This can be easily done by the end-user

@webdjoe webdjoe merged commit ad0211c into webdjoe:master Oct 15, 2024
5 checks passed
@webdjoe
Copy link
Owner

webdjoe commented Oct 20, 2024

Released in 2.1.13

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

Successfully merging this pull request may close these issues.

3 participants