Asyncio improvements

[Lash-L]

Place to keep track of all of the ideas @allenporter brought up in #222

• making breaking changes to drop all the sync APIs and focus on simpler async APIs.
• reduce the class hierarhcy so there are not multiple base classes involved, and use composition rather than inheritance.
• adding logging for each outgoing request seq id and received resopnse seq id could help show the issue happening in more detail.
• add guards to not send a message with a request id that has already been allocated. One example of a potential bug is that the choosing of a request id can pick an id that already exists since its just picking random numbers.
• Add an explicit guard here when the future is already completed.
• Add a guard in _async_response so that it doesn't overwrite an existing future if the request id is already in use
• There are cases where a response is received and it checks against the queue and pulls out the message, but then if it doesn't already exist it continues anyway. Why would this ever be allowed to happen? seems like a bug or malformed response so it also needs logging rather than being ignored since it could hide concurrency bugs
• Remove the multiple levels of futures happening between asyncio.ensure_future then a sync method _async_response which calls an async method _wait_response which directly awaits on a future. I don't think all these levels of indirection are needed
• Don't pass around tuples of (response, exception). Just use the future built in exceptions

[allenporter]

I've made a few tiny changes of cleanup to get more familiar with the code base and understand it better. There are now three changes I would like to focus on. Let me know if you have any feedback before I get started.

Reduce the class hierarchy

I've also noticed there are a lot of duplicate initializations happening of local variables which is really confusing. For example RoborockClient initializes _waiting_queue but so does RoborockMqttClient, etc. Also the loggers are set inconsistently, sometimes in the base class and sometimes in the child class, so log messages are inconsistent and hard to track down to the source class of which API is being used. Another example is _send_msg_raw is duplicated between the v1 MQTT client and the base class with duplicate implementations.

There are a few phases:

(1) Drop unnecessary hierarchy:

One other example of the type of simple change we can make is we have this graph:

RoborockLocalClientV1 -> (RoborockLocalClient, RoborockClientV1)
RoborockLocalClient -> (RoborockClient, asyncio.Protocol)
RoborockClientV1 -> RoborockClient

And all the code used by RoborockLocalClient is only used by RoborockLocalClientV1 so we should drop it entirely.

(2) Stop multi-inheritence. In the example here for asyncio.Protocol we should extract out the transport specific behavior into a separate class that manages the details for the async io protocol stuff. The problem is that in any class you can't tell which methods are for the transport vs part of the internal class hierarchy. The MQTT client is much more complex than protocol.

(3) Reduce scope of public methods. However, we still need to facilitate testing.

Request Queue Overall

As a detailed example of reducing the class hierarchy, we should overall the request queue to add a simpler interface.

• We don't want the responsibility for managing concurrency correctly to be split across the class hierarchy.
• We should avoid using the request queue for connect and disconnect steps of MQTT since they don't need it. They already use a fixed request id, which adds confusion when trying to identify the duplicate request id issue. They are protected by a mutex and have straight forward dedicated callbacks where you don't need to know who sent the outgoing message and match it with the incoming message.

Testing

We can't do any of this without better testing, and my impression is that many of the key request flows are not unit tested at the moment. We can add test coverage for the primary request handling flows for each major client. This will give us more confidence when making changes that it's not going to break anything.

[Lash-L]

@allenporter Agree with everything

When I first made the change to add support for the A1 devices, I thought it made sense to do all this crazy inheritance so i wouldn't duplicate code. As time has gone on, I definitely see that was not the right approach.

I'll spend sometime starting to write some tests. But for now, i think everything you said is good to start