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 11 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 82.30%. Comparing base (edb06c5) to head (469249e).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1940      +/-   ##
==========================================
+ Coverage   82.26%   82.30%   +0.04%     
==========================================
  Files         197      197              
  Lines       19139    19188      +49     
  Branches     1050     1050              
==========================================
+ Hits        15744    15793      +49     
  Misses       3218     3218              
  Partials      177      177              

☔ 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

@Nihisil
Copy link

Nihisil commented Jan 25, 2025

@rytilahti Hello, is there anything that needs to be done for this pull request to be merged? I have this device, and I am interested in having support for it in the library

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