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

hallard · 2018-03-01T21:21:39Z

I increased the break time condition setting up serial port to lower baud rate when writing 0 (break) on wake method
Bug could be reproduced by non stop shaking the node, with this code, seem fixed

May be we could also arrange autoBaud to reflect this change and do only one method.

johanstokking · 2018-03-04T12:53:06Z

src/TheThingsNetwork.cpp

+    length = checkComm();
+
+    // We succeded talking to the module ?
+    baudDetermined = length ? true : false;


length > 0 ?

johanstokking · 2018-03-04T12:53:18Z

src/TheThingsNetwork.cpp

+    // Send sys get ver check we have an answer
+    sendCommand(SYS_TABLE, 0, true, false);
+    sendCommand(SYS_TABLE, SYS_GET, true, false);
+    sendCommand(SYS_TABLE, SYS_GET_VER, false, false);


Combine in one command

johanstokking · 2018-03-04T12:54:36Z

src/TheThingsNetwork.cpp

+
+void TheThingsNetwork::wake()
+{  
+  HardwareSerial *pLora = (HardwareSerial*) modemStream;


We can't do this; it's not guaranteed that modemStream is a HardwareSerial. In fact, we use quite some alternatives

The library is called TTN Arduino device lib, and TTN devices (The Things Node and The Things Uno) are for now connected and using hardware serial of the 32U4.
I think focus should be fixing Low Power issues (and this fix works) until we have another device using another method than hardware Serial. In this case we could resolve with some #define and boards.txt definition)
Anyway I'm opened to any other alternatives. Just let me know

This library is intended to use with any Microchip RN2xx3 module, whether or not on The Things Uno or The Things Node, for example SODAQ boards or Arduinos with the RN module on a shield. Therefore, we cannot introduce a dependency on specific serials, let alone a cast like this, unfortunately.

Ok, I've implemented the same HAL than in Sodaq_RN2483 library so it use

#if defined(ARDUINO_ARCH_AVR) typedef HardwareSerial SerialType; #elif defined(ARDUINO_ARCH_SAM) || defined(ARDUINO_ARCH_SAMD) typedef Uart SerialType; #else typedef Stream SerialType; #endif

and of course changing Stream to SerialType in the library, should do the trick.

johanstokking · 2018-03-04T12:55:57Z

src/TheThingsNetwork.h

@@ -60,6 +61,7 @@ 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 checkComm();


checkModuleAvailable()

johanstokking · 2018-03-04T15:44:08Z

src/TheThingsNetwork.cpp


    // We succeded talking to the module ?
-    baudDetermined = length ? true : false;
+    baudDetermined = length > 0 ? true : false;


You can remove the ?: thing now 😁

johanstokking · 2018-03-04T15:44:21Z

src/TheThingsNetwork.cpp

@@ -440,7 +440,7 @@ void TheThingsNetwork::wake()
    pLora->write((uint8_t)0x55);
    pLora->flush();
    pLora->write(SEND_MSG);
-    if (checkComm()>0) {
+    if (checkModuleAvailable()>0) {


Spacing; ) > 0

johanstokking · 2018-03-05T15:43:34Z

src/TheThingsNetwork.h

+typedef Uart SerialType;
+#else
+typedef Stream SerialType;
+#endif


The code breaks on Stream, because there's no begin(), right?

We can't introduce breaking changes in this repository.

We also have users using AltSoftSerial, deriving from Stream(), introducing begin().

No in this case because Serial Type is depending on target and all defined target have begin()

For AltSoftSerial, it's another thing, I need to think on how to avoid problem with this one.

Where's begin() in Stream defined?

Not here: https://github.com/arduino/Arduino/blob/master/hardware/arduino/avr/cores/arduino/Stream.h, nor in https://github.com/arduino/Arduino/blob/master/hardware/arduino/avr/cores/arduino/Print.h

Sure the original TTN code breaks on begin() because it's a Stream. I'm just saying that this PR does not break because there is a typedef of SerialType depending on target (AVR or SAMD) using this in the include file

#if defined(ARDUINO_ARCH_AVR) typedef HardwareSerial SerialType; #elif defined(ARDUINO_ARCH_SAM) || defined(ARDUINO_ARCH_SAMD) typedef Uart SerialType; #else typedef Stream SerialType; #endif

and new TTN object instance use SerialType

TheThingsNetwork(SerialType &modemStream, Stream &debugStream, ttn_fp_t fp, uint8_t sf = TTN_DEFAULT_SF, uint8_t fsb = TTN_DEFAULT_FSB);

I need to see what we can do for AltSoftSerial, but I do not see any other solution than a define.

johanstokking

We just can't introduce dependencies like this.

I'm afraid this has to be in the sketch itself.

johanstokking · 2018-03-28T10:09:29Z

src/TheThingsNetwork.h

@@ -8,6 +8,9 @@
 #include <Stream.h>
 #include <avr/pgmspace.h>

+// If you need to use soft serial library you need to uncomment this line
+//#define USE_SOFT_SERIAL


We can't expect people to uncomment lines somewhere in libraries. Also, that will be overwritten when updating them.

The problem is that we can't do that in sketch, not that I do not want, it won't just work because when the compiler compile the library it does not know anything about what is done in the sketch, nor defined values with #define

So a decision is on the go, using stream instead of HardwareSerial makes sense to have majority devices compatible (which are not TTN one) without changing a line of code in the library. But on the other hand it impact the LowPower mode for TTN devices that use all Hardware Serial.

From my point of view TTN lib should match TTN devices and make them working as expected (ie no impact on battery life), and I think it’s what TTN backers and devices buyers want but I may be wrong.

Another vision is to make others boards users happy and be able to use soft serial on other devices (not TTN one) without changing any line of code in the library. This will impact severely battery life for users in the case above that wait for a battery life for one year.

But it's not my decision.

Another way could be having dedicated TTN Boards (As sparkfun, Adafruit, ..) in Arduino IDE, for example The Things Node and The Things Uno and select the correct board to build and upload. This way, library know the target board and can do some conditional compilation with HardwareSerial instead of stream for TTN boards.

Ok, we now have a fix, Samuel created TTN boards and we tested together.

Need to install new TTN boards from Arduino IDE Boards URL. In IDE preference add this additional board manager URL
https://raw.githubusercontent.com/samuel-puschacher/TTP_Arduino_Boards/master/package_thethingsproducts_index.json

Once added, install from board manager install TheThingsNetwork boards

Once done Select the correct board as target

If selected The Things Uno or The Things Node, it will use HardwareSerial and LowPowerFix will be active, else, stay as usual Stream.

Hope this is satisfying ?

So I hope, we can bring this further now

This solution is even downwards compatible ....
And the best the docs are already updated, see PR: TheThingsNetwork/docs#226

With in the sketch, I mean in the user sketch, the baud rate thing. But I think a board is a very good suggestion.

johanstokking

Thanks. Better to keep PRs small and concise; i.e. not introduce new changes that are unrelated (like using sprintf())

johanstokking · 2018-03-29T13:22:59Z

src/TheThingsNetwork.cpp

@@ -612,18 +646,16 @@ void TheThingsNetwork::showStatus()
  debugPrintIndex(SHOW_RX_DELAY_2, buffer);
 }

-void TheThingsNetwork::configureEU868()
+// Puting this common fonction save 238 bytes of flash


function saves

johanstokking · 2018-03-29T13:23:49Z

src/TheThingsNetwork.cpp

@@ -668,21 +707,7 @@ void TheThingsNetwork::configureAS920_923()
  sendMacSet(MAC_ADR, "off"); // TODO: remove when ADR is implemented for this plan


Reminds me; ADR is implemented

ok, so removed the line

johanstokking · 2018-03-29T13:24:46Z

src/TheThingsNetwork.cpp

@@ -612,18 +646,16 @@ void TheThingsNetwork::showStatus()
  debugPrintIndex(SHOW_RX_DELAY_2, buffer);
 }

-void TheThingsNetwork::configureEU868()
+// Puting this common fonction save 238 bytes of flash
+void TheThingsNetwork::configureChannelsFreq(uint32_t freq, uint8_t begin, uint8_t end, uint8_t start)


Seems to make more sense to make end inclusive

Unclear what begin vs start means

changed name to first (first channel), last (last channel) and firs_dr (first channel to change dr)

johanstokking · 2018-03-29T13:26:45Z

src/TheThingsNetwork.h

@@ -8,6 +8,9 @@
 #include <Stream.h>
 #include <avr/pgmspace.h>

+// If you need to use soft serial library you need to uncomment this line
+//#define USE_SOFT_SERIAL


With in the sketch, I mean in the user sketch, the baud rate thing. But I think a board is a very good suggestion.

johanstokking · 2018-03-29T13:26:54Z

src/TheThingsNetwork.h

@@ -20,6 +23,19 @@

 #define TTN_BUFFER_SIZE 300

+// The Things Industries devices


The Things Products

johanstokking · 2018-03-29T13:28:56Z

src/TheThingsNetwork.cpp

+    modemStream->flush();
+    delay(20);
+    // set baudrate back to normal and send autobaud
+    modemStream->begin(57600);


Again, Stream does not have begin(), right? If so, where is it defined?

I.e. this code is still breaking when SerialType cannot be determined and falls back to Stream.

Correct tested with a defined symbol when a hardware serial is used
take care, now this will compile for SparkFun Micro but won't work since the begin will not be executed. For this to work users need to select new TTN boards in IDE ;-)

Why would that not work? In those cases, HARDWARE_UART gets defined on https://github.com/TheThingsNetwork/arduino-device-lib/pull/231/files/bab08ab865752ff5208c71f6f9dae807e6be8fba..a164a60fd13751bf79ae74f4b50bb9afcabd29ed#diff-49a17fc31f5e939a36dc3a3e67227c1bR30

johanstokking · 2018-03-29T13:31:27Z

Can you file a PR in https://github.com/TheThingsProducts/arduino for the board file @samuel-puschacher ?

hallard · 2018-03-29T14:10:48Z

@johan,
Why you don't just Fork Samuel board repo since he's the creator and the maintainer?
I let you see with him how to organize this because it's not on any of my repo.

johanstokking · 2018-03-29T16:08:17Z

src/TheThingsNetwork.cpp

+    modemStream->flush();
+    delay(20);
+    // set baudrate back to normal and send autobaud
+    modemStream->begin(57600);


Why would that not work? In those cases, HARDWARE_UART gets defined on https://github.com/TheThingsNetwork/arduino-device-lib/pull/231/files/bab08ab865752ff5208c71f6f9dae807e6be8fba..a164a60fd13751bf79ae74f4b50bb9afcabd29ed#diff-49a17fc31f5e939a36dc3a3e67227c1bR30

johanstokking · 2018-03-29T16:08:37Z

src/TheThingsNetwork.h


 #elif defined(ARDUINO_ARCH_SAM) || defined(ARDUINO_ARCH_SAMD)
 typedef HardwareSerial SerialType;
+#define HARDWARE_UART

 #elif defined(ARDUINO_ARCH_SAM) || defined(ARDUINO_ARCH_SAMD)


Why are you checking exactly the same thing as on L28?

Because it's a mistake, it's fixed

johanstokking · 2018-03-29T16:10:55Z

@hallard @samuel-puschacher I'm fine with forking if the repository is named arduino or arduino-boards

johanstokking · 2018-03-29T16:16:48Z

src/TheThingsNetwork.cpp

+
+bool TheThingsNetwork::isSleeping()
+{  
+  return baudDetermined ? false : true ;


return !baudDetermined;

Also is this a sensible check? Semantically it's not clear to me.

yes it's the same thing, changed it

johanstokking · 2018-03-29T16:17:22Z

src/TheThingsNetwork.cpp

+    // check we can talk
+    length = checkModuleAvailable();
+
+    // We succeded talking to the module ?


succeEded

johanstokking · 2018-03-29T16:18:02Z

src/TheThingsNetwork.cpp

+    length = checkModuleAvailable();
+
+    // We succeded talking to the module ?
+    baudDetermined = length > 0 ;


If a number is non-zero, it will implicitly be true when assigning to a boolean.

yes because it's a test operation so it return true or false, I added some parenthesis

johanstokking · 2018-03-29T16:18:25Z

src/TheThingsNetwork.cpp

+void TheThingsNetwork::wake()
+{  
+  // If sleeping
+  if (!baudDetermined) 


Why not call the isSleeping() method? It's a call, but it's semantically clearer.

johanstokking · 2018-03-29T16:18:34Z

src/TheThingsNetwork.cpp

@@ -612,18 +650,16 @@ void TheThingsNetwork::showStatus()
  debugPrintIndex(SHOW_RX_DELAY_2, buffer);
 }

-void TheThingsNetwork::configureEU868()
+// Puting this common fonction saves 238 bytes of flash


johanstokking · 2018-03-29T16:19:43Z

src/TheThingsNetwork.cpp

  uint8_t ch;
-  for (ch = 0; ch < 8; ch++)
+  char buf[10];


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.

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.

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

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

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

samuel-puschacher · 2018-03-29T16:24:07Z

@johanstokking
Done

jpmeijers

Frankly I am not happy with these changes.

jpmeijers · 2018-03-30T15:04:16Z

src/TheThingsNetwork.h

+#else
+typedef Stream SerialType;
+#endif
+


I don't think we should do this in the library. With this code we are not allowing people to use strange setups like a second RN2483 on a SoftwareSerial port on a Things Uno.

And why are we not allowing SoftwareSerial on SAM and SAMD devices?

We could also disable it on the Uno... but power usage will increase
But I can't see a reason why to put a second Rn2483 on the Things Uno

Yeah, I could remove this for SAM and SAMD, it's no problem.
I can even let hardware serial only for Things Node (and not Things UNO) if this makes everyone happy ;-)

But I think no one would put a second RN2843 on the Uno!
My Opinion: make a special constant, you have to define if you want to use a second RN2843!
No beginner would like to connect a second RN2843, the people which think about such things would also be able to make a special configuration! And for the majority of people, the uno will be power efficient.

jpmeijers · 2018-03-30T15:06:18Z

src/TheThingsNetwork.cpp

+#ifdef HARDWARE_UART
+    modemStream->begin(2400);
+#endif
+    modemStream->write((uint8_t) 0x00);


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()?

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.

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?

jpmeijers · 2018-03-30T15:09:57Z

src/TheThingsNetwork.cpp

  uint8_t ch;
-  for (ch = 0; ch < 8; ch++)
+  char buf[10];


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.

johanstokking · 2018-04-03T13:53:22Z

src/TheThingsNetwork.cpp

  uint8_t ch;
-  for (ch = 0; ch < 8; ch++)
+  char buf[10];


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

johanstokking · 2018-04-03T13:59:16Z

@samuel-puschacher I cannot fork the repository as it is now:

The commit trail is very dirty
The URLs should be on github.com/TheThingsProducts/... We can fork it and change it, but see [1]
GitHub has tags for versioning and releases for distribution, see for example the Arduino library. We can fork it and do our own tags and releases, but then this repository has to be clean, and see also [1]
The product is called The Things Uno (no caps)

[1] The point of forking is to diverge from an existing project and/or contribute changes back to the original repository. Are you planning on maintaining this repository in your account, regardless of what is in TheThingsProducts (i.e. diverging projects)? If not, why not create a PR on the existing repository?

hallard · 2018-04-03T15:53:13Z

@johanstokking
When I first forked the library, the code was already containing some sprintf() in v2.5.1. It was done by this commit
So when I optimized the code size, I won approx 700 bytes of flash, and yes I added sprintf() but it was not new using this call since some where already in place on original code.
I don't see what I've done wrong on this point.
Charles

johanstokking · 2018-04-04T12:12:53Z

Indeed, it did contain sprintf(), but I'd rather get rid of it than introducing more occurrences, especially in unrelated feature branches like these.

jpmeijers · 2018-04-04T14:21:53Z

@hallard @samuel-puschacher and me discussed this PR over the weekend. We think it would be better splitting this PR up into smaller ones. I was planning to do this, but didn't get time for it yet. But the idea is to have the sleep fix in a different PR than the code size optimisation. This also means the sprintf's will be in a different PR, where we can optimise them out in a better way.

When the PR split is done we can close this one.

jpmeijers · 2018-04-04T15:54:55Z

Merging this into a separate branch to do the splitting.

fixed RN2xxx not waked from sleep by autoBaud sometimes

ff4d97c

hallard mentioned this pull request Mar 1, 2018

Wake function - feature request #221

Closed

johanstokking requested changes Mar 4, 2018

View reviewed changes

hallard added 2 commits March 4, 2018 15:49

changes requested by johan

ed31a8a

changes requested by johan

fa0079f

johanstokking requested changes Mar 4, 2018

View reviewed changes

hallard added 3 commits March 4, 2018 17:37

johan request

2193f9e

cosmetic

4be44af

HAL abstration of modemStream

2634eb0

johanstokking requested changes Mar 5, 2018

View reviewed changes

hallard added 2 commits March 6, 2018 11:28

Ability to use AltSoftSerial Library

1b3cdf9

Code optimization, added ~700 bytes flash for sketch

cfb5679

johanstokking requested changes Mar 28, 2018

View reviewed changes

HardwareSerial only used on TheThings Node and Uno

bab08ab

johanstokking requested changes Mar 29, 2018

View reviewed changes

Applied changes requested by johan

a164a60

johanstokking requested changes Mar 29, 2018

View reviewed changes

Fixed dual check

0a8141e

johanstokking requested changes Mar 29, 2018

View reviewed changes

cosmetic

48f3239

jpmeijers reviewed Mar 30, 2018

View reviewed changes

Added Hardware Serial only for the things node

ee3f781

johanstokking requested changes Apr 3, 2018

View reviewed changes

jpmeijers changed the base branch from master to fix/autobaud-powersaving April 4, 2018 15:53

jpmeijers merged commit 433a221 into TheThingsNetwork:fix/autobaud-powersaving Apr 4, 2018

		@@ -668,21 +707,7 @@ void TheThingsNetwork::configureAS920_923()
		sendMacSet(MAC_ADR, "off"); // TODO: remove when ADR is implemented for this plan

		@@ -20,6 +23,19 @@

		#define TTN_BUFFER_SIZE 300

		// The Things Industries devices

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

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

Uh oh!

Conversation

hallard commented Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hallard Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johanstokking left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hallard Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johanstokking left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

hallard commented Mar 1, 2018 •

edited

Loading

hallard Mar 6, 2018 •

edited

Loading

hallard Mar 28, 2018 •

edited

Loading