search

jpmeijers / RN2483-Arduino-Library

Arduino C++ code to communicate with a Microchip RN2483 module
Apache License 2.0
84 stars 60 forks source link

Make handling commands non-blocking via optional async mode #75

Open TD-er opened 4 years ago

TD-er commented 4 years ago

This rewrite does make the handling of the commands non-blocking, so you can use it along with other tasks on the node.

The interface of the library is kept the same as much as possible.

TD-er commented 4 years ago

This is quite a big (huge) rewrite, so you may also consider it is not really fitting as a pull request for your library. If so, then I will make it a separate repository.

TD-er commented 4 years ago

Apparently Travis failed over the (not yet updated) examples. I still have to fix those examples to be compatible again with the changes.

jpmeijers commented 4 years ago

Wow that is quite an impressive change. Thanks for the contribution!

@jwillemsen what do you think? Should we merge this in?

I'm of the opinion that it can be merged in as long as backwards compatibility is kept (or with minor changes). Also the other Arduino RN2xx3 library - The Things Network library - does things in a synchronous manner, so being asynchronous gives this library a new meaning to exist.

TD-er commented 4 years ago

I am running it in my ESPEasy project as a controller, so it does need to do a lot of other stuff in the background on an ESP8266. Handling the I/O is now quite fast, as most commands can be executed in 20 - 50 msec (@57600 baud) and sending data via mac tx does not take longer than 80 msec to make sure the data is accepted by the module. Well that's with async mode enabled, which is something you have to enable by calling a function to switch on that mode. If you don't enable it, it will work as before, although it will be quite a bit more responsive if no module is connected or not configured at the defined pins.

The used timeouts are:

jwillemsen commented 4 years ago

Lot of changes, haven't look at in detail but there is no new example that shows how to use the async feature, without that it is hard for anyone other to use. That example should also be compiled on travis

Mark-Wills commented 4 years ago

Yes. A library lives or dies not on the quality of its code, but on the quality of its documentation.

On Mon, 17 Feb 2020, 07:38 Johnny Willemsen, notifications@github.com wrote:

Lot of changes, haven't look at in detail but there is no new example that shows how to use the async feature, without that it is hard for anyone other to use. That example should also be compiled on travis

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/jpmeijers/RN2483-Arduino-Library/pull/75?email_source=notifications&email_token=AFAGDCV5BIMBVDN3PYFF5HDRDI5GTA5CNFSM4KWFPLPKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEL5LJ2I#issuecomment-586855657, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFAGDCX6GIERGL7VMTWHOX3RDI5GTANCNFSM4KWFPLPA .

TD-er commented 4 years ago

Sure I will add an example to use the async mode. It was first an initial pull request pending a reply to see if you even would like to consider merging it. But if you like that, I will add a few examples.

TD-er commented 4 years ago

I just added a simple example to illustrate the async mode, but have still to test it myself. I am not near a node with LoRa right now, but got some time to make a quick example.

Maybe you can also test it yourself, and I will later test it myself too and let you know if it is working :)

N.B. I guess there should also be something written about this in documentation files?

Alex9779 commented 3 years ago

Just my two cents, found this library a few days ago and want to adapt for a current project not on Arduino, this PR almost totally rewrites the whole library IMHO. The current master state implements just some convenience methods and shortcuts for simple interaction with the module, bundling certain functions and processes into one call. Except some details it is not really just usable for Arduino. This PR changes that, so I think this should be published as a separated library...

TD-er commented 3 years ago

This PR changes that, so I think this should be published as a separated library...

That's also fine with me. The PR as it is, is now in use on my nodes since Februari, when I created this PR and so far I only found one issue where the module can remain trapped in an "eternal busy state" which requires a reset of the module. That does seem to be a bug in the module itself, but it would be nice to have it at least detected in the library so the state can be set to "requires reset". I do have code for it running on one node, but so far the module just behaved nicely :(

@jpmeijers Please let me know what you think is best to do, merge this or consider it a forked library?

jpmeijers commented 2 years ago

It seems like after merging your first PR https://github.com/jpmeijers/RN2483-Arduino-Library/pull/66 this one has conflicts and can't be merged.

TD-er commented 2 years ago

It seems like after merging your first PR #66 this one has conflicts and can't be merged.

I will have a look at it.

TD-er commented 2 years ago

OK, was working for 40-ish minutes on the merge, then another commit was merged so it would not allow me to commit my merge. :(

Will take another look later today (or tomorrow)

jpmeijers commented 2 years ago

Sorry for that. I was trying to clean up old trivial PRs which only changes single lines and should not break merges.

Funny though, because this PR is pointing to a dedicated branch which should not have changed.

No rush, I'm the one that is years behind on admin on this project. Thanks a lot for all the contributions you made over the years.

TD-er commented 2 years ago

Merges via GitHub (not local) will no longer be accepted when the main branch changed. So then I lost the possibility to commit them.

I will later try to merge it locally, as that does give more insights in what was changed in the main branch.