-
Notifications
You must be signed in to change notification settings - Fork 48
feat: fix a01 and b01 response handling in new api #453
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
Conversation
ea1587b
to
7a2b8e2
Compare
if not finished.is_set(): | ||
finished.set() | ||
|
||
unsub = await mqtt_channel.subscribe(find_response) |
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.
Okay maybe i'm just getting confused by the asyncness and maybe that is leading to my confusion with the returns above as well.
My first thought when I was reading this was that we are adding a subscription everytime that we send a message. Then when a message goes through, we are going through all of the subscriptions and if they aren't a match, we end up returning None on them.
However, looking at it, it seems that subscribe holds exactly one callback, so if I had three different send_commands I wanted to send at once - wouldn't that override the subscription callback for each one? Or does the lock within roborock_session prevent that - if that's the case, doesn't that dramatically increase the amount of time that I will be waiting to get a response?
Maybe it's okay, as if you are using the api correctly, all IDs that you are querying should be done with one function call - but there could be commands that we don't utilize in A01 that should have responses as well
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.
You're right, the code I was using for mqtt session changed a bunch of times, but i reverted it for this PR.
The intent was to use a version that allowed any number of callbacks. I think a version fo this was reverted so this no longer makes sense.
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.
Updated this to remove the limitation of a single subscribe call for the mqtt channel. This matches the behavior of local channel.
This builds on #452 by removing additional response decoding in
RoborockMessage
, separating out the different protocols better. This now has separate RPC logic for a01, b01, and v1. This removes thePendingRpc
abstraction that was just added since it is no longer necessary since we can handle all responses within a single function call using a subscribe callback.