Open kevinkk525 opened 5 years ago
The pyboard D commit looks good. Will you take a look at these changes too? Notably 1,2,4,6,7,8 and maybe the pause/resume feature I implemented on a different branch?
I haven't forgotten these, but they do impose substantial API changes on existing users. By contrast my latest commit has zero API changes :)
Re 1,2,4 I'm still unsure whether the gains, particularly from using a package and replacing the config dictionary, justify the API disruption. I originally started out with lots of constructor args and changed to the dict. This does have some advantages. For example it is initialised with default args in the library but you can modify it with project-level defaults in config.py. So, in a project where (say) the server IP or WiFi credentials change, you just distribute a new config.py. You can, of course, pass **config to the multi-parameter constructor, but if you're going to do that you might as well stick with the dict.
7 (retained messages) is definitely worth doing. Re 6, 8 I'm not convinced about unsubscribing: are embedded applications likely to unsubscribe?
Right now I'm busy trying to understand some weird behaviour of Pyboard D applications and am trying to produce a decent test case.
Thanks for your answer. I can understand that you are worried about API changes to existing users. This however always slows down progress. Maybe an updated version with the current API can be kept on a separate branch that'll only receive bugfixes once stable?
1,2,4 would be nice to have if a new version already changes the API. The advantages you see in using a dict are not what I can see. With normal constructor args you can have all those default values too and you will have a project-level config anyway that will store the needed values that will overwrite the default ones and can therefore still be easily swapped by distributing a new config.py.
7 (retained messages) is where the biggest API change lays because the function that is being called when a new message is received now takes 3 args instead of 2.
6 unsubscribe: I use it often. On topic "home/device/state" I keep the retained status that the device has. On topic "home/device/state/set" the device receives commands to change that state to a new state. So on reboot my devices subscribes to the state topic ("home/device/state") to get the retained value and restores the state it had before its reboot. Then it unsubscribes that topic because it does not need that anymore to free resources (as you need a higher level subscription handler to know what function to call on which topic). Then it subscribes to the command topic ("home/device/state/set") and listens to changes that might even have occured during its outage. I don't know if that's the best way for restoring device states but I did not find anything better yet. Therefore I don't know if embedded applications are likely to use unsubscribe but mine do. It is however not a big code change to implement unsubscribe and does only extend the API, not break compatibility.
8 is also useful with only subscribe as I had problems subscribing from different uasyncio coros in short succession.
Pause/Resume might be helpful for battery powered devices to properly disconnect from the server to save power.
Once your new version is stable and ready, I can pull those changes and create pull requests only with the changes you want to implement. That will hopefully make it easier for you.
Good luck hunting down the problems with the Pyboard D! Hope you can find it soon.
Just as an update as time has passed:
Ah yes, I forgot that this PR pulls all the changes from my master repository. I guess this can be closed since you won't be merging the remaining changes anyway.
This pull request is merely to give you an overview of the changes I made, so you can have them in mind when looking at the code. I know you'll only start working on this once you check the module for the new hardware and retest the reliability. Thanks for your efforts with this module.
Changes: