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 basic dmaker.fan.p8 support and streamline miOT fan implementations #1029

Closed
wants to merge 2 commits into from

Conversation

syssi
Copy link
Collaborator

@syssi syssi commented May 3, 2021

Closes: #882

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, but I'm wondering if we should rethink how we should handle model handling. From the user's viewpoint it doesn't make sense to have separate classes for similar devices, where avoidable.

One potential way to achieve this would be to 1) allow explicit definition for models (as it is currently for the main impl class) and 2) if none is given, resort to calling info() on the first I/O call to find out the model. What do you think?

@syssi
Copy link
Collaborator Author

syssi commented May 5, 2021

I've introduced a class per device to have improved CLI support. I'm unsure it's a good decision or not.

@rytilahti
Copy link
Owner

I think we should not do that as it has more potential for causing confusion in the long run, so here is my proposal for the CLI use:

  1. We add model keyword argument to Device class constructor (much akin to what already exists on some sub-classes). This shall default to None / auto-detect and saved in self._model.
  2. We add --model option alongside the --ip and --token, to allow forcing specific model.
  3. If the internal self._model is None, any call to @command decorated function causes info() call to find the model.
  4. (Optionally,) if the class only supports a single model, it can initialize self._model in its constructor to skip the auto-detection.

Would that work for you? Would it be okay for you to rely on manual model definition for the next release (which I wish to get one out ASAP), and then adding this model feature for the next one after that?

@syssi
Copy link
Collaborator Author

syssi commented May 5, 2021

Fine for me! This PR shouldn't block the release.

@rytilahti
Copy link
Owner

Okay, great! I'll try to prepare a new release later tonight then :-)

@rytilahti
Copy link
Owner

I created #1038 to explore the idea I described above. It is rather hacky at this point and will need some more work, but I think something like that would be good to have in the longer term.

@ha0y
Copy link
Contributor

ha0y commented May 7, 2021

LGTM, but I'm wondering if we should rethink how we should handle model handling. From the user's viewpoint it doesn't make sense to have separate classes for similar devices, where avoidable.

One potential way to achieve this would be to 1) allow explicit definition for models (as it is currently for the main impl class) and 2) if none is given, resort to calling info() on the first I/O call to find out the model. What do you think?

Agreed. And another thing we should rethink is that how we handle new devices. Now we have a long list of miot-enabled devices(#627). And for each new device, we write many similar codes.

MIOT_MAPPING = {
MODEL_FAN_1C: {
# https://miot-spec.org/miot-spec-v2/instance?type=urn:miot-spec-v2:device:fan:0000A005:dmaker-1c:1
"power": {"siid": 2, "piid": 1},
"fan_level": {"siid": 2, "piid": 2},
"child_lock": {"siid": 3, "piid": 1},
"swing_mode": {"siid": 2, "piid": 3},
"power_off_time": {"siid": 2, "piid": 10},
"buzzer": {"siid": 2, "piid": 11},
"light": {"siid": 2, "piid": 12},
"mode": {"siid": 2, "piid": 7},
},
MODEL_FAN_P8: {
# Source https://miot-spec.org/miot-spec-v2/instance?type=urn:miot-spec-v2:device:fan:0000A005:dmaker-p8:1
"power": {"siid": 2, "piid": 1},
"fan_level": {"siid": 2, "piid": 2},
"child_lock": {"siid": 3, "piid": 1},
"swing_mode": {"siid": 2, "piid": 3},
"power_off_time": {"siid": 2, "piid": 10},
"buzzer": {"siid": 2, "piid": 11},
"light": {"siid": 2, "piid": 12},
"mode": {"siid": 2, "piid": 7},
},
MODEL_FAN_P9: {
# Source https://miot-spec.org/miot-spec-v2/instance?type=urn:miot-spec-v2:device:fan:0000A005:dmaker-p9:1
"power": {"siid": 2, "piid": 1},
"fan_level": {"siid": 2, "piid": 2},
"child_lock": {"siid": 3, "piid": 1},
"fan_speed": {"siid": 2, "piid": 11},
"swing_mode": {"siid": 2, "piid": 5},
"swing_mode_angle": {"siid": 2, "piid": 6},
"power_off_time": {"siid": 2, "piid": 8},
"buzzer": {"siid": 2, "piid": 7},
"light": {"siid": 2, "piid": 9},
"mode": {"siid": 2, "piid": 4},
"set_move": {"siid": 2, "piid": 10},
},
MODEL_FAN_P10: {
# Source https://miot-spec.org/miot-spec-v2/instance?type=urn:miot-spec-v2:device:fan:0000A005:dmaker-p10:1
"power": {"siid": 2, "piid": 1},
"fan_level": {"siid": 2, "piid": 2},
"child_lock": {"siid": 3, "piid": 1},
"fan_speed": {"siid": 2, "piid": 10},
"swing_mode": {"siid": 2, "piid": 4},
"swing_mode_angle": {"siid": 2, "piid": 5},
"power_off_time": {"siid": 2, "piid": 6},
"buzzer": {"siid": 2, "piid": 8},
"light": {"siid": 2, "piid": 7},
"mode": {"siid": 2, "piid": 3},
"set_move": {"siid": 2, "piid": 9},
},
MODEL_FAN_P11: {
# Source https://miot-spec.org/miot-spec-v2/instance?type=urn:miot-spec-v2:device:fan:0000A005:dmaker-p11:1
"power": {"siid": 2, "piid": 1},
"fan_level": {"siid": 2, "piid": 2},
"mode": {"siid": 2, "piid": 3},
"swing_mode": {"siid": 2, "piid": 4},
"swing_mode_angle": {"siid": 2, "piid": 5},
"fan_speed": {"siid": 2, "piid": 6},
"light": {"siid": 4, "piid": 1},
"buzzer": {"siid": 5, "piid": 1},
# "device_fault": {"siid": 6, "piid": 2},
"child_lock": {"siid": 7, "piid": 1},
"power_off_time": {"siid": 3, "piid": 1},
"set_move": {"siid": 6, "piid": 1},
},
}

So I think we can fetch its MIoT-Spec from the server, and parse it so that it is automatically adapted. That 's what I have done in https://github.com/ha0y/xiaomi_miot_raw . Thanks to you and @syssi because it would not have been possible without your great works. If you don't mind, please give it a try. And the adapting method is in https://github.com/ha0y/xiaomi_miot_raw/blob/master/custom_components/xiaomi_miot_raw/deps/miot_device_adapter.py , which simply tries to find out all supported services and properties.

@rytilahti
Copy link
Owner

rytilahti commented Jun 4, 2021

Hi @ha0y and sorry for the late response, I have been following your efforts to provide generic support for miot devices. And I completely agree, that the current way we are doing miot support in this library does not scale. However, I have two concerns here:

  1. I think this library should not depend on externally sourced resources (like the definition files hosted online)
  2. Vendoring those files to be a part of the library is also not great

I wrote some initial ideas on adding generic device types to this library here #1059 (comment) -- do you have an insight how easy it would to create rather generic, device-type specific interfaces for miot devices? In other words, do you know if, for example the vacuum devices, use same set of siid&piid values for a specific set of basic functionalities, or would that require device-specific mappings for each and every device?

Considering we have a common "BaseFan" interface, what would it take to have the devices from the category to have at least basic functionality in a single "MiotFan" implementation?

@marcelrv
Copy link

marcelrv commented Jun 5, 2021

@rytilahti as dat as I have seen.. the devices do not have same set of siid&piid.

There are some overlaps, but imho you can't assume same piid/siid point to the same sorry of property.
Inside brand e.g. viomi vacuums there is more overlap, but even then it may differ

@ha0y
Copy link
Contributor

ha0y commented Jun 8, 2021

I think this library should not depend on externally sourced resources (like the definition files hosted online)
Vendoring those files to be a part of the library is also not great

Yes, it is not possible to vendor these files in the library, because each device has their own definition files, and dozens of devices join Xiaomi IoT every week.

In other words, do you know if, for example the vacuum devices, use same set of siid&piid values for a specific set of basic functionalities, or would that require device-specific mappings for each and every device?

It is also not possible to use the same set of siid & piid because they are not fixed. Accroding to Xiaomi's document, the manufacturer can specify the service and property of their device, which will change the order of them. And the apps doesn't identify those services and properties by iid, but by URN, which (as far as I know) only exists in spec files.

When it comes to fan: many manufacturers create a private service to provide some additional functions, including 1%-100% speed. This is also a tough problem.

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

Successfully merging this pull request may close these issues.

Add support for Mi Smart Standing Fan 1C (dmaker.fan.p8)
4 participants