TheThingsNetwork / arduino-device-lib

Arduino Library for TTN Devices
MIT License
208 stars 96 forks source link

Disable retries by RN2483 itself. #214

Closed jpmeijers closed 7 years ago

jpmeijers commented 7 years ago

Fixes #213.

johanstokking commented 7 years ago

Why?

jpmeijers commented 7 years ago

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:

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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