Skip to content

Fixed LoRa module not waked from sleep by autoBaud sometimes #231

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.

Sign up for GitHub

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

Merged
merged 13 commits into from
Apr 4, 2018
Merged

Fixed LoRa module not waked from sleep by autoBaud sometimes #231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
ff4d97c
fixed RN2xxx not waked from sleep by autoBaud sometimes
hallard Mar 1, 2018
ed31a8a
changes requested by johan
hallard Mar 4, 2018
fa0079f
changes requested by johan
hallard Mar 4, 2018
2193f9e
johan request
hallard Mar 4, 2018
4be44af
cosmetic
hallard Mar 4, 2018
2634eb0
HAL abstration of modemStream
hallard Mar 5, 2018
1b3cdf9
Ability to use AltSoftSerial Library
hallard Mar 6, 2018
cfb5679
Code optimization, added ~700 bytes flash for sketch
hallard Mar 6, 2018
bab08ab
HardwareSerial only used on TheThings Node and Uno
hallard Mar 28, 2018
a164a60
Applied changes requested by johan
hallard Mar 29, 2018
0a8141e
Fixed dual check
hallard Mar 29, 2018
48f3239
cosmetic
hallard Mar 29, 2018
ee3f781
Added Hardware Serial only for the things node
hallard Mar 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
180 changes: 71 additions & 109 deletions src/TheThingsNetwork.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ uint8_t receivedPort(const char *s)
return port;
}

TheThingsNetwork::TheThingsNetwork(Stream &modemStream, Stream &debugStream, ttn_fp_t fp, uint8_t sf, uint8_t fsb)
TheThingsNetwork::TheThingsNetwork(SerialType &modemStream, Stream &debugStream, ttn_fp_t fp, uint8_t sf, uint8_t fsb)
{
this->debugStream = &debugStream;
this->modemStream = &modemStream;
Expand Down Expand Up @@ -387,6 +387,12 @@ size_t TheThingsNetwork::readResponse(uint8_t prefixTable, uint8_t indexTable, u
return readLine(buffer, size);
}

size_t TheThingsNetwork::checkModuleAvailable()
{
// Send sys get ver check we have an answer
return readResponse(SYS_TABLE, SYS_TABLE, SYS_GET_VER, buffer, sizeof(buffer));
}

void TheThingsNetwork::autoBaud()
{
// Courtesy of @jpmeijers
Expand All @@ -396,19 +402,51 @@ void TheThingsNetwork::autoBaud()
while (attempts-- && length == 0)
{
delay(100);
// Send break + Autobaud
modemStream->write((byte)0x00);
modemStream->write(0x55);
modemStream->write(SEND_MSG);
sendCommand(SYS_TABLE, 0, true, false);
sendCommand(SYS_TABLE, SYS_GET, true, false);
sendCommand(SYS_TABLE, SYS_GET_VER, false, false);
modemStream->write(SEND_MSG);
length = modemStream->readBytesUntil('\n', buffer, sizeof(buffer));
// check we can talk
length = checkModuleAvailable();

// We succeeded talking to the module ?
baudDetermined = (length > 0) ;
}
delay(100);
clearReadBuffer();
modemStream->setTimeout(10000);
baudDetermined = true;
}

bool TheThingsNetwork::isSleeping()
{
return !baudDetermined;
}

void TheThingsNetwork::wake()
{
// If sleeping
if (isSleeping())
{
// Send a 0 at lower speed to be sure always received
// as a character a 57600 baud rate
modemStream->flush();
#ifdef HARDWARE_UART
modemStream->begin(2400);
#endif
modemStream->write((uint8_t) 0x00);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the reason for all these (breaking) changes just so that we can send the break condition at a lower baudrate? Won't it be simpler to do it differently like bitbanging the break condition on the correct pins, using digitalWrite() and delay()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reasons of the changes are Low Power. Waking 32U4 (or other) from sleep by the watchdog every 8s for doing nothing is a non sense when you have a module such like the RN2483 than can wake you after a given time with sleep command. The wake up occur when the module sends data to the serial line (that wake up the Atmel device). Unfortunately sometimes with this technique, and we can reproduce the problem, the existing wake (autobaud) was not able to wake the module and then lock the RN2483/Atmel communication. The fix I've made with begin() solve this issue.
I already thought about bitbanging but touching/changing I/O port that is configured before by hardware UART stack (by 1st begin) is not really clean, we don't know how all of this will react afterward. Except calling a new begin() afterward to ensure all is correctly initialized so we're back in startup point.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Now I understand what you are trying to do. I did the same in this repos: https://github.com/ncthompson/ThingsWeather/blob/master/firmware/things_uno/things_uno.ino
We however saw that after a few weeks the RN2483 would not wake up as expected, so we reverted back to using the ATmega's WDT.

Is it really impossible to handle the serial interrupt and re-bauding of the RN2483 from the user space, without changing the library? How about just manually doing an autobaud, including the begin() and baud changes, but in the user's program?

modemStream->flush();
delay(20);
// set baudrate back to normal and send autobaud
#ifdef HARDWARE_UART
modemStream->begin(57600);
#endif
modemStream->write((uint8_t)0x55);
modemStream->flush();
modemStream->write(SEND_MSG);
if (checkModuleAvailable() > 0) {
baudDetermined = true;
}
}
}

void TheThingsNetwork::reset(bool adr)
Expand Down Expand Up @@ -612,18 +650,16 @@ void TheThingsNetwork::showStatus()
debugPrintIndex(SHOW_RX_DELAY_2, buffer);
}

void TheThingsNetwork::configureEU868()
// Puting this common function saves 238 bytes of flash
void TheThingsNetwork::configureChannelsFreq(uint32_t freq, uint8_t first, uint8_t last, uint8_t first_dr)
{
sendMacSet(MAC_RX2, "3 869525000");
sendChSet(MAC_CHANNEL_DRRANGE, 1, "0 6");

char buf[10];
uint32_t freq = 867100000;
uint8_t ch;
for (ch = 0; ch < 8; ch++)
char buf[10];
Copy link
Member

Choose a reason for hiding this comment

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

Although I know that sprintf() doesn't allocate memory, all these small buffers on the stack do allocate extra memory. So yeah the code is smaller, and we'll leave it this way, but do note that the end result is that this code requires more (dynamic) memory.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I really don't like using sprintf() on embedded devices, unless it is really necessary. I also prefer to use statically allocated memory, rather than many dynamically allocated variables. Using statically allocated ones you can know at compile time that you will run out of memory. Doing it dynamically will just cause strange behaviour down the line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Guys,
just a a reminder, sprintf stuff has nothing to do with my PR and was present is the code before I made any changes. It was present and used, so I used it also for the same things it was already used and thought it was consistent even if I agree about dynamically allocated variables.
Charles

Copy link

Choose a reason for hiding this comment

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

Sprintf wasnt implemented in this PR, so that would be stuff for an issue @jpmeijers

Copy link
Member

Choose a reason for hiding this comment

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

@hallard @samuel-puschacher you are changing existing code and replacing that by using sprintf(). That has nothing to do with this PR.


for (ch = first; ch <= last; ch++)
{
sendChSet(MAC_CHANNEL_DCYCLE, ch, "799");
if (ch > 2)
if (ch > first_dr)
{
sprintf(buf, "%lu", freq);
sendChSet(MAC_CHANNEL_FREQ, ch, buf);
Expand All @@ -632,6 +668,13 @@ void TheThingsNetwork::configureEU868()
freq = freq + 200000;
}
}
}

void TheThingsNetwork::configureEU868()
{
sendMacSet(MAC_RX2, "3 869525000");
sendChSet(MAC_CHANNEL_DRRANGE, 1, "0 6");
configureChannelsFreq(867100000, 0, 8, 2);
sendMacSet(MAC_PWRIDX, TTN_PWRIDX_EU868);
}

Expand Down Expand Up @@ -665,24 +708,9 @@ void TheThingsNetwork::configureAS920_923()
* CH0 = 923.2MHz
* CH1 = 923.4MHz
*/
sendMacSet(MAC_ADR, "off"); // TODO: remove when ADR is implemented for this plan
sendMacSet(MAC_RX2, "2 923200000");

char buf[10];
uint32_t freq = 922000000;
uint8_t ch;
for (ch = 0; ch < 8; ch++)
{
sendChSet(MAC_CHANNEL_DCYCLE, ch, "799");
if (ch > 1)
{
sprintf(buf, "%lu", freq);
sendChSet(MAC_CHANNEL_FREQ, ch, buf);
sendChSet(MAC_CHANNEL_DRRANGE, ch, "0 5");
sendChSet(MAC_CHANNEL_STATUS, ch, "on");
freq = freq + 200000;
}
}
configureChannelsFreq(922000000, 0, 8, 1);
// TODO: SF7BW250/DR6 channel, not properly supported by RN2903AS yet
//sendChSet(MAC_CHANNEL_DCYCLE, 8, "799");
//sendChSet(MAC_CHANNEL_FREQ, 8, "922100000");
Expand All @@ -701,21 +729,7 @@ void TheThingsNetwork::configureAS923_925()
sendMacSet(MAC_ADR, "off"); // TODO: remove when ADR is implemented for this plan
sendMacSet(MAC_RX2, "2 923200000");

char buf[10];
uint32_t freq = 923600000;
uint8_t ch;
for (ch = 0; ch < 8; ch++)
{
sendChSet(MAC_CHANNEL_DCYCLE, ch, "799");
if (ch > 1)
{
sprintf(buf, "%lu", freq);
sendChSet(MAC_CHANNEL_FREQ, ch, buf);
sendChSet(MAC_CHANNEL_DRRANGE, ch, "0 5");
sendChSet(MAC_CHANNEL_STATUS, ch, "on");
freq = freq + 200000;
}
}
configureChannelsFreq(923600000, 0, 8, 1);
// TODO: SF7BW250/DR6 channel, not properly supported by RN2903AS yet
//sendChSet(MAC_CHANNEL_DCYCLE, 8, "799");
//sendChSet(MAC_CHANNEL_FREQ, 8, "924500000");
Expand All @@ -734,18 +748,7 @@ void TheThingsNetwork::configureKR920_923()
sendChSet(MAC_CHANNEL_STATUS, 0, "off");
sendChSet(MAC_CHANNEL_STATUS, 1, "off");

char buf[10];
uint32_t freq = 922100000;
uint8_t ch;
for (ch = 2; ch < 9; ch++)
{
sendChSet(MAC_CHANNEL_DCYCLE, ch, "799");
sprintf(buf, "%lu", freq);
sendChSet(MAC_CHANNEL_FREQ, ch, buf);
sendChSet(MAC_CHANNEL_DRRANGE, ch, "0 5");
sendChSet(MAC_CHANNEL_STATUS, ch, "on");
freq = freq + 200000;
}
configureChannelsFreq(922100000, 2, 9, 0);
sendMacSet(MAC_PWRIDX, TTN_PWRIDX_KR920_923);
}

Expand Down Expand Up @@ -865,24 +868,13 @@ bool TheThingsNetwork::sendChSet(uint8_t index, uint8_t channel, const char *val
{
clearReadBuffer();
char ch[5];
if (channel > 9)
{
ch[0] = ((channel - (channel % 10)) / 10) + 48;
ch[1] = (channel % 10) + 48;
ch[2] = '\0';
}
else
{
ch[0] = channel + 48;
ch[1] = '\0';
}
sprintf(ch, "%d ", channel);
debugPrint(F(SENDING));
sendCommand(MAC_TABLE, MAC_PREFIX, true);
sendCommand(MAC_TABLE, MAC_SET, true);
sendCommand(MAC_GET_SET_TABLE, MAC_CH, true);
sendCommand(MAC_CH_TABLE, index, true);
modemStream->write(ch);
modemStream->write(" ");
modemStream->write(value);
modemStream->write(SEND_MSG);
debugPrint(channel);
Expand Down Expand Up @@ -910,44 +902,16 @@ bool TheThingsNetwork::sendPayload(uint8_t mode, uint8_t port, uint8_t *payload,
sendCommand(MAC_TABLE, MAC_PREFIX, true);
sendCommand(MAC_TABLE, MAC_TX, true);
sendCommand(MAC_TX_TABLE, mode, true);
char sport[4];
if (port > 99)
{
sport[0] = ((port - (port % 100)) / 100) + 48;
sport[1] = (((port % 100) - (port % 10)) / 10) + 48;
sport[2] = (port % 10) + 48;
sport[3] = '\0';
}
else if (port > 9)
{
sport[0] = ((port - (port % 10)) / 10) + 48;
sport[1] = (port % 10) + 48;
sport[2] = '\0';
}
else
{
sport[0] = port + 48;
sport[1] = '\0';
}
modemStream->write(sport);
modemStream->print(" ");
debugPrint(sport);
debugPrint(F(" "));
char buf[5];
sprintf(buf, "%d ", port);
modemStream->write(buf);
debugPrint(buf);
uint8_t i = 0;
for (i = 0; i < length; i++)
{
if (payload[i] < 16)
{
modemStream->print("0");
modemStream->print(payload[i], HEX);
debugPrint(F("0"));
debugPrint(payload[i], HEX);
}
else
{
modemStream->print(payload[i], HEX);
debugPrint(payload[i], HEX);
}
sprintf(buf, "%02X", payload[i] );
modemStream->print(buf);
debugPrint(buf);
}
modemStream->write(SEND_MSG);
debugPrintLn();
Expand All @@ -969,11 +933,9 @@ void TheThingsNetwork::sleep(uint32_t mseconds)
modemStream->write(buffer);
modemStream->write(SEND_MSG);
debugPrintLn(buffer);
}

void TheThingsNetwork::wake()
{
autoBaud();
// to be determined back on wake up
baudDetermined = false;
}

void TheThingsNetwork::linkCheck(uint16_t seconds)
Expand Down
16 changes: 14 additions & 2 deletions src/TheThingsNetwork.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,15 @@

#define TTN_BUFFER_SIZE 300

// The Things Products devices
// Things Node only need this, we won't impact Things Uno user
#if defined(ARDUINO_THINGS_NODE)
typedef HardwareSerial SerialType;
#define HARDWARE_UART
#else
typedef Stream SerialType;
#endif

typedef uint8_t port_t;

enum ttn_response_t
Expand All @@ -42,7 +51,7 @@ enum ttn_fp_t
class TheThingsNetwork
{
private:
Stream *modemStream;
SerialType *modemStream;
Stream *debugStream;
ttn_fp_t fp;
uint8_t sf;
Expand All @@ -60,7 +69,9 @@ class TheThingsNetwork
void debugPrintIndex(uint8_t index, const char *value = NULL);
void debugPrintMessage(uint8_t type, uint8_t index, const char *value = NULL);

size_t checkModuleAvailable();
void autoBaud();
void configureChannelsFreq(uint32_t freq, uint8_t first, uint8_t last, uint8_t first_dr);
void configureEU868();
void configureUS915(uint8_t fsb);
void configureAS920_923();
Expand All @@ -78,7 +89,7 @@ class TheThingsNetwork
void sendGetValue(uint8_t table, uint8_t prefix, uint8_t index);

public:
TheThingsNetwork(Stream &modemStream, Stream &debugStream, ttn_fp_t fp, uint8_t sf = TTN_DEFAULT_SF, uint8_t fsb = TTN_DEFAULT_FSB);
TheThingsNetwork(SerialType &modemStream, Stream &debugStream, ttn_fp_t fp, uint8_t sf = TTN_DEFAULT_SF, uint8_t fsb = TTN_DEFAULT_FSB);
void reset(bool adr = true);
void showStatus();
size_t getHardwareEui(char *buffer, size_t size);
Expand All @@ -93,6 +104,7 @@ class TheThingsNetwork
ttn_response_t sendBytes(const uint8_t *payload, size_t length, port_t port = 1, bool confirm = false, uint8_t sf = 0);
ttn_response_t poll(port_t port = 1, bool confirm = false);
void sleep(uint32_t mseconds);
bool isSleeping();
void wake();
void saveState();
void linkCheck(uint16_t seconds);
Expand Down