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

Added support for Roidmi Eve #1072

Merged
merged 4 commits into from
Jul 12, 2021
Merged

Conversation

martin9000andersen
Copy link
Contributor

@martin9000andersen martin9000andersen commented Jun 12, 2021

I have added support for robot Roidmi Eve (Robot vacuum cleaner).

Many status values are implemented and many features are implemented e.g.:
start-clean, stop, go-home, set_sound_volume, set_cleaning_mode, set_sweep_type, set_work_station_freq, start_dust, etc.

I do not know the syntax to activate cleaning of specific rooms, any input on how to make the function "start_room_sweep_unknown" work is highly appreciated.

Copy link
Owner

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I did a first round of reviews and left some specific comments inline, but on generla level:

  • Please add descriptive docstrings to all properties and methods belonging to the public API
  • Remove commented out code (except for the variable mappings, in case someone wants to look into those later)
  • Please add (at least) is_on, is_paused, got_error from https://github.com/rytilahti/python-miio/blob/master/miio/vacuumcontainers.py#L125 - the long term goal is to make all vacuum implementations to have a common interface, so this will be useful for that.

miio/device.py Outdated Show resolved Hide resolved
miio/roidmivacuum_miot.py Outdated Show resolved Hide resolved
miio/roidmivacuum_miot.py Outdated Show resolved Hide resolved
miio/roidmivacuum_miot.py Outdated Show resolved Hide resolved
miio/roidmivacuum_miot.py Show resolved Hide resolved
miio/roidmivacuum_miot.py Outdated Show resolved Hide resolved
miio/roidmivacuum_miot.py Outdated Show resolved Hide resolved
miio/roidmivacuum_miot.py Outdated Show resolved Hide resolved
miio/roidmivacuum_miot.py Outdated Show resolved Hide resolved
miio/roidmivacuum_miot.py Outdated Show resolved Hide resolved
@martin9000andersen
Copy link
Contributor Author

Thank you very much for your comments. I really like your long term goal of a common interface.
I will have a look at it in the coming days.

@martin9000andersen
Copy link
Contributor Author

I have updated the code to be more like vacuumcontainers.py (before it was closer to dreamevacuum_miot.py).

@codecov-commenter
Copy link

codecov-commenter commented Jul 4, 2021

Codecov Report

Attention: Patch coverage is 89.48949% with 35 lines in your changes missing coverage. Please review.

Project coverage is 76.07%. Comparing base (b9393f3) to head (8e2cd89).
Report is 374 commits behind head on master.

Files Patch % Lines
miio/roidmivacuum_miot.py 89.45% 35 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1072      +/-   ##
==========================================
+ Coverage   75.43%   76.07%   +0.64%     
==========================================
  Files          74       75       +1     
  Lines        8372     8722     +350     
  Branches      743      747       +4     
==========================================
+ Hits         6315     6635     +320     
- Misses       1875     1905      +30     
  Partials      182      182              

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

Copy link
Owner

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some more comments, I think it's good to go after this round but be aware that the API will change for the next major release to allow consolidating the common functionalities among all supported vacuums :-)

While doing the review I did some googling and started to wonder if this PR is for "Roidmi Eve Plus" or is there also a version without the "plus"? If that's the same one, this might be one option when it's time to upgrade away from my gen1 rockrobo.. What's your opinion about the device?

miio/roidmivacuum_miot.py Outdated Show resolved Hide resolved
miio/roidmivacuum_miot.py Show resolved Hide resolved
miio/roidmivacuum_miot.py Outdated Show resolved Hide resolved
miio/roidmivacuum_miot.py Outdated Show resolved Hide resolved
miio/roidmivacuum_miot.py Outdated Show resolved Hide resolved
miio/roidmivacuum_miot.py Outdated Show resolved Hide resolved
miio/tests/test_roidmivacuum_miot.py Outdated Show resolved Hide resolved
miio/tests/test_roidmivacuum_miot.py Outdated Show resolved Hide resolved
miio/tests/test_roidmivacuum_miot.py Outdated Show resolved Hide resolved
miio/roidmivacuum_miot.py Outdated Show resolved Hide resolved
@martin9000andersen
Copy link
Contributor Author

I added some more comments, I think it's good to go after this round but be aware that the API will change for the next major release to allow consolidating the common functionalities among all supported vacuums :-)

Thank you for the comments. I have added comments on a few points, the rest of the points look very clear and I hope to get time to solve them within a week.

@martin9000andersen
Copy link
Contributor Author

While doing the review I did some googling and started to wonder if this PR is for "Roidmi Eve Plus" or is there also a version without the "plus"? If that's the same one, this might be one option when it's time to upgrade away from my gen1 rockrobo.. What's your opinion about the device?

I have the "PLUS" model. The manual covers both models. The robot is the same, the "PLUS" is about the base. The "PLUS" base is with dust-collection. The manual shows a simple base for the non-PLUS model (but I have not seen the non-PLUS version for sale)
The miio protocol has a command to ask about "station-type", that is an int, for me this return 102, and I guess that the simple base will return a different number.

I am very satisfied with Roidmi Eve Plus. The largest draw back I see is missing Home assistant support, but with this pull request and the comming uniform vaccum interface, I think that will be solved.

@martin9000andersen
Copy link
Contributor Author

I have solved (most of) your comments, but also added a few extra functions.
Let me know if there are more changes you like me to make before merging.

miio/roidmivacuum_miot.py Outdated Show resolved Hide resolved
miio/roidmivacuum_miot.py Outdated Show resolved Hide resolved
miio/roidmivacuum_miot.py Outdated Show resolved Hide resolved
miio/roidmivacuum_miot.py Outdated Show resolved Hide resolved
miio/roidmivacuum_miot.py Outdated Show resolved Hide resolved
miio/roidmivacuum_miot.py Outdated Show resolved Hide resolved
miio/roidmivacuum_miot.py Outdated Show resolved Hide resolved
miio/roidmivacuum_miot.py Outdated Show resolved Hide resolved
miio/roidmivacuum_miot.py Outdated Show resolved Hide resolved
Copy link
Owner

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All set, thanks @martin9000andersen!

@rytilahti rytilahti merged commit 1d5536d into rytilahti:master Jul 12, 2021
@kali876
Copy link

kali876 commented Apr 16, 2022

I see the subject is a bit frozen :D
I do not hide that it needs the functionality of vacuuming a specific zone.
If you know how to get mapping for zone vacuuming - I can help :)
I can follow all instructions as long as I get them :)
thx

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

Successfully merging this pull request may close these issues.

4 participants