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

Add support Mi Robot Vacuum-Mop 2 Pro (ijai.vacuum.v3) #1497

Merged
merged 15 commits into from
Oct 6, 2022

Conversation

k402xxxcenxxx
Copy link
Contributor

@k402xxxcenxxx k402xxxcenxxx commented Aug 14, 2022

Add support Mi Robot Vacuum-Mop 2 Pro (ijai.vacuum.v3)

Fixes #1388 fixes #1385

@codecov-commenter
Copy link

codecov-commenter commented Aug 14, 2022

Codecov Report

Merging #1497 (33d5905) into master (f4abd59) will increase coverage by 0.11%.
The diff coverage is 89.52%.

@@            Coverage Diff             @@
##           master    #1497      +/-   ##
==========================================
+ Coverage   80.82%   80.93%   +0.11%     
==========================================
  Files         151      153       +2     
  Lines       14798    14989     +191     
  Branches     1696     3678    +1982     
==========================================
+ Hits        11960    12131     +171     
- Misses       2591     2610      +19     
- Partials      247      248       +1     
Impacted Files Coverage Δ
miio/integrations/vacuum/mijia/pro2vacuum.py 87.87% <87.87%> (ø)
miio/integrations/vacuum/mijia/__init__.py 100.00% <100.00%> (ø)
...integrations/vacuum/mijia/tests/test_pro2vacuum.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@k402xxxcenxxx
Copy link
Contributor Author

Add support for basic actions:

  • Go Home
  • Start Cleaning
  • Stop Cleaning
  • Set fan speed
  • Get status

The code structure is based on miio\integrations\vacuum\mijia\g1vacuum.py.

Currently not sure the error code mapping, so leave the mapping empty

@k402xxxcenxxx k402xxxcenxxx marked this pull request as ready for review August 26, 2022 16:54
@rytilahti
Copy link
Owner

Sorry, I haven't had time to review this yet, one question I had was whether the implementation differs that much from the g1 to require a separate implementation file?

It is not really a big issue, especially with the upcoming introspectable interfaces (see #1495 and https://python-miio.readthedocs.io/en/latest/contributing.html#status-containers), but if the changes are minor we could avoid code duplication.

@k402xxxcenxxx
Copy link
Contributor Author

Sorry, I haven't had time to review this yet, one question I had was whether the implementation differs that much from the g1 to require a separate implementation file?

It is not really a big issue, especially with the upcoming introspectable interfaces (see #1495 and https://python-miio.readthedocs.io/en/latest/contributing.html#status-containers), but if the changes are minor we could avoid code duplication.

That is a good question!

I have tried integrating ijai.vacuum.v3(pro2) into g1, but finally decided to create a new file.
They have some difference:

  1. G1 has ChargingStatus but pro2 combines charging status with overall status.
  2. G1 has find action but pro2 doesn't
  3. G1 separates main/side brush to different SIID, but pro2 collects these states into sweeping state
  4. pro2 has more map controls, but G1 only has map info

These differences suggest that they may not have been designed by the same product line. So I decided to split them into two files. But this is the first time I've written this kind of script, so I'm not super sure that my choices are correct. If you still suggest me to integrate them into one file after reading my rationale, I will try to do so.

Thanks again for your easy-to-use codebase!

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 @k402xxxcenxxx, I added some comments inline to improve it before it's ready to be merged!

"""Response (MIoT format) of a Mi Robot Vacuum-Mop 2 Pro (ijai.vacuum.v3)

[
{'did': 'battery', 'siid': 3, 'piid': 1, 'code': 0, 'value': 100},
Copy link
Owner

Choose a reason for hiding this comment

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

Could you please add tests for the property getters using the example payload to make sure the return values and their types are as expected? Just in case this gets refactored in the future :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @rytilahti, I tried adding a state payload test. However, I'm not familiar with writing pytest. Can you help me review it first? Then I can write the following tests based on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will check the test code first. I forget to run test before push.

@rytilahti
Copy link
Owner

I have tried integrating ijai.vacuum.v3(pro2) into g1, but finally decided to create a new file. They have some difference:

The reasoning sounds good to me, thanks for trying to integrate the implementations! No need to try to force different product lines under a single integration, especially considering the ongoing work to allow downstream users to programmatically find out about available information.

These differences suggest that they may not have been designed by the same product line. So I decided to split them into two files. But this is the first time I've written this kind of script, so I'm not super sure that my choices are correct. If you still suggest me to integrate them into one file after reading my rationale, I will try to do so.

Splitting the implementations sounds good to me. One of the reasons I have tried to "force" support for different devices has been to make the interfaces somewhat consistent. This was due to structural problems in this library that are now being worked on as mentioned in my other comment.

I'm glad that you think the code base was easy to work on, I hope it will become even easier in the future :-)

@k402xxxcenxxx
Copy link
Contributor Author

@rytilahti thanks for reviewing. I will find time to improve it.

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.

No need to define the unit for timedeltas for now, otherwise this is good to go, thanks!

return self.data["main_brush_life_level"]

@property
@sensor("Main Brush Life Time Left", unit="hr")
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
@sensor("Main Brush Life Time Left", unit="hr")
@sensor("Main Brush Life Time Left")

return self.data["side_brush_life_level"]

@property
@sensor("Side Brush Life Time Left", unit="hr")
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
@sensor("Side Brush Life Time Left", unit="hr")
@sensor("Side Brush Life Time Left")

return self.data["filter_life_level"]

@property
@sensor("Filter Life Time Left", unit="hr")
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
@sensor("Filter Life Time Left", unit="hr")
@sensor("Filter Life Time Left")

return self.data["mop_life_level"]

@property
@sensor("Mop Life Time Left", unit="hr")
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
@sensor("Mop Life Time Left", unit="hr")
@sensor("Mop Life Time Left")

return self.data["clean_area"]

@property
@sensor("Last Clean Time", unit="mins", icon="mdi:timer-sand")
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
@sensor("Last Clean Time", unit="mins", icon="mdi:timer-sand")
@sensor("Last Clean Time", icon="mdi:timer-sand")

Comment on lines 93 to 95
class RepeatState(Enum):
Off = False
On = True
Copy link
Owner

@rytilahti rytilahti Sep 24, 2022

Choose a reason for hiding this comment

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

I just briefly checked if all the booleans are defined as such and saw this. This is not used anywhere and should be removed now?

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.

LGTM, thanks again! 👍

@slecram
Copy link

slecram commented Dec 22, 2023

@rytilahti Hi! When there will be a new release of python-miio to support die mop 2 pro? I'd like to implement my robot into homeassistant, but this will only works with a new release.

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.

Unsupported model ijai.vacuum.v3 Mi Robot Vacuum-Mop 2 Pro (ijai.vacuum.v3)
4 participants