Skip to content

Conversation

@jpmeijers
Copy link
Collaborator

Fixes #213.

@jpmeijers jpmeijers requested a review from johanstokking June 20, 2017 11:46
@johanstokking
Copy link
Member

Why?

@jpmeijers
Copy link
Collaborator Author

jpmeijers commented Jun 22, 2017

Say you are sending a 51 byte confirmed packet on SF12, using 1 retry (and we never get an ack).
T_packet = 2.3s
Which means a 230s silent time when keeping to 1% duty cycle. When sending the mac tx cnf command to the radio, after receiving the ok, we will have to wait 2*230s before receiving either a mac_err or one of the other possible responses.

This causes two issues:

  • For the user it is unclear why the send function is blocking for such a long time.
  • If we merge Feature/prevent inf loop #210, waiting for a response will timeout after 10s. The library will then continue, assuming the radio is in a different state than it is.

Increasing the timeout for readLine() is possible, but only to a maximum of 255 seconds because the timeout value is a uint8_t. For the case of 1 retry 255s < (2*230s), which means the readLine will timeout before the RN is done retrying.

We can solve this by increasing the size of the timeout parameter of the readLine() function. This however still leaves the user wondering why the send function is blocking for such a long time. With the default of 7 retries it will be (8*230s)=1840s=30.7 minutes.

I find it a cleaner solution to remove all retries that is done by the radio itself. Then the send function can return within a matter of seconds, telling the user if the attempt was successful or not. It is then up to the user if he wants to retry directly, or go to sleep for half an hour and then try again. The latter is much more energy efficient and transparent.

Also have a look at my description in #213

@johanstokking
Copy link
Member

That's an accurate description of the functionality and it's the reason why it's in here.

When people want confirmed uplink, they pass true to that parameter. Sure, it will take longer, but at least they don't have to bother with retries themselves.

We're not changing the default value because it's a nice feature to have by default and this PR changes behavior so it may break existing solutions.

@johanstokking
Copy link
Member

An alternative is providing an optional parameter in the constructor to override this default value of 7.

Also note that only in EU868 this may cause serious delays.

@jpmeijers
Copy link
Collaborator Author

Whatever we choose to do, we need to make sure #210 doesn't cause issues. Of course it is a possibility not adding the infinite loop protection, but I feel more comfortable with it.

@johanstokking
Copy link
Member

The RN module will respond eventually, so #210 is not necessary either.

@johanstokking johanstokking deleted the feature/213-no-retries branch June 22, 2017 16:04
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