-
-
Notifications
You must be signed in to change notification settings - Fork 589
Add the source field to MiotProperty #2037
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 the source field to MiotProperty #2037
Conversation
…c miot. Also defaults the value to the first value when unable to find the choice
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2037 +/- ##
=======================================
Coverage 82.26% 82.26%
=======================================
Files 197 197
Lines 19144 19145 +1
Branches 1052 1052
=======================================
+ Hits 15749 15750 +1
Misses 3218 3218
Partials 177 177 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this 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! It would be nice to have a test for the source, and this behaviour. The logic might need to be verified, see my comment inline.
miio/miot_models.py
Outdated
selected = next(c.description for c in self.choices if c.value == value) | ||
selected = next( | ||
(c.description for c in self.choices if c.value == value), | ||
self.choices[0], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be that the reported 1
is for the index in the available choices?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be. But the app didn't parse that 1 index as a value. So unsure what happened.
The miot protocol works in mysterious ways
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, if you want to test it with different values on your device to confirm the behavior, it could be interesting. Otherwise, I would avoid changing the behavior here now, as it feels like a firmware bug on your device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, if you want to test it with different values on your device to confirm the behavior, it could be interesting. Otherwise, I would avoid changing the behavior here now, as it feels like a firmware bug on your device.
Yeah it does sound like a firmware bug. Would it be better if it returned None instead?
I'm unsure what the source field actually does. That's why I marked it as optional. Do you have any idea what it's purpose is? |
No idea, but it's fine to have it as optional. I'm more worried about changing the behavior of choices, so if you exclude that change from this PR we can merge this immediately. |
The xiaomi.fan.p76 works over generic miot.
This also fixes a weird bug where the device does not respond with a value that is in the "choices list".
For me. Vertical swing returned 1 when the options are 30, 60, 90 and 100.
Sounds like a bug in miot in general.
Unsure what the behavior should be.
The mihome app showed the value as "-" but it was still doing vertical swing
Hopefully this should resolve the issue #2032
Closes #2032