peterhinch / micropython-mqtt

A 'resilient' asynchronous MQTT driver. Recovers from WiFi and broker outages.
MIT License
549 stars 116 forks source link

Added mqtt_as_queue - mqtt_as with queue #72

Closed aspro-at closed 2 years ago

aspro-at commented 2 years ago

In the documentation it is described that the message handler is blocking and that no messages can be sent while one is received/handled. Therefore it is up to the application to implement some queue mechanism. That's what the class mqtt_as_queue adds to mqtt_as at class level.

peterhinch commented 2 years ago

One option is to use a queue. Another is to launch a coroutine for each publication as described here. There are advantages and disadvantages to each approach. The current design intentionally leaves the choice open.

aspro-at commented 2 years ago

So it is the same situation as with timeouts for publications. This is exactly the reason mqtt_as_queue.py extends the class MQTTClient from mqtt_as.py just as it is with mqtt_as_timeout.py.

peterhinch commented 2 years ago

The reason we implemented mqtt_as_timeout was because Kevin Köck identified a subtle problem with the obvious solution (using await asyncio.wait_for). We felt that this should be explained in the docs and a solution offered. There is no comparable "gotcha" with a queue, which is easy to implement. Further there are a number of design decisions with a queue:

  1. Should it be fixed size or allowed to grow forever if connectivity fails?
  2. If fixed size should storage be pre-allocated?
  3. If fixed size, how should it behave if it fills?

There is also the question of the API. If I were designing it, I would replace the publish method in the subclass and have it automatically launch a coroutine when an item is placed on an empty queue. Queueing would then be transparent to the user. I would also use a fixed size, pre-allocated queue which launched an optional user-supplied callback if it became full.

I don't claim my design is better than yours - merely suggesting that if you gave the problem to ten coders you'd get ten different answers; answers which would undoubtedly vary with the application the individual had in mind.

These are the reasons we didn't implement a queue. I think it lives in the application and not in the library.

aspro-at commented 2 years ago

Thank you for your detailed explanation. You’re right the design of the queue must live in the application.

Having read your mail carefully I am going to change my design a little bit. A queue that might grow forever until the device „fails“ does not seem reliable.

Thanks,

Andreas Philipp

Von: Peter Hinch @.> Gesendet: Dienstag, 3. Mai 2022 10:59 An: peterhinch/micropython-mqtt @.> Cc: aspro-at @.>; Author @.> Betreff: Re: [peterhinch/micropython-mqtt] Added mqtt_as_queue - mqtt_as with queue (PR #72)

The reason we implemented mqtt_as_timeout was because Kevin Köck identified a subtle problem with the obvious solution (using await asyncio.wait_for). We felt that this should be explained in the docs and a solution offered. There is no comparable "gotcha" with a queue, which is easy to implement. Further there are a number of design decisions with a queue:

  1. Should it be fixed size or allowed to grow forever if connectivity fails?
  2. If fixed size should storage be pre-allocated?
  3. If fixed size, how should it behave if it fills?

There is also the question of the API. If I were designing it, I would replace the publish method in the subclass and have it automatically launch a coroutine when an item is placed on an empty queue. Queueing would then be transparent to the user. I would also use a fixed size, pre-allocated queue which launched an optional user-supplied callback if it became full.

I don't claim my design is better than yours - merely suggesting that if you gave the problem to ten coders you'd get ten different answers; answers which would undoubtedly vary with the application the individual had in mind.

These are the reasons we didn't implement a queue. I think it lives in the application and not in the library.

— Reply to this email directly, view it on GitHub https://github.com/peterhinch/micropython-mqtt/pull/72#issuecomment-1115876651 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AN6MRP5OBHHWOZIYLLFT5RDVIDTFHANCNFSM5UZOOCJA . You are receiving this because you authored the thread. https://github.com/notifications/beacon/AN6MRP2GR6F2BEXAVIQHV7TVIDTFHA5CNFSM5UZOOCJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOIKBO2KY.gif Message ID: @. @.> >