Separate network hardware interface from mqtt client #57
Conversation
as a base class for all hardware interface implementations.
to reduce code size on each platform and to make the modules easier to handle.
|
Anyone got any thoughts about the changes? |
There was a problem hiding this comment.
It's nice that you pulled the device specific implementations into separate files - that was the idea behind the change all along.
However - I still think you should implement two callbacks:
- one for connected
- one for disconnected
It's a tiny overhead in the base library but it makes things much more pleasant to work with, especially if you have multiple code parts that interact with the interface.
| # If a callback is passed, run it and return. | ||
| # If a coro is passed initiate it and return. | ||
| # coros are passed by name i.e. not using function call syntax. | ||
| def launch(func, tup_args): | ||
| res = func(*tup_args) | ||
| if isinstance(res, _type_coro): | ||
| res = asyncio.create_task(res) | ||
| return res |
There was a problem hiding this comment.
While I get while you would want functions I deliberately chose to allow only async functions.
If you have a longer task and and you launch it with create task and it's duration is e.g. 10 secs you have a state change the next callback will run in parallel to the already launched tasks and anything can happen.
If you only allow async functions then you can at least try to cancel them.
There was a problem hiding this comment.
Sorry but I think you somehow mixed callbacks and coroutines in this answer and now it doesn't really make much sense but I think I get your point.
Callbacks would be the better option for wifi state changes because they have to be executed quickly without any delays in it.
Coroutines can cause the problem of multiple coroutines running in parallel if the state changes quickly, which is quite possible. Cancelling those is not implemented in this simple interface and the user can't cancel them either because the tasks are not bound anywhere.
Personally I'd go with callbacks only and not allow coroutines but I have to think about backwards compatibility and the original mqtt_as has a wifi_coro, a coroutine. So I have to allow it in order to not break existing implementations.
We have the same problem with the connected_coro in mqtt_as, which isn't a callback either. The only reason neither coroutines cause trouble in mqtt_as is that there's a 5 seconds delay in the wlan connect to ensure wlan stability. Most coroutines should have finished after 5 seconds. But if we use a different interface or reduce the wlan reconnects, this will become a problem.
But again, backwards compatibility.. can't change that. Have to keep it that way. However, the wifi connected callback is now called directly after the connection is created and not after the broker was connected. This might result in more wifi callbacks as the wifi could get disconnected quickly if the connection is bad but I think it's an acceptable compromise and not many people will do much in the wifi connected callback because in mqtt_as this doesn't really mean anything because wifi will get disconnected again if the broker connection fails. (which is generally something I'd like to change in a more advanced interface because if your broker vanishes e.g. because you changed its ip adress or switched devices, your micropython device becomes inaccessible because you only have 5 seconds to establich a webrepl connection and interrupt the reconnect process..)
This is the reason why in my project, I subclassed the mqtt client and only have one connected coroutine that launches all my custom coroutines and also cancels all those tasks if the wifi gets disconnected again before it could finish (the coroutines just subscribe to the topics again, which would get paused if the client gets disconnected, so those would easily stack up).
There was a problem hiding this comment.
Sorry but I think you somehow mixed callbacks and coroutines in this answer and now it doesn't really make much sense but I think I get your point.
Is a coro that is called on connect not a callback? I am not sure about the correct terminology.
Personally I'd go with callbacks only and not allow coroutines but I have to think about backwards compatibility and the original mqtt_as has a wifi_coro, a coroutine. So I have to allow it in order to not break existing implementations.
If the has a long running sync callback things still can block, that's why I think a coro is better because it's immediately clear they have to be non blocking. And if it's all coros they have to be canceled by the base class.
There was a problem hiding this comment.
Callbacks in python (afaik) are always synchronous functions. Even the asyncio module in Python uses callbacks on certain Task events and those are all synchronous functions. Therefore my distinction between a callback=synchronous function and a coroutine=asynchronous function.
If the has a long running sync callback things still can block, that's why I think a coro is better because it's immediately clear they have to be non blocking. And if it's all coros they have to be canceled by the base class.
A long sync function will block no matter where it is implemented. If someone writes blocking code, he will do so no matter if the context is a callback or a coroutine.
If someone assumes he can write blocking code while using uasyncio, he doesn't undertand the environment he's programming in.
That said, callbacks are always supposed to be short whereas when using coroutines, people might write long code, which is not blocking but can take several seconds to finish.
Of course, not a problem if that task can be cancelled, but that would require the user to be aware of it and make their code handle cancellation correctly, which could be problematic for people new to python/asyncio and therefore it's best to stay away from that. Also it would make the base class more complex, which I'd like to avoid, not many will need that feature. I myself only log the connection state to the console in my project. A very short callback.
(As mentioned elsewhere my connected coro is a lot longer and more complex and actually cancels all tasks if the connection state changes. But that isn't part of the hardware interface but mqtt_as..)
| return await self._connect() | ||
| return False | ||
|
|
||
| def isconnected(self): |
There was a problem hiding this comment.
my choice would be is_connected because I think it's much more readable
There was a problem hiding this comment.
while I don't disagree, the micropython network module uses isconnected() and so does mqtt_as so I think it makes more sense to go with that.
| async def connect(self): | ||
| """Serve connect request. Triggers callbacks if state changes""" | ||
| if await self._connect(): | ||
| self._change_state(True) | ||
| return True | ||
| return False |
There was a problem hiding this comment.
There should be a variable that prevents the method from being called when it's already running (e.g. self._is_connecting).
Connecting can take up huge amounts of time and it doesn't make sense to have those running in parallel.
With the variable it's possible to just spawn connect tasks.
There was a problem hiding this comment.
I do agree actually. It is not a problem if only one project/library is controlling the interface but if multiple interfaces try it, then it becomes a problem. A 2nd interface calling connect should just wait for the result of the 1st call.
However, it might be better to create a more advanced interface blueprint in order to keep this one as simple as possible and as close to the source as possible. The goal of the current files is to just separate mqtt_as from the hardware interface without adding features or complexity.
I do agree that more features and complexity is needed for a safe approach handling multiple libraries. So once the current changes/approach is approved, I should add such a blueprint (by making the wlan interfaces better).
Hmm yes this would be debatable, however, the original mqtt_as only uses one callback, so to have compatibility to existing projects, I can't switch to two separate callbacks, even though the change in code is tiny. |
Couldn't we just use the original callback and subscribe it for both callbacks? That way we could have two different callback possibilities for the future while the existing implementation still works. |
|
Having 2 callbacks also raises the question of which arg should be passed. Might still be ok with the wifi state as boolean. |
No args at all. Just a possible callback for connected and disconnected. It actually boils down to "when do you want to do something specific". async def my_connected_func(state):
if not state:
return None
...
async def my_disonnected_func(state):
if state:
return None
...
My implementation is actually already done - I'm just trying to contribute something, too. Could we make a Wifi a base class which holds only the credentials and then inherit the device specific implementation from there? e.g. |
| # If a callback is passed, run it and return. | ||
| # If a coro is passed initiate it and return. | ||
| # coros are passed by name i.e. not using function call syntax. | ||
| def launch(func, tup_args): | ||
| res = func(*tup_args) | ||
| if isinstance(res, _type_coro): | ||
| res = asyncio.create_task(res) | ||
| return res |
There was a problem hiding this comment.
Sorry but I think you somehow mixed callbacks and coroutines in this answer and now it doesn't really make much sense but I think I get your point.
Is a coro that is called on connect not a callback? I am not sure about the correct terminology.
Personally I'd go with callbacks only and not allow coroutines but I have to think about backwards compatibility and the original mqtt_as has a wifi_coro, a coroutine. So I have to allow it in order to not break existing implementations.
If the has a long running sync callback things still can block, that's why I think a coro is better because it's immediately clear they have to be non blocking. And if it's all coros they have to be canceled by the base class.
mqtt_as/interfaces/wlan/esp32.py
Outdated
| # https://forum.micropython.org/viewtopic.php?f=16&t=3608&p=20942#p20942 | ||
| self.BUSY_ERRORS += [118, 119] # Add in weird ESP32 errors | ||
| self._ssid = ssid | ||
| self._wifi_pw = wifi_pw |
There was a problem hiding this comment.
I was thinking about self._pw but kept it like that for now for better recognizability since it was named this way in mqtt_as. Once @peterhinch is happy with the changes, I'll shorten this variable. In this context it is very obvious what it does, so no need for longer names.
Seems like an acceptable workaround.
It is, but building a workaround for backwards compatiblity with one callback would be rather big. Current goal is to stay as small as possible and as compatible as possible.
Yes I might do that, there is some common code for checking wifi stability and the wifi credentials and other ports might get added later. better that they don't need to copy that part too. |
I'm afraid I don't understand your point.
But if you start scheduling coros it gets bloated quickly. |
mqtt_as/mqtt_as.py
Outdated
| try: | ||
| await self.wifi_connect() | ||
| except OSError: | ||
| if not await self._interface.connect(): # TODO: switch to reconnect once PR final. |
There was a problem hiding this comment.
Please could you explain this comment.
There was a problem hiding this comment.
The code in this PR is not modified (yet), I just separated it from the original mqtt_as.
For the new interface to work better, the disconnect and connect here should be replaced as a reconnect, because that's what mqtt_as is requesting here. This way interface implementations can decide wether a reconnect is really needed or not. For your resilient wifi connection it might make sense to reconnect the wlan on ever mqtt connection problem but an ethernet or other implementation might be able to check the connection state reliably enough and decide not to reconnect the interface. This will also be relevant for projects that have multiple applications that rely on the wlan (e.g. additional webserver, http push/get). [Note: detecting a bad wifi link would then still be possible by checking the rate of reconnect requests and if those exceed e.g. 5 requests in 10 seconds, the wlan interface reconnects. But that's up to the user/dev to implement.]
In the default implementation of the WLAN interface, reconnect will default to the code seen here: disconnect, wait 1 sec, connect.
|
What is the state of this PR? Is help needed? |
|
Due to my current health situation I can't even use a PC or phone anymore and it might not change. So I wouldn't mind someone taking over. On 11/22/21, 18:05 Vincent Thibault ***@***.***> wrote:
What is the state of this PR? Is help needed?
—You are receiving this because you authored the thread.Reply to this email directly, view it on GitHub, or unsubscribe.Triage notifications on the go with GitHub Mobile for iOS or Android.
|
|
Sorry to hear that. Thanks for your contribution and take care of yourself. |
|
@kevinkk525 Shocked to hear that! I really hope that you get well again! Best wishes! |
|
Hi Kevin, I hope you are in good health. Is there any chance of completion of this pull request? If this is still not possible, is it possible for you to write what needs to be done? If Kevin can't help with this, is there any chance for you to help @peterhinch ? |
|
I have no plans to work on this. Recent aims have been supporting and testing new WiFi platforms and helping support a low power application which takes down the interface programmatically - so this PR has probably become rather stale. |
Goal:
This is a first draft that hasn't been tested yet. Looking for feedback on the implementation.
(Reminder for later: Changed mqtt_as folder to package due to relative imports. Have to test how that works with backwards compatibility. However, users will have to install this correctly anyway and might then need to change their import line, but nothing else.)
Note about the interface change callbacks: mqtt_as originally called the wifi callbacks after the connection to the broker was established, which looking back was actually a bug imho (line 527). Now it is called directly after the successful wlan connection (which is actually at least 5 seconds later due to the code waiting for wifi stability).
Future thoughts: