Support for Zero Fog DWZF(G)-4500Z (shuii.humidifier.jsq002)humidifier (#1171)

[iromeo]

Hi, this is PR add support for Zero Fog DWZF(G)-4500Z (shuii.humidifier.jsq002)humidifier and fixes Issue #1171
New device support was tested by user on his device.

[codecov-commenter]

Codecov Report

Merging #1318 (c2b1574) into master (a55149c) will increase coverage by 0.01%.
The diff coverage is 86.23%.

@@            Coverage Diff             @@
##           master    #1318      +/-   ##
==========================================
+ Coverage   82.43%   82.45%   +0.01%     
==========================================
  Files         109      109              
  Lines       11615    11727     +112     
  Branches     1376     1395      +19     
==========================================
+ Hits         9575     9669      +94     
- Misses       1838     1848      +10     
- Partials      202      210       +8     

Impacted Files	Coverage Δ
miio/airhumidifier_jsq.py	89.40% <86.02%> (-5.89%)	⬇️
miio/__init__.py	100.00% <100.00%> (ø)
miio/discovery.py	50.00% <100.00%> (+0.66%)	⬆️

---

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 a55149c...c2b1574. Read the comment docs.

[rytilahti]

Thanks for the PR @iromeo!

Please extend the existing classes to support this model instead of creating new ones: 1) it will be easier for downstreams to use it when there is a common interface and 2) later on, it will be easier to implement a common interface among different humidifer implementations.

[rytilahti]

Let's re-use the existing enum for this, you can check for the model & raise an exception if an unsupported mode is tried to be set. We want to have a common interface in the future among different airhumidifers, so not having two different ones in this class will make it simpler.

[iromeo]

Hi, having some good API will be great.

At the moment such enums are actually used as string <-> response_value mappings that are used for two purposes. First as response value (string or int) to presentable name mapping, the second as a list of available device modes names for UI. E.g. to generate names of available options in:

• python-miio CLI interface
• in projects like https://github.com/syssi/xiaomi_airpurifier

If I reuse the existing enum, we will have to hardcode device mode and brightness levels names in all projects based on python-miio. At the moment python-miio is a single place that captures all info about devices, e.g. their protocols and all options. So it isn't a good idea to force all dependent projects to figure out from the scratch what are valid names for different options for each device supported in the library. Better to do this work once in 'python-miio'. Moreover, even python-miio CLI will return confusing error message if a user passes the wrong mode argument via the command line:

I mean, that code like:

    @command(
        click.argument("mode", type=EnumType(OperationModeJsq002)),
        default_output=format_output("Setting mode to '{mode.value}'"),
    )
    def set_mode(self, mode: OperationModeJsq002):
        ...

results in message generated by click library (from argument type error), like ~ incorrect value 'foo', expected one of: 'level1', 'level2', 'level3'. So it doesn't call set_mode. If using OperationModeJsq error will be included mode names that are N/A for jsq002 and that is confusing for user.

So how do you suggest fixing it correctly? As far as I understand better is do not use enums at all and hardcode the same info into a dictionary. It will be useful for API because we could use same method return types everywhere.

E.g.:

1. Humidifier device could have a mode presentable name: str <-> response value: object mapping. And a method that returns list of all supported modes names. Then dependent projects could get list of supported option names to show them in UI. And same for brightness modes names. So information about the mapping have to be hardcoded somewhere, the question is in what form.

class AirHumidifierStatusJsq002:
    @property
    def mode(self) -> str:
      # return presentable mode name

or

class AirHumidifierStatusJsq002:
    @property
    def mode(self) -> obj:
      # return raw value from device

and :

class AirHumidifierJsq002(..):
   def __init__(..):
      self._mode_values_2_name = OrderDict([(1, "Level1"), .., (3, "Level3")])

   def mode_names() -> Tuple[str]:
      return tuple( self. _mode_values_2_name.keys())

2. As for :

 @command(
        click.argument("mode", type=EnumType(OperationMode)),
        default_output=format_output("Setting mode to '{mode.value}'"),
    )
    def set_mode(self, mode: OperationMode):
        ...

seems is better to use again 'string/int values/object' e.g.:

Using presentable names:

 @command(
        click.argument("mode", type=str)
        default_output=format_output("Setting mode to '{mode.value}'"),
    )
    def set_mode(self, mode: str):
       # if `mode` not in `self. mode_names()` than report an error
        ...

or use raw values reported by device as mode

 @command(
        click.argument("mode", type=obj)
        default_output=format_output("Setting mode to '{mode.value}'"),
    )
    def set_mode(self, mode: obj):
       # if `mode` not in `supported device response values` than report an error
        ...

---

So what do you think, and especially:

• Should I replace enums with str values?
• Is it better to use for Status mode device raw value (int or str) or better report some string presentable names? Maybe for dependent projects string names are better. Because users will use them in their automation, etc. code when comparing device state with some desired state.
• If do not use enums, should I extend AirHumidifierStatus and override signatures of def mode(self) -> OperationMode: and other properties? Or better to have an independent status class?
• I could modify *jsq001 API also (I was a contributor). This will break compatibility with https://github.com/syssi/xiaomi_airpurifier project, but there I'm working on PR for *jsq002 support, so I could update integrations with both devices. If we use presentable names for status mode values, user automations shouldn't be broken. * jsq001 isn't supported by HA, so not changes in HA required. Maybe some other home automation solutions uses current *jsq001 API, I'm not aware of this.

[iromeo]

@rytilahti Hi, any suggestion about my above thoughts?

[rytilahti]

Hey @iromeo, sorry, I haven't had time to formulate a proposal how I think this should be approached, but here a couple of thoughts:

• IMO, the same model/device family (like shuii.humidifier.jsq* in this case) should be implemented using a simple class that adapts to the model-specific differences. This means sharing the same data classes (for status) and raising exceptions when unavailable methods are called.
• Different modes: this library really needs a common interface for the features (or settings) that apply to different devices. A first step could be something like adding supported_modes (or whatever naming fits the best) that returns a dict[str, int] and set_mode_by_name(str). This would be similar to what roborock.Vacuum has with fanspeed_presets to inform homeassistant for the available fan speeds.
• For generic functionality, enums (or booleans where applicable) should be preferred, as those can be type-checked statically. Maybe set_led(bool) as a generic interface suffices until we find a proper solution to provide different "settings" in a common way? The implementations can keep having their separate, internal implementations, but it'd be great to have a stable interface to avoid imports like this https://github.com/syssi/xiaomi_airpurifier/blob/develop/custom_components/xiaomi_miio_airpurifier/fan.py#L31-L74 .

I'll see if I can find some time to implement a common interface for vacuums based using typing.Protocol that could be used as an inspiration for similar interface for humidifers, but I cannot promise currently anything.

tldr; my concern is that as there are currently no strict API contracts, it is awfully complicated to use this lib, e.g., in homeassistant as it is necessary to adapt parts of the code depending on the device and the implementation. Where possible, consider sharing the interfaces (at least) on the vendor level (shuii.humidifer.*) in this case – this will make it far more easier to use the code, as well as to refactor it if there'll be a common "humidifier" interface that covers also other implementations for easier downstream use.

To briefly answer your explicit questions:

Should I replace enums with str values?

Enums are better (as they can be consolidated far easier in the future) than strings.

Is it better to use for Status mode device raw value (int or str) or better report some string presentable names? Maybe for dependent projects string names are better. Because users will use them in their automation, etc. code when comparing device state with some desired state.

Raw, device-reported values should not be used as-is as due to the reasons above.

If do not use enums, should I extend AirHumidifierStatus and override signatures of def mode(self) -> OperationMode: and other properties? Or better to have an independent status class?

As long as the class is exchangeable with the existing one, it's fine. I would rather see the current class extended to cover new devices.

I could modify *jsq001 API also (I was a contributor). This will break compatibility with https://github.com/syssi/xiaomi_airpurifier project, but there I'm working on PR for *jsq002 support, so I could update integrations with both devices. If we use presentable names for status mode values, user automations shouldn't be broken. * jsq001 isn't supported by HA, so not changes in HA required. Maybe some other home automation solutions uses current *jsq001 API, I'm not aware of this.

I think it's fine to break the compatibility here, having a common API will also make the implementation of xiaomi_airpurifier simpler :-)

[iromeo]

For generic functionality, enums (or booleans where applicable) should be preferred, as those can be type-checked statically.

I agree with bools

As for enums:

• Using single technical enum for a series/all devices, where half of the values are N/A in the device A and other half ar N/A in the device B (and results in runtime exception) doesn't bring much profit in terms of type checking. No profit because correct types will result in run time error when using the unsupported value from enums. So just a set of supported string names (that could be requested in API for each device) doesn't differ form enum in such case.

• Maybe the main profit of enum is for internationalization, e.g. we could consider aggregated enum for all states of all airpurifier devices just as a list of all known states names, some of them are unique, some of them a reused in multiple devices. For new device C a contributor could reuse existing states and add new states if required. So we could implement translation for these states instead of doing it for each device personally, e.g. to avoid cases when A uses "level #1" and B uses "Level1" default names instead of AllStates.LEVEL_1. So each device should provide list of supported states from the enum and some mapping used for device response parsing from incoming object to one of supported enum states.

• Also for library uses maybe one large enum for all devices states could be beneficial, to be sure that there is one constant for States.Off state, instead of a range of similar strings provided by devices, e.g Off, Turned Off, Powered Off

shuii.humidifier.jsq* should be implemented using a simple class that adapts to the model-specific differences. This means sharing the same data classes (for status) and raising exceptions when unavailable methods are called.

To my mind shuii.humidifier.jsq001 and shuii.humidifier.jsq002 are two very different devices and the most similar in them is just their device names. Their feature sets somewhat overlaps, but there is a difference on both sides, also there is a difference in communication protocols, command names, response format, state names, state ranges and their ids. Seems we need some generic humidifier/purifier API that could cover a wide set of features, and each device could report what features are supported from the all known features set.

fine to break the compatibility here,
Ok, I'll refactor this some way so we could discuss the result )

[rytilahti]

Reuse the existing & do the conversion when needed. You can create a separate dict mapping to map between the enum and the integer value to perform conversions based on self.model.

[rytilahti]

Please extend the existing status class, the values that are not available for a model should be returned as None.

[vkarpunin]

Hi! Could I help somehow to move this PR forward?