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

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

Merged
merged 24 commits into from
Jan 14, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
4629038
Adding attempts to readLine and state marker if radio module is unres…
savnik Aug 17, 2018
0653d8d
Fix spacing around brackets
savnik Aug 24, 2018
012e651
Adding attempts to TheTHingsNetwork.h
savnik Aug 24, 2018
9693732
No need for else when if has return
savnik Aug 24, 2018
77403c6
Set default in header file
savnik Oct 3, 2018
2a1968e
decrement attempts
savnik Oct 3, 2018
91d5b8b
Proposal to handle exception when RN module does not respond
savnik Oct 3, 2018
2df5307
Changed variable name from radioModuleInvalidState to needsHardReset
savnik Oct 26, 2018
d4a6ce5
Added hardReset function with non-blocking delay. made needsHardReset…
savnik Oct 26, 2018
72fc4f2
Updating documentation for hardreset
savnik Oct 26, 2018
71e3d0a
Changed wording of unresponsive rn module in readline function
savnik Dec 1, 2018
833ec1e
Spacing in if statement in readline function
savnik Dec 1, 2018
0fc3bc2
Removing comment at readline function
savnik Dec 1, 2018
d74d17d
Fixed spelling mistake of millis in resetHard
savnik Dec 1, 2018
fe0c8ab
Replacing single qoute with double qoute to fix warning: character co…
savnik Dec 1, 2018
a0afabf
remove empty line
savnik Dec 1, 2018
69ef443
Fixed missing (
savnik Dec 5, 2018
896a8aa
moved adr parameter to reset and added parameter description for hard…
savnik Dec 5, 2018
88c81a0
Spelling mistake
savnik Dec 5, 2018
13b554f
Reformulated the non i/o blocking wait function to make it more under…
savnik Dec 5, 2018
8c8cb8c
Updated the way while brackets are placed in resetHard
savnik Dec 5, 2018
8ec0a25
Updated hardreset documentation to include initial pinmode setup
savnik Dec 22, 2018
a17f5e4
rewording of hardreset output pin config
savnik Dec 22, 2018
87dacf4
Using delay(1000) instead of custom delay function to ensure that ESP…
savnik Dec 28, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 16 additions & 9 deletions src/TheThingsNetwork.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Where is it set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

timeout is set in:

this->modemStream->setTimeout(10000);

Copy link
Member

Choose a reason for hiding this comment

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

Set defaults in the header file

{
size_t read = 0;
while (read == 0)
while (read == 0 && attempts>0)
Copy link
Member

Choose a reason for hiding this comment

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

Spacing; attempts > 0

Copy link
Member

Choose a reason for hiding this comment

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

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

{
read = modemStream->readBytesUntil('\n', buffer, size);
}
buffer[read - 1] = '\0'; // set \r to \0
return read;
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.
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

return 0;
}
else{
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

buffer[read - 1] = '\0'; // set \r to \0
return read;
}
}

size_t TheThingsNetwork::readResponse(uint8_t prefixTable, uint8_t index, char *buffer, size_t size)
Expand Down Expand Up @@ -417,7 +423,7 @@ void TheThingsNetwork::reset(bool adr)
size_t length = readResponse(SYS_TABLE, SYS_RESET, buffer, sizeof(buffer));

autoBaud();
length = readResponse(SYS_TABLE, SYS_TABLE, SYS_GET_VER, buffer, sizeof(buffer));
length = readResponse(SYS_TABLE, SYS_TABLE, SYS_GET_VER, buffer, sizeof(buffer));

// buffer contains "RN2xx3[xx] x.x.x ...", splitting model from version
char *model = strtok(buffer, " ");
Expand All @@ -436,6 +442,7 @@ void TheThingsNetwork::reset(bool adr)
sendMacSet(MAC_ADR, "off");
}
this->adr = adr;
this->radioModuleInvalidState = false;
}

void TheThingsNetwork::saveState()
Expand Down Expand Up @@ -776,8 +783,8 @@ void TheThingsNetwork::configureKR920_923()
void TheThingsNetwork::configureIN865_867()
{
sendMacSet(MAC_ADR, "off"); // TODO: remove when ADR is implemented for this plan
sendMacSet(MAC_RX2, "2 866550000"); // SF10
sendMacSet(MAC_RX2, "2 866550000"); // SF10

// Disable the three default LoRaWAN channels
sendChSet(MAC_CHANNEL_STATUS, 0, "off");
sendChSet(MAC_CHANNEL_STATUS, 1, "off");
Expand Down Expand Up @@ -1035,7 +1042,7 @@ void TheThingsNetwork::sleep(uint32_t mseconds)
}

void TheThingsNetwork::wake()
{
{
autoBaud();
}

Expand All @@ -1051,7 +1058,7 @@ void TheThingsNetwork::linkCheck(uint16_t seconds)
modemStream->write(buffer);
modemStream->write(SEND_MSG);
debugPrintLn(buffer);
waitForOk();
waitForOk();
}

uint8_t TheThingsNetwork::getLinkCheckGateways()
Expand Down
1 change: 1 addition & 0 deletions src/TheThingsNetwork.h
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ class TheThingsNetwork
bool adr;
char buffer[512];
bool baudDetermined = false;
bool radioModuleInvalidState = false;
void (*messageCallback)(const uint8_t *payload, size_t size, port_t port);

void clearReadBuffer();
Expand Down