Skip to content

Conversation

@peleccom
Copy link
Contributor

@peleccom peleccom commented Jan 10, 2022

@peleccom peleccom changed the title Feature/dreamef9 support Dreame F9 Vacuum (dreame.vacuum.p2008) support Jan 10, 2022
@rytilahti
Copy link
Owner

Hi and thanks for the PR! Have you tried to check how much does this differ from the current dreame implementation? If it's just about different mappings (and some other extra features), it would be better to extend that instead of having a separate implementation for this.

@peleccom
Copy link
Contributor Author

@rytilahti Hi. It has different mapping and some different statuses. Do you mean we should create subclass for Dreame F9 in that case?

@rytilahti
Copy link
Owner

@peleccom hi! Okay, so it would be better to modify the existing implementation to have a dictionary of mappings (e.g., https://github.com/rytilahti/python-miio/blob/master/miio/fan_miot.py#L17) and using the same keys wherever possible.

I see that the current dreame implementation is not using call_action for the actions, likely because the wasn't available when the integration was created.. So, the way I see this should be done is to adjust the existing mc1808 mapping to include the necessary actions (as yours does) and consolidate the code to use call_action for everything.

If there are some functions that are available only on p2008, you can check for the model (self.model) and raise an exception to inform that the functionality is not available. The plan is to have some common interfaces to make it easier to add homeassistant integration notwithstanding that devices are produced by different vendors.

Now for the status container, we can extend that to cover new features of p2008, and it's fine to return None if the data is not available due to a different mapping.
About the status codes themselves, the best approach depends on whether we can re-use the existing mapping at all or not. Are the common codes (or statuses) being shared by both models, or do the differ so much that this would require a separate status code class?

@peleccom
Copy link
Contributor Author

Ok. Now it share the same implementation as Dreame 1C.

#1254 as a rewrite of Dreame 1C implementation looks good and simplify code much. But it is very different from other implementation. Also current state doesn't allow to use commands from cli tool as I can see.

Also I want to ask if it is some progress with universal interface for all vacuum implementations? Can I help with something?

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.

A brief review round:

  • Please avoid separate container & implementation classes, as this makes it just harder to prepare to move using common interfaces.
  • DreameVacuumMiot would be the main class, which supports both models, you can adjust the behavior inside the class by leveraging the model information (self.model).

@rytilahti
Copy link
Owner

Ok. Now it share the same implementation as Dreame 1C.

Looks already much better! 👍

#1254 as a rewrite of Dreame 1C implementation looks good and simplify code much. But it is very different from other implementation. Also current state doesn't allow to use commands from cli tool as I can see.

Ah, you are right. It's better to get your PR done first, and then consider adding support for other dreame devices to the integration.

Also I want to ask if it is some progress with universal interface for all vacuum implementations? Can I help with something?

Alas, there hasn't been much going on on that front, I simply don't have time to work on it currently. The first step to make that happen would likely involve creating those interfaces (for Vacuum and VacuumStatus) and start porting all integrations over, starting from small with only very basic functionality and extending that over time.

@rytilahti
Copy link
Owner

Hi @peleccom, I realized that there is no facilities to handle mappings dictionaries without fuss.. I created #1302 to improve the situation, that should make it simpler to implement the required changes, please let me know if that helps :-)

@peleccom
Copy link
Contributor Author

@rytilahti Please check

@codecov-commenter
Copy link

codecov-commenter commented Jan 31, 2022

Codecov Report

Merging #1290 (7ddd689) into master (2af2d67) will decrease coverage by 0.09%.
The diff coverage is 76.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1290      +/-   ##
==========================================
- Coverage   82.43%   82.34%   -0.10%     
==========================================
  Files         109      109              
  Lines       11614    11831     +217     
  Branches     1376     1397      +21     
==========================================
+ Hits         9574     9742     +168     
- Misses       1838     1880      +42     
- Partials      202      209       +7     
Impacted Files Coverage Δ
miio/discovery.py 49.33% <ø> (ø)
miio/miot_device.py 81.53% <0.00%> (-1.28%) ⬇️
...io/integrations/vacuum/dreame/dreamevacuum_miot.py 75.08% <65.07%> (-8.57%) ⬇️
miio/__init__.py 100.00% <100.00%> (ø)
...ions/vacuum/dreame/tests/test_dreamevacuum_miot.py 100.00% <100.00%> (ø)
miio/integrations/vacuum/roborock/vacuum.py 63.38% <0.00%> (+0.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2af2d67...7ddd689. Read the comment docs.

@peleccom peleccom force-pushed the feature/dreamef9_support branch from 4134434 to 4ab8c5e Compare January 31, 2022 21:43
@rytilahti
Copy link
Owner

Btw, looks like another model (dreame.vacuum.p2028) is also using the same command set: #1217 (comment)

@peleccom
Copy link
Contributor Author

peleccom commented Feb 4, 2022

@rytilahti Fine. I've added Dreame Z10 Pro (dreame.vacuum.p2028) to the list of supported models.
Can we merge PR now?

@rytilahti
Copy link
Owner

@peleccom this is just pending on the status container docstring as I mentioned earlier: #1290 (comment) - would you please handle that? :-)

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.

Looks great, thanks for the PR @peleccom! Please rebase on top of the current master and I'll merge it.

@rytilahti rytilahti merged commit ea51f5f into rytilahti:master Feb 7, 2022
rytilahti added a commit that referenced this pull request Feb 17, 2022
This release adds support for several new devices (see details below, thanks to @PRO-2684, @peleccom, @ymj0424, and @supar), and contains improvements to Roborock S7, yeelight and gateway integrations (thanks to @starkillerOG, @Kirmas,>

Python 3.6 is no longer supported, and Fan{V2,SA1,ZA1,ZA3,ZA4} utility classes are now removed in favor of using Fan class.

[Full Changelog](0.5.9.2...0.5.10)

**Breaking changes:**

- Split fan.py to vendor-specific fan integrations [\#1304](#1304) (@rytilahti)
- Drop python 3.6 support [\#1263](#1263) (@rytilahti)

**Implemented enhancements:**

- Improve miotdevice mappings handling [\#1302](#1302) (@rytilahti)
- airpurifier\_miot: force aqi update prior fetching data [\#1282](#1282) (@rytilahti)
- improve gateway error messages [\#1261](#1261) (@starkillerOG)
- yeelight: use and expose the color temp range from specs [\#1247](#1247) (@Kirmas)
- Add Roborock S7 mop scrub intensity [\#1236](#1236) (@shred86)

**New devices:**

- Add support for zhimi.heater.za2 [\#1301](#1301) (@PRO-2684)
- Dreame F9 Vacuum \(dreame.vacuum.p2008\) support [\#1290](#1290) (@peleccom)
- Add support for Air Purifier 4 Pro \(zhimi.airp.va2\) [\#1287](#1287) (@ymj0424)
- Add support for deerma.humidifier.jsq{s,5} [\#1193](#1193) (@supar)

**Merged pull requests:**

- Add roborock.vacuum.a23 to supported models [\#1314](#1314) (@rytilahti)
- Move philips light implementations to integrations/light/philips [\#1306](#1306) (@rytilahti)
- Move leshow fan implementation to integrations/fan/leshow/ [\#1305](#1305) (@rytilahti)
- Split fan\_miot.py to vendor-specific fan integrations [\#1303](#1303) (@rytilahti)
- Add chuangmi.remote.v2 to chuangmiir [\#1299](#1299) (@rytilahti)
- Perform pypi release on github release [\#1298](#1298) (@rytilahti)
- Print debug recv contents prior accessing its contents [\#1293](#1293) (@rytilahti)
- Add more supported models [\#1292](#1292) (@rytilahti)
- Add more supported models [\#1275](#1275) (@rytilahti)
- Update installation instructions to use poetry [\#1259](#1259) (@rytilahti)
- Add more supported models based on discovery.py's mdns records [\#1258](#1258) (@rytilahti)
@rytilahti rytilahti mentioned this pull request Feb 17, 2022
rytilahti added a commit that referenced this pull request Feb 17, 2022
This release adds support for several new devices (see details below, thanks to @PRO-2684, @peleccom, @ymj0424, and @supar), and contains improvements to Roborock S7, yeelight and gateway integrations (thanks to @starkillerOG, @Kirmas, and @shred86).
Thanks also to everyone who has reported their working model information, we can use this information to provide better discovery in the future and this release silences the warning for known working models.

Python 3.6 is no longer supported, and Fan{V2,SA1,ZA1,ZA3,ZA4} utility classes are now removed in favor of using Fan class.

[Full Changelog](0.5.9.2...0.5.10)

**Breaking changes:**

- Split fan.py to vendor-specific fan integrations [\#1304](#1304) (@rytilahti)
- Drop python 3.6 support [\#1263](#1263) (@rytilahti)

**Implemented enhancements:**

- Improve miotdevice mappings handling [\#1302](#1302) (@rytilahti)
- airpurifier\_miot: force aqi update prior fetching data [\#1282](#1282) (@rytilahti)
- improve gateway error messages [\#1261](#1261) (@starkillerOG)
- yeelight: use and expose the color temp range from specs [\#1247](#1247) (@Kirmas)
- Add Roborock S7 mop scrub intensity [\#1236](#1236) (@shred86)

**New devices:**

- Add support for zhimi.heater.za2 [\#1301](#1301) (@PRO-2684)
- Dreame F9 Vacuum \(dreame.vacuum.p2008\) support [\#1290](#1290) (@peleccom)
- Add support for Air Purifier 4 Pro \(zhimi.airp.va2\) [\#1287](#1287) (@ymj0424)
- Add support for deerma.humidifier.jsq{s,5} [\#1193](#1193) (@supar)

**Merged pull requests:**

- Add roborock.vacuum.a23 to supported models [\#1314](#1314) (@rytilahti)
- Move philips light implementations to integrations/light/philips [\#1306](#1306) (@rytilahti)
- Move leshow fan implementation to integrations/fan/leshow/ [\#1305](#1305) (@rytilahti)
- Split fan\_miot.py to vendor-specific fan integrations [\#1303](#1303) (@rytilahti)
- Add chuangmi.remote.v2 to chuangmiir [\#1299](#1299) (@rytilahti)
- Perform pypi release on github release [\#1298](#1298) (@rytilahti)
- Print debug recv contents prior accessing its contents [\#1293](#1293) (@rytilahti)
- Add more supported models [\#1292](#1292) (@rytilahti)
- Add more supported models [\#1275](#1275) (@rytilahti)
- Update installation instructions to use poetry [\#1259](#1259) (@rytilahti)
- Add more supported models based on discovery.py's mdns records [\#1258](#1258) (@rytilahti)
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