chore: Add some async improvements #229
Conversation
|
I think it would be best to just keep it simple and create a counter to be used as the request_id (self.counter % 32767). What you think? |
|
Yeah that probably makes sense. I worried about local messages conflicting though if we went in a sequential order, but it probably does make more sense |
|
I don't know if it can conflict between cloud and local. But we can do something like top to bottom on cloud and bottom to top on local |
Sorry, I misspoke, I was specifically talking about commands run via the roborock app. i.e. I do something on my phone's app, it happens to be request id 34212 and that happens to be the next sequential id for HA. But it is probably rare enough that that edge case shouldn't even be really considered. |
|
Okay made the changes, what do you think about that? |
| f"Attempting to create a future with an existing request_id... New id is {new_id}. " | ||
| f"Code may not function properly." | ||
| ) | ||
| request_id = new_id |
There was a problem hiding this comment.
I think this ends up writing the future with a different key, but does not change the request id chosen by the caller. I think this means the response will still be left hanging and/or associated with the wrong request?
| @@ -155,9 +155,9 @@ class MessageRetry: | |||
| class RoborockMessage: | |||
| protocol: RoborockMessageProtocol | |||
| payload: bytes | None = None | |||
| seq: int = randint(100000, 999999) | |||
| seq: int = get_next_int(100000, 999999) | |||
There was a problem hiding this comment.
I believe these numbers are set once statically, and not once per message creation, which i think was the intent. I think this is the equivalent of setting a constant for all created messages.
There was a problem hiding this comment.
Looks like you may be correct? I modified what was existing prior without thinking about it much, I thought data classes worked a bit like init but obviously not. Good catch
Relates to #228