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

non blocking functions added #62

Merged
merged 5 commits into from
Aug 19, 2018

Conversation

universam1
Copy link
Contributor

the endPacket() is a blocking function that can take over 6 seconds depending on the radio settings. Here is a non blocking version added, tested successfully on ESP8266. Note the grace time required for the radio to properly update the registers.

@sandeepmistry
Copy link
Owner

@universam1 thanks for submitting!

Have you seen these API's in any other packet based Arduino libraries? Just wondering if there's existing API's we can leverage.

@universam1
Copy link
Contributor Author

Well, I would say it is actually more of a standard then exceptional to support non blocking calls (http://bfy.tw/FAaI)
There are even mcu's like ESP8266 that are built in an assumption that calls are
non blocking to happen. That's why my other PR #61 I would rather call a dirty workaround then a proper implementation.

So out of my head lets mention
https://github.com/sadr0b0t/stepper_h
arduino/Arduino#1240
http://www.epyon.be/2016/05/22/non-blocking-ethernet-example-teensy arduino/Arduino#1476
https://github.com/jrullan/neotimer
https://github.com/olewolf/DHT_nonblocking
and numerous other ones I came across.

Don't get me wrong, I consider your library is great, but non blocking functions are mandatory for me and turn a great library to professional.
Of course if you like different function names, no problem!

@universam1
Copy link
Contributor Author

Would like to get this moving and reach an agreement if you @sandeepmistry and @morganrallen are happy with this implementation or rather overload the existing function with an async parameter?

@sandeepmistry
Copy link
Owner

I'd like to take a look at how other existing Stream based Arduino libraries handle async. The links from #62 (comment) are not necessarily Stream based.

Maybe nothing exists yet?

@universam1
Copy link
Contributor Author

I agree a streaming support might be desirable at the long run! Though this would be a bigger step.
Following DevOps style it is normally preferable to release often but smaller.
If a feature can be implemented without breaking backward compatibility this can be considered good to deploy.
But of course it depends on your philosophy for this project!

@sandeepmistry
Copy link
Owner

Some API ideas:

  • have either endPacket (or beginPacket) have an optional API for async mode
  • then beginPacket fails (returns 0), if a packet is currently been sent by the radio.

@universam1 universam1 force-pushed the nonblocking branch 2 times, most recently from 8806e51 to 7efaf8c Compare March 8, 2018 20:28
@universam1
Copy link
Contributor Author

@sandeepmistry Implemented:

  • endPackt has the optional parameter bool async
  • and beginPacket fails if currently sending

Please review if that is fine, thanks

Repository owner deleted a comment from MauroMombelli Mar 8, 2018
Copy link
Owner

@sandeepmistry sandeepmistry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@universam1 Good progress!

I think this is good to merge after my two comments are addressed and the code style (mainly brackets) matches the existing code base. The API docs will also have to be updated.

while (!Serial)
;

Serial.println("LoRa Sender");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe adding "non-blocking" to the title here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine with me

src/LoRa.h Outdated
@@ -22,7 +22,8 @@ class LoRaClass : public Stream {
void end();

int beginPacket(int implicitHeader = false);
int endPacket();
int endPacket(bool async = false);
bool isTransmitting();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer if this API was private for now.

Copy link
Collaborator

@morganrallen morganrallen Jul 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sandeepmistry Does this mean, leave int endPacket() in public and move int endPacket(bool async = false) private? Don't know C++ classes well enough to understand what this will accomplish.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@morganrallen I was thinking isTransmitting() should be private for now.

With the current PRbeginPacket(...), fails if there is a transmission in progress.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@morganrallen any thoughts on the above?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, thanks makes sense, missed this comment last week.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved into private

Repository owner deleted a comment from MauroMombelli Mar 12, 2018
@morganrallen
Copy link
Collaborator

I've checked this PR out and it appears to work so I'm inclined to merge it as it's an important feature. I'll address the comments and merge this evening if there are no objections.

@sandeepmistry @universam1

@morganrallen morganrallen self-assigned this Jul 30, 2018
@universam1
Copy link
Contributor Author

fine with me 👍

@sandeepmistry
Copy link
Owner

API.md also needs to be updated accordingly for the new supported values.

@universam1
Copy link
Contributor Author

@morganrallen @sandeepmistry updated the PR accordingly, LGTM

API.md Outdated
@@ -103,6 +103,34 @@ LoRa.endPacket()

Returns `1` on success, `0` on failure.

## Sending data - non blocking
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should have a totally new section for non-blocking, rather modify the existing sending section, since there is really just a new parameter to endPacket and beginPacket has another reason to fail.

@morganrallen any thoughts on this?

@universam1
Copy link
Contributor Author

@sandeepmistry @morganrallen I've changed the API accordingly and removed the additional part merging now.
Please though LGTM, our review processes take way to long!

@universam1
Copy link
Contributor Author

all requests fullfilled, if no further objections @sandeepmistry I understand it's fine to merge

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.

3 participants