Adding attempts to readLine and state marker if radio module is unres… #244

savnik · 2018-08-17T14:49:26Z

Proposal to fix issue #242

…ponsive

CLAassistant · 2018-08-17T14:49:33Z

All committers have signed the CLA.

johanstokking

Thanks!

johanstokking · 2018-08-21T09:18:47Z

src/TheThingsNetwork.cpp

+    this->radioModuleInvalidState = true; // Inform the application about the radio module is not responsive.
+    return 0;
+  }
+  else{


Please make spacing consistent around braces and brackets. Also no need for else after a return.

johanstokking · 2018-08-21T09:19:15Z

src/TheThingsNetwork.cpp

@@ -357,15 +357,21 @@ void TheThingsNetwork::clearReadBuffer()
  }
 }

-size_t TheThingsNetwork::readLine(char *buffer, size_t size)
+size_t TheThingsNetwork::readLine(char *buffer, size_t size, uint8_t attempts = 3) // Default timeout value 10s and is set in class initiator


Where is it set?

timeout is set in:

arduino-device-lib/src/TheThingsNetwork.cpp

Line 294 in 4629038

this->modemStream->setTimeout(10000);

Set defaults in the header file

savnik · 2018-09-14T10:13:52Z

@johanstokking did you have time to review the changes?

johanstokking · 2018-09-26T20:07:17Z

src/TheThingsNetwork.cpp

@@ -357,15 +357,21 @@ void TheThingsNetwork::clearReadBuffer()
  }
 }

-size_t TheThingsNetwork::readLine(char *buffer, size_t size)
+size_t TheThingsNetwork::readLine(char *buffer, size_t size, uint8_t attempts = 3) // Default timeout value 10s and is set in class initiator


Set defaults in the header file

johanstokking · 2018-09-26T20:07:35Z

src/TheThingsNetwork.cpp

 {
  size_t read = 0;
-  while (read == 0)
+  while (read == 0 && attempts>0)


Spacing; attempts > 0

Shouldn't you be decrementing attempts somewhere? Like while (!read && attempts--) ?

johanstokking · 2018-09-26T20:09:44Z

src/TheThingsNetwork.cpp

  {
    read = modemStream->readBytesUntil('\n', buffer, size);
  }
+  if(attempts<=0)
+  { // If attempts is activated return 0 and set RN state marker
+    this->radioModuleInvalidState = true; // Inform the application about the radio module is not responsive.


Although this is a way to signal, still it requires action from the application. I don't really know if application developers know how to resolve this.

The best way to resolve this is to reset the RN module through the reset pin. Would it be a good idea to configure (optionally) the reset pin, and engage a soft reset in the library at this point?

I see your valid point. Now a good question is should we let the library handle the reset or give the decision to the application. If the library is resetting the RN module we will need to let the application know so the application can handle this, eg. by re-joining the network.
But in either way we must raise a flag to the application and abort the current task.
If we reset the RN module in the library we can do a soft-reset by sending a sys_reset cmd or a hard-reset using the reset pin. In either way the library needs to rejoin the network and then re-execute the function. Should the module then retry for infinity or a limited tries?

One option could be to soft/hard reset the module every time this error occur, print a debug message with details and raise the flag. In this way we have raised awareness of a problem and tried to resolve it while giving power over the exception handling to the application.

readLine is used in:

readResponse

personalize

join

sendBytes

waitForOk

However maybe the simple solution is the best. On error exit with debug message and error flag. Then update documentation and make a good example on how to handle the error on application level.

My wording was wrong; I meant hard reset, by using the reset pin. Sending the soft reset command is not going to work as the serial is broken already.

And you're right, doing a hard reset in the midst of a join or TX operation, and restoring the state (i.e. joining again) is not going to work either.

So I'm good with the signaling to the application. Please rename to needsHardReset, introduce a resetHard() that takes a pin number as parameter, and reset the needsHardReset after hard resetting. Also documentation would be helpful. Thanks!

Okay. I will work on this and come back soon.

johanstokking · 2018-09-26T20:10:14Z

src/TheThingsNetwork.h

  void (*messageCallback)(const uint8_t *payload, size_t size, port_t port);

  void clearReadBuffer();
-  size_t readLine(char *buffer, size_t size);
+  size_t readLine(char *buffer, size_t size, uint8_t attempts);


So add the default always in the header file

… a public variable

johanstokking

Good. Almost there

src/TheThingsNetwork.cpp

johanstokking · 2018-10-29T13:24:52Z

src/TheThingsNetwork.cpp

+  unsigned long time_now = milis();
+  int period = 1000;
+  while(millis() < time_now + period){
+    // wait


Why not use delay(), and do we need a whole second?

It was just to make it non-blocking in case the application is using interrupt. The reset function is not time critical.
I have looked in the datasheet and there is no advice on minimum time for the signal for a reset to be triggered. I know that 1s works by test, but I have not tested <1s.

Actually delay() is the better one to use. On some platforms where it is necessary delay() will call yield(), while a while loop will not.

Can you implement this @savnik ?

…nstant too long for its type

johanstokking

The rest is good.

I would like to see consistent spacing, i.e. before and after (...) and around operators, though.

docs/TheThingsNetwork.md

johanstokking · 2018-12-05T10:14:18Z

src/TheThingsNetwork.cpp

+  unsigned long time_now = milis();
+  int period = 1000;
+  while(millis() < time_now + period){
+    // wait


Can you implement this @savnik ?

…reset

…standable

savnik · 2018-12-05T20:57:05Z

So the wait function is implemented. I have tried to make it more clear now.
Maybe we do not see the same regarding spacing - but i can not see where the spacing is wrong now?

@jpmeijers

Letting this over to @jpmeijers

jpmeijers · 2018-12-15T16:19:27Z

Only saw the review request now. I'll try and give this PR a proper look and review during the coming week.

jpmeijers

Only two small changes please. For the rest I am happy with this PR.

src/TheThingsNetwork.cpp

docs/TheThingsNetwork.md

ElectronicallyE · 2018-12-28T03:49:16Z

Thanks guys on making progress on this. I was about to write up an issue about this, but seems like you've got a handle on it already. Would be glad to start using TTN as soon as this is resolved as I have be unable to because of this hanging problem.

…8266 background operations will work

johanstokking · 2018-12-28T13:26:58Z

@jpmeijers good to go now?

savnik · 2019-01-09T19:56:23Z

@jpmeijers have you had time to take a look at the two changes you sugested?

jpmeijers

lgtm

johanstokking · 2019-01-14T18:38:16Z

Released in https://github.com/TheThingsNetwork/arduino-device-lib/releases/tag/v2.5.14

Thanks @savnik and @jpmeijers

Adding attempts to readLine and state marker if radio module is unres…

4629038

…ponsive

johanstokking self-requested a review August 21, 2018 09:16

johanstokking self-assigned this Aug 21, 2018

johanstokking reviewed Aug 21, 2018

View reviewed changes

savnik added 3 commits August 24, 2018 11:05

Fix spacing around brackets

0653d8d

Adding attempts to TheTHingsNetwork.h

012e651

No need for else when if has return

9693732

johanstokking requested changes Sep 26, 2018

View reviewed changes

savnik added 6 commits October 3, 2018 13:40

Set default in header file

77403c6

decrement attempts

2a1968e

Proposal to handle exception when RN module does not respond

91d5b8b

Changed variable name from radioModuleInvalidState to needsHardReset

2df5307

Added hardReset function with non-blocking delay. made needsHardReset…

d4a6ce5

… a public variable

Updating documentation for hardreset

72fc4f2

johanstokking requested changes Oct 29, 2018

View reviewed changes

savnik added 6 commits December 1, 2018 03:52

Changed wording of unresponsive rn module in readline function

71e3d0a

Spacing in if statement in readline function

833ec1e

Removing comment at readline function

0fc3bc2

Fixed spelling mistake of millis in resetHard

d74d17d

Replacing single qoute with double qoute to fix warning: character co…

fe0c8ab

…nstant too long for its type

remove empty line

a0afabf

johanstokking previously requested changes Dec 5, 2018

View reviewed changes

savnik added 5 commits December 5, 2018 21:40

Fixed missing (

69ef443

moved adr parameter to reset and added parameter description for hard…

896a8aa

…reset

Spelling mistake

88c81a0

Reformulated the non i/o blocking wait function to make it more under…

13b554f

…standable

Updated the way while brackets are placed in resetHard

8c8cb8c

johanstokking requested a review from jpmeijers December 6, 2018 12:57

jpmeijers requested changes Dec 22, 2018

View reviewed changes

src/TheThingsNetwork.cpp Show resolved Hide resolved

docs/TheThingsNetwork.md Show resolved Hide resolved

savnik added 2 commits December 22, 2018 18:08

Updated hardreset documentation to include initial pinmode setup

8ec0a25

rewording of hardreset output pin config

a17f5e4

ElectronicallyE mentioned this pull request Dec 28, 2018

RN2903 and Arduino Uno not Working with Library #248

Closed

Using delay(1000) instead of custom delay function to ensure that ESP…

87dacf4

…8266 background operations will work

jpmeijers approved these changes Jan 12, 2019

View reviewed changes

johanstokking merged commit 389ab70 into TheThingsNetwork:master Jan 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding attempts to readLine and state marker if radio module is unres… #244

Adding attempts to readLine and state marker if radio module is unres… #244

savnik commented Aug 17, 2018

CLAassistant commented Aug 17, 2018 •

edited

Loading

johanstokking left a comment

johanstokking Aug 21, 2018

savnik Aug 24, 2018

johanstokking Aug 21, 2018

savnik Aug 24, 2018

johanstokking Sep 26, 2018

savnik commented Sep 14, 2018

johanstokking Sep 26, 2018

johanstokking Sep 26, 2018 •

edited

Loading

johanstokking Sep 26, 2018

johanstokking Sep 26, 2018

savnik Oct 3, 2018

savnik Oct 3, 2018

johanstokking Oct 9, 2018

savnik Oct 12, 2018

johanstokking Sep 26, 2018

johanstokking left a comment

johanstokking Oct 29, 2018

savnik Oct 29, 2018

jpmeijers Dec 3, 2018

johanstokking Dec 5, 2018

johanstokking left a comment

johanstokking Dec 5, 2018

savnik commented Dec 5, 2018

jpmeijers commented Dec 15, 2018

jpmeijers left a comment

ElectronicallyE commented Dec 28, 2018 •

edited

Loading

johanstokking commented Dec 28, 2018

savnik commented Jan 9, 2019

jpmeijers left a comment

johanstokking commented Jan 14, 2019

Adding attempts to readLine and state marker if radio module is unres… #244

Adding attempts to readLine and state marker if radio module is unres… #244

Conversation

savnik commented Aug 17, 2018

CLAassistant commented Aug 17, 2018 • edited Loading

johanstokking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savnik commented Sep 14, 2018

Choose a reason for hiding this comment

johanstokking Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanstokking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanstokking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savnik commented Dec 5, 2018

jpmeijers commented Dec 15, 2018

jpmeijers left a comment

Choose a reason for hiding this comment

ElectronicallyE commented Dec 28, 2018 • edited Loading

johanstokking commented Dec 28, 2018

savnik commented Jan 9, 2019

jpmeijers left a comment

Choose a reason for hiding this comment

johanstokking commented Jan 14, 2019

CLAassistant commented Aug 17, 2018 •

edited

Loading

johanstokking Sep 26, 2018 •

edited

Loading

ElectronicallyE commented Dec 28, 2018 •

edited

Loading