Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 7 commits into
base: feature/async
Choose a base branch
from

Conversation

TD-er
Copy link
Collaborator

@TD-er TD-er commented Feb 16, 2020

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.

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
Copy link
Collaborator Author

TD-er commented Feb 16, 2020

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
Copy link
Collaborator Author

TD-er commented Feb 16, 2020

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

@jpmeijers
Copy link
Owner

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
Copy link
Collaborator Author

TD-er commented Feb 16, 2020

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:

  • 1500 msec for any command's first reply
  • rxdelay2 + 3000 msec for mac tx replies in the rx2 window.
  • 10 seconds for OTAA join (after the first reply)

@jwillemsen
Copy link
Collaborator

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
Copy link

Mark-Wills commented Feb 17, 2020 via email

@TD-er
Copy link
Collaborator Author

TD-er commented Feb 17, 2020

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.

This is some example for to illustrate the async mode.
@TD-er
Copy link
Collaborator Author

TD-er commented Feb 17, 2020

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?

The example code using the library should not make assumptions about the async mode, but check with the library to know what mode is active.
@jwillemsen jwillemsen removed their request for review August 25, 2020 16:31
@Alex9779
Copy link

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
Copy link
Collaborator Author

TD-er commented Sep 18, 2020

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 jpmeijers changed the base branch from master to feature/async October 4, 2021 13:15
@jpmeijers
Copy link
Owner

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

@TD-er
Copy link
Collaborator Author

TD-er commented Oct 4, 2021

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
Copy link
Collaborator Author

TD-er commented Oct 4, 2021

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
Copy link
Owner

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
Copy link
Collaborator Author

TD-er commented Oct 4, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants