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 xiaomi.airp.va2b support #1940

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

dsh0416
Copy link
Contributor

@dsh0416 dsh0416 commented Jun 16, 2024

Meanwhile, for xiaomi.airp.va2b, it would cause {'code': -9999, 'message': 'user ack timeout'} when querying status, and I am still figuring why.

Copy link

codecov bot commented Jun 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.67%. Comparing base (8643a57) to head (6cf9755).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1940      +/-   ##
==========================================
+ Coverage   81.41%   81.67%   +0.25%     
==========================================
  Files         193      197       +4     
  Lines       18636    19176     +540     
  Branches     4045     4208     +163     
==========================================
+ Hits        15173    15662     +489     
- Misses       3180     3220      +40     
- Partials      283      294      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dsh0416 dsh0416 changed the title add xiaomi.airp.va2b support Draft: add xiaomi.airp.va2b support Jun 16, 2024
@dsh0416 dsh0416 marked this pull request as draft June 16, 2024 16:43
@dsh0416 dsh0416 changed the title Draft: add xiaomi.airp.va2b support add xiaomi.airp.va2b support Jun 16, 2024
@dsh0416
Copy link
Contributor Author

dsh0416 commented Jun 16, 2024

By randomly comment out some of the properties make the fetching process works. It might be caused by an incorrect Message.parse process in miioprotocol.py.

@dsh0416
Copy link
Contributor Author

dsh0416 commented Jun 16, 2024

By randomly comment out some of the properties make the fetching process works. It might be caused by an incorrect Message.parse process in miioprotocol.py.

Problem confirmed. It looks like the default max_properties is too large for xiaomi.airp.va2b.

@@ -558,7 +558,6 @@ def status(self) -> RoidmiVacuumStatus:
return RoidmiVacuumStatus(
{
prop["did"]: prop["value"] if prop["code"] == 0 else None
# max_properties limmit to 10 to avoid "Checksum error" messages from the device.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, this comment should be removed, since the max_properties is not properly limited at all, and there is also a typo in this comment. Maybe it is copy-pasted from somewhere else.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, that's probably just due to some copy&pasting. Please remove changes to roidmivacuum_miot from this PR (as they are unrelated, but feel free to create a separate PR to remove those if you wish!) and then this is good to go 👍

@@ -569,7 +568,6 @@ def consumable_status(self) -> RoidmiConsumableStatus:
return RoidmiConsumableStatus(
{
prop["did"]: prop["value"] if prop["code"] == 0 else None
# max_properties limmit to 10 to avoid "Checksum error" messages from the device.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same here.

@@ -580,7 +578,6 @@ def cleaning_summary(self) -> RoidmiCleaningSummary:
return RoidmiCleaningSummary(
{
prop["did"]: prop["value"] if prop["code"] == 0 else None
# max_properties limmit to 10 to avoid "Checksum error" messages from the device.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

and same here.

@dsh0416 dsh0416 marked this pull request as ready for review June 16, 2024 17:57
@dsh0416
Copy link
Contributor Author

dsh0416 commented Jun 16, 2024

I don't think I would be able to fix the readthedocs CI properly, so that this commit is now ready for review.

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 @dsh0416! 👍

Please remove the unrelated roidmivacuum changes from this PR and this is ready to merged.

@@ -558,7 +558,6 @@ def status(self) -> RoidmiVacuumStatus:
return RoidmiVacuumStatus(
{
prop["did"]: prop["value"] if prop["code"] == 0 else None
# max_properties limmit to 10 to avoid "Checksum error" messages from the device.
Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, that's probably just due to some copy&pasting. Please remove changes to roidmivacuum_miot from this PR (as they are unrelated, but feel free to create a separate PR to remove those if you wish!) and then this is good to go 👍

Copy link
Contributor Author

@dsh0416 dsh0416 left a comment

Choose a reason for hiding this comment

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

suggest for revert

miio/integrations/roidmi/vacuum/roidmivacuum_miot.py Outdated Show resolved Hide resolved
miio/integrations/roidmi/vacuum/roidmivacuum_miot.py Outdated Show resolved Hide resolved
miio/integrations/roidmi/vacuum/roidmivacuum_miot.py Outdated Show resolved Hide resolved
@dsh0416
Copy link
Contributor Author

dsh0416 commented Jun 26, 2024

reverted. Thank you for reviewing, and I may move the comments changes to a further PR. @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.

2 participants