Change structure to module, ESP32 loboris fork, ESP8266 minimal, different config

[kevinkk525]

Thanks for your awesome work with this mqtt-client!

Changes:

1. Sorted files and made a structure that made sense so you know which file belongs where without reading the documentation every time
2. Made repo a module to be used like

   from micropython_mqtt_as.mqtt_as import MQTTClient
   from micropython_mqtt_as.config import config

   making it possible to just clone the repo and copy it to espXXXX/modules also reducing file clutter in this directory.

3. Adapted code to ESP32 loboris fork as sys.platform returns a different string as the official port and all workarounds for the official port are not needed in this fork except the Wifi credentials.
4. Changed MQTTClient constructor initialization from using a dictionary to using keywords with default parameters.
5. Made a minimal version of mqtt_as for the ESP8266
6. Updated all files to work correctly with these changes
7. Updated documentation to reflect all changes

Motivation for the changes: For my project I had to adapt the library to use it on the ESP32 with loboris fork but also use it on my ESP8266 that is short on RAM all the time. Therefore I had the following motivation for each of the above mentioned changes:

1. I don't like to walk through a mess of files not knowing which one is important or where it belongs to and I don't want to read all the documentation just to know which files belong where.
2. Like all modules this should be a directory as well making usage easier.
3. Made it work with loboris fork but did not want to use workarounds that are not needed on this fork
4. I felt that this kind of initialization is the more pythonic way of doing things but apart from that it has an important advantage on the ESP8266, removing the config dict completely uses 100-200 Bytes less, which is important on ESP8266.
5. This version for the ESP8266 has all non related code and also some not commonly functions removed, saving another 150-250 Bytes so that after all changes I get 250-450 Bytes more RAM.
6. Although I do not need any other file I felt that it is important to finish the work I started and not leave half the repo unusable.
7. Wouldn't want issues because of wrong documentation.

Please let me know what you think of it. I know that especially the config change makes it not just a replacement.

[peterhinch]

Thank you for the contribution. These are substantial changes. I like the fact that the dict is now optional: I should have thought of that. :) The dict has the benefit of providing a way to do per-project configuration, so I wouldn't want to lose the facility, but I take the point about space.

The statement that the Loboris port does not have the reliability issues of the official port begs the question of how thoroughly this has been tested. Another issue is whether we actually still need the horrible ESP32 kludges in the code, which I notice are still present. It's some time since I tested this, perhaps the Loboris port (or even the official one) has improved. I was not planning any major changes to this library until these issues with the ESP32 vendor code had been addressed. If they have, we should get rid of these. If not, perhaps we should wait until we can.

The problem is that thoroughly testing this takes a substantial amount of time and I'm loath to do this until there is reason to believe that the ESP32 is capable of reliable operation without arbitrary kludges. Much of the point of a "resilient" driver is reliability. I believe I achieved this on the ESP8266 but the ESP32 still had issues.

Testing needs to be performed over conditions like WiFi outages, broker outages, slow brokers and internet connectivity failures. Long-running tests (days) near the limit of WiFi range should be done. One telling test is to gradually move the device out of WiFi range and back in again, and repeating. Have you tried this test?

Given that this testing needs to be repeated for ESP8266, ESP32 and ESP32 Loboris perhaps you can appreciate that I'm not keen to undertake it until we can use clean code. But I am very keen to end up with a truly resilient ESP32 solution.

A separate ESP8266 codebase does introduce a maintenance and testing issue which I have tried to avoid.

One issue I'd planned to address is to remove the Sonoff code. I never achieved satisfactory performance with the Sonoff device which I consider to have hardware issues.

I look forward to hearing your views.

[kevinkk525]

Thanks for your answer, I like to have a discussion about this PR. The problem with a mandatory config dict is that you have to import it in your main script and fill it by using the config file of your own project. That makes it either redundant or you would use a config file/dict of some module as your project config which is less modular and harder to unterstand. Neither of these cases was appealing to me, so I made it optional. Those who wish to use it can still do so with little changes in their existing code and all others can still keep the modules separate and use a standard way to configure each module.

You are absolutely right about the loboris fork, I did not test it that thoroughly. I also consider the loboris fork still experimental when it comes to using uasyncio. Just about a month ago loboris fixed a few bugs I found that prevented uasyncio from working at all. And even now I get occasional overflow errors. So this is still experimental but if someone wants to use your library with that fork he will be surprised as it won't correctly work and crash on a reconnect as the library thinks you are using an esp8266 (his sys.platform returns "ESP32_LoBo") and calls wifi.connect without credentials. So this is firstly a change to make it work with this fork. But as the idle() call is definitely not neccessary on his fork, I decided to remove the other workarounds as well as I did not get any obvious problems without these and I did not like some blocking sleep_ms calls in it. But you are right that this should be better tested for stability. Maybe it's a solution to clearly state that ESP32 (especially loboris fork) is still experimental (but your library is still better than anything similar out there). Or make a separate development branch if you prefer this. I'd just not prefer to send people to my fork all the time because your repo lacks the support for this fork. That will only create confusion.

About the tests I did: Most of these I have not done. I tested with good wifi signal, wifi signal loss (completely) and broker restart. I don't have an ESP32 running 24/7 yet as I'm in the process of finishing my project for publication and had optimize it heavily to fit onto the ESP8266 RAM demands again.

This is not true though, I did not change any of the core behaviours. Check the diffs and you will see that the changes are mostly "cosmetic" and even the ESP8266 minimal version is just stripped of code that is not run on the ESP8266 platform anyways and a few convenience functions like "wan_ok". Even the official ESP32 port is still the same. So no additional testing for the existing platforms is needed. They still run the same code. (Changes with no obvious change were just autopep8).

As pointed out in the paragraph before, the minimal version for the ESP8266 is just a simple stripped down version with no deep changes and should be very easy to maintain additionally. Changes to codebase can be done 1:1 in the minimal version while every workaround added in the main codebase won't go to the minimal version. Also ESP8266 is really stable so I would not expect many changes in the future. On the other hand, if your codebase will grow in the future, a minimal version will be even more neccessary as the ESP8266 runs out of RAM quite quickly.

Sorry for my long answer with strangely formed sentences :D I hope you understand it.

[peterhinch]

By your own reckoning the ESP32/Loboris port is still unstable. A key aim of this repo is to be "resilient". It's clear from your testing that the state remains the same as when I released it: ESP8266 is resilient, ESP32 is not. I see no point in doing an update until the vendor code/MicroPython port are stable. Frankly my view is that the ESP32 is not yet fit for purpose.

I would budget around two weeks work to prove the resilience of each port. Currently a negative result is assured.

So my judgement is that we should wait until preliminary testing such as you have performed demonstrates a chance of success.

[kevinkk525]

I completely understand your point. Therefore I'd propose leaving this PR open until the ESP32/loboris fork is considered stable. I'd also appreciate a statement in the README or as an issue about the existence of an experimental version of this library for loboris fork with a link to this PR and/or my fork (branch "module"). That way I could still send people to your repo no matter what fork/microcontroller they use and they'd find the solution they are looking for so development for ESP32/loboris fork is possible without everyone checking how to make it work for that fork but still keeping your repo completely "resilient". What do you think is the best course of action?

[peterhinch]

I have modified my mqtt_as.py to include your code for detecting the Loboris port, so my code will run albeit with the nasty blocking delays.

I've updated the README.md to include a reference to your fork, with a note that it has removed these delays.

Thank you for the work you've done on this. I plan to issue an update later in the year. There are several reasons for the delay. Firstly I'm working on other things. Secondly I think we're agreed that the ESP32 is not yet stable enough for a truly resilient solution. Finally I plan to support an additional hardware platform. I hope to receive development samples in a month or two. I'm afraid I can't be more specific about this.

[kevinkk525]

Thanks a lot, this should be very helpful.

Happy to hear that there will be a new platform soon. Have fun testing, I hope it will pass all tests. I understand that there are more important projects you are working on. I'm looking forward to it. Thanks for all your effort.

I think we found a good compromise on this PR.

[thijstriemstra]

saving another 150-250 Bytes so that after all changes I get 250-450 Bytes more RAM.

+1!!