adafruit / TinyLoRa

LoRaWAN Library
68 stars 37 forks source link

set default backward compatible region #29

Closed nerdyscout closed 4 years ago

nerdyscout commented 4 years ago

I am outside the US. Every time I start a project using this library I have to edit the lib again. It happend often to me, after an update of the library, the project doesnt work anymore because it was reset to default. This little fix allows to define the region in the project instead of in the library, if nothing is defined the US is still default.

nerdyscout commented 4 years ago

with my current PR I would simply use it like:

#include <Arduino.h>
...
#define EU863
#include <TinyLoRa.h>

which is not really nice but should work and wont break current implementations. But I am open to other solutions, if you got one.

clavisound commented 4 years ago

What about to add setRegion function? With a GPS device if the device changes region, we can choose new region.

brentru commented 4 years ago

@clavisound That'd work too.

Though, a call to setRegion would need to occur before the call to begin(), and all the examples will need to be changed to support this change.

I did something similar with setting the region in the CircuitPython version of this library: https://github.com/adafruit/Adafruit_CircuitPython_TinyLoRa/blob/master/adafruit_tinylora/adafruit_tinylora.py#L172

The arduino-lmic library allows a user to set the TTN region through the inclusion of a project_settings/lmic_project_config.h file. Though, I think a function call in the sketch to setRegion(REGION_NAME) would be simpler.

nerdyscout commented 4 years ago

Altough I like the idea to change the region while runtime i think there are limited devices which need such a feature. At the same time it would mean all frequency plans have to be implemented and therefore extra code size. For those reasons I would suggest to implement this optionally, the default behavior should be like it is currently.

brentru commented 4 years ago

@nerdyscout The conditional inclusion for frequency plans would remain as-is, the code size wouldn't change too much except for the addition of a setRegion() function.

nerdyscout commented 4 years ago

@nerdyscout The conditional inclusion for frequency plans would remain as-is, the code size wouldn't change too much except for the addition of a setRegion() function.

I dont think so. Currently the preprocessor should strip away not defined regions. Using setRegion would require 4x char PROGMEM TinyLoRa::LoRa_Frequency[8][3], might be not much space, but on such small microcontrollers sometimes counts every bit.

Anyway... my current pull request is backward compatible and solves the problem for the moment. I would like to integrate this now. Implementing a more advanced version will take time and can be done later.

clavisound commented 4 years ago

@nerdyscout The conditional inclusion for frequency plans would remain as-is, the code size wouldn't change too much except for the addition of a setRegion() function.

I dont think so. Currently the preprocessor should strip away not defined regions. Using setRegion would require 4x char PROGMEM TinyLoRa::LoRa_Frequency[8][3], might be not much space, but on such small microcontrollers sometimes counts every bit.

I had the same worries, but hello_LoRa.ino has only 16 bytes more and same SRAM with my setPower function in my branch.

BIN: 7420 bytes (25%) SRAM: 307 TinyLoRa-1.2.1
BIN: 7436 bytes (25%) SRAM: 307 TinyLoRa-clavisound/setPower branch

16 bytes more is not a problem. And they are used only if setPower function used.

I want TinyLoRa to be tiny, or else would be LMIC, but it seems it's not a problem. You can test and report. I suppose with setRegion the BIN will be 100 bytes more. It's not a problem.

Anyway... my current pull request is backward compatible and solves the problem for the moment. I would like to integrate this now. Implementing a more advanced version will take time and can be done later.

I fully agree, it's not intrusive. I hope brentru agrees.

I have the will to implement the setRegion function in the future. I hoped someone can do the job for me :-)

brentru commented 4 years ago

@clavisound It'd be very appreciated if you end up implementing a setRegion in the future.

@nerdyscout Ok, let's get this ready to merge-in, could you bump the version here (https://github.com/adafruit/TinyLoRa/blob/master/library.properties) to 1.2.2? I'll merge/release once passing since this PR is backwards-compatible with the current release.

brentru commented 4 years ago

@nerdyscout It seems commit #44ce58c reverted #138128, please update the version again

nerdyscout commented 4 years ago

@nerdyscout It seems commit #44ce58c reverted #138128, please update the version again

yes, sorry for that. I will fix it this evening, currently still at work ;)