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 mop dryer add-on of the S7 MaxV Ultra station #1621

Merged
merged 5 commits into from
Jan 1, 2023

Conversation

jpbede
Copy link
Contributor

@jpbede jpbede commented Dec 7, 2022

The S7 MaxV Ultra station (and AFAIK also the S7 Ultra station) has an optional mop dryer add-on. Based on reverse engineering of the Mi Home app plugin code, I added support for this.

Currently this code is partially untested as I am waiting for my add-on to be delivered. Delivery is expected around 12/19.

But I appreciate code review and feedback already :)

@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2022

Codecov Report

Merging #1621 (1ee2ebc) into master (cdca98f) will increase coverage by 0.08%.
The diff coverage is 98.85%.

❗ Current head 1ee2ebc differs from pull request most recent head 32f820d. Consider uploading reports for the commit 32f820d to get more accurate results

@@            Coverage Diff             @@
##           master    #1621      +/-   ##
==========================================
+ Coverage   80.26%   80.35%   +0.08%     
==========================================
  Files         161      161              
  Lines       15907    15983      +76     
  Branches     3577     3552      -25     
==========================================
+ Hits        12768    12843      +75     
- Misses       2876     2884       +8     
+ Partials      263      256       -7     
Impacted Files Coverage Δ
...o/integrations/vacuum/roborock/vacuumcontainers.py 84.90% <95.65%> (+0.64%) ⬆️
.../integrations/vacuum/roborock/tests/test_vacuum.py 98.62% <100.00%> (+0.20%) ⬆️
miio/integrations/vacuum/roborock/vacuum.py 63.30% <100.00%> (+1.83%) ⬆️
miio/devtools/__init__.py 0.00% <0.00%> (ø)
miio/devtools/pcapparser.py 0.00% <0.00%> (ø)
miio/devtools/propertytester.py 0.00% <0.00%> (ø)
miio/devtools/simulators/miiosimulator.py 0.00% <0.00%> (ø)
miio/devtools/simulators/miotsimulator.py 0.00% <0.00%> (ø)

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

@jpbede jpbede marked this pull request as ready for review December 8, 2022 06:01
@jpbede jpbede force-pushed the feat/s7maxv-mop-dryer branch from 3134f5a to affc463 Compare December 8, 2022 06:11
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.

It would be great to get it also tested on a real device, but looks good on the surface to me, thanks for the PR @jpbede!

@@ -443,10 +444,63 @@ def test_cleaning_brush_cleaned_count(self):
"""Test getting cleaning brush cleaned count."""
assert self.device.consumable_status().cleaning_brush_cleaned_count == 44

def test_mop_dryer_model_check(self):
Copy link
Owner

Choose a reason for hiding this comment

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

What do you think about simplifying this using pytest.mark.parametrize?
Something like this (untested):

@pytest.mark.parametrize("method_name,params", [("mop_dryer_settings", None), ("set_mop_drier_enabled", {"enabled": True}), ...])
def test_mop_drier_raises_unsupported(method_name, params):
    with pytest.raises(UnsupportedFeatureException):
        method = getattr(self.device, method_name)
        if params is not None:
            method(**params)
         else:
            method()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK @pytest.mark.parametrize is not usable when using TestCase because the reference to self is missing.

Copy link
Owner

Choose a reason for hiding this comment

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

Ohh, okay. I think this is fine and can be reconsidered when the tests get cleaned up to avoid inheriting from TestCase, thanks for the PR and for giving it a go against a real device! 👍

miio/integrations/vacuum/roborock/tests/test_vacuum.py Outdated Show resolved Hide resolved
miio/integrations/vacuum/roborock/vacuum.py Show resolved Hide resolved
miio/integrations/vacuum/roborock/vacuumcontainers.py Outdated Show resolved Hide resolved
@jpbede
Copy link
Contributor Author

jpbede commented Dec 28, 2022

I've tested it now with a real device. Unfortunately the add-on is not reporting mop_dryer_remaining_seconds and is_mop_drying, so I need to remove the check in _verify_mop_dryer_supported.

Starting, stopping and setting the dry time is working.

@jpbede jpbede force-pushed the feat/s7maxv-mop-dryer branch from 1ee2ebc to 32f820d Compare December 28, 2022 15:09
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 for the PR @jpbede!

This does not currently modify the status() logic to allow exposing them directly to homeassistant, but that requires some internal changes on how the embedding is done so I think this is ready to be merged now.

@@ -443,10 +444,63 @@ def test_cleaning_brush_cleaned_count(self):
"""Test getting cleaning brush cleaned count."""
assert self.device.consumable_status().cleaning_brush_cleaned_count == 44

def test_mop_dryer_model_check(self):
Copy link
Owner

Choose a reason for hiding this comment

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

Ohh, okay. I think this is fine and can be reconsidered when the tests get cleaned up to avoid inheriting from TestCase, thanks for the PR and for giving it a go against a real device! 👍

@rytilahti rytilahti merged commit 6e5e0a5 into rytilahti:master Jan 1, 2023
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.

3 participants