fix: threadsafe waiting queue#301
Conversation
| @@ -53,15 +54,19 @@ async def send_message(self, roborock_message: RoborockMessage): | |||
| response_protocol = request_id + 1 | |||
There was a problem hiding this comment.
Is this trying to set the response protocol to the next thing, e.g. HELLO_REQUEST to HELLO_RESPONSE? I think it ends up munging things up and i can't tell what this is trying to do.
There was a problem hiding this comment.
Yes
I believe that it might be a mapping for dps request and response. But since I didn't have that I noticed that the response protocol was always a number higher
There was a problem hiding this comment.
OK great, maybe i can figure out another way to use the new protocol types. I'll think about this more and maybe:
(1) Add some tests to exercise these cases explicitly
(2) address first as a separate change
(3) come back to this
| _LOGGER.debug("Putting request key %s in the queue", request_key) | ||
| with self._lock: | ||
| if request_key in self._queue: | ||
| raise ValueError(f"Request key {request_key} already exists in the queue") |
There was a problem hiding this comment.
Maybe this should be a subclass of RoborockException for easier error handling?
There was a problem hiding this comment.
My thinking is that if this happens its a bug/shouldn't happen case and so it shouldn't be treated like a server side error. My thinking is: what do we expect a caller to do to handle this?
|
@allenporter - do you want me to do a full review on this? Or is it still a WIP? |
Still WIP -- I am worried the +1 logic commented above is not quite right and may raise exceptions. |
allenporter
force-pushed
the
wait-queue-lock
branch
from
fe55809 to
6fedba3
Compare
|
Will revisit this with new aiomqtt based client. |
3 participants
Make the waiting queue a threadsafe queue.
This attempts to push the protocol to be a request key, but it seems to have odd interactions with local clients that munge the protocol. Will do some testing on a live system before marking ready for review. Parts of this may end up getting reverted.
Issue #228