adafruit / TinyLoRa

LoRaWAN Library
68 stars 37 forks source link

modify examples to avoid corrupting fixed message #11

Closed jerryneedell closed 5 years ago

jerryneedell commented 5 years ago

In the examples, a fixed string is created to send the message "Hello LoRa" but when the message to be transmitted is processed, the contents of the data array passed to senData are destroyed during the encryption process.

If the message is only initialized at declaration then it is sent properly the first time, then garbled every time after that as the array is repeatedly encrypted.

It should be made clear that sendData destroys the data array. The example can be easily fixed with a few simple changes:

replace the declaration

unsigned char loraData[11] = {"hello LoRa"};

with

char loraData[11];

in the loop replace

lora.sendData(loraData, sizeof(loraData), lora.frameCounter);

with

strcpy(loraData,"Hello Lora");
lora.sendData((unsigned char *)loraData, sizeof(loraData), lora.frameCounter);

There are many ways to deal with this. We could have sendData make a local copy of the message, but that is a potential waste of up to 252 bytes.

In general, this will not be in issue if users are sending changing data with every transmission so I really don't think anything needs to be "fixed" other than the way the example feeds the message to sendData.

another option is to modify the example to send values instead of text. For example I have one sending the frame counter as the message: I declare: unsigned char loraData[2] = {0,0};

then when ready to send:

  loraData[0]=lora.frameCounter & 0xff;
  loraData[1]=(lora.frameCounter>>8) & 0xff;

The message can be created in a variety of ways - the argument passed to sendData must be cast to an (unsigned char *) when it is sent if it is not to begin with.

brentru commented 5 years ago

There are many ways to deal with this. We could have sendData make a local copy of the message, but that is a potential waste of up to 252 bytes.

Thank you for reminding me about this, I remember we spoke about this issue a few weeks back.

I opened a new branch for this issue (https://github.com/adafruit/TinyLoRa/tree/fix-encrypt) which copies over the packet to in lora.sendData(data) to a new character array which is fed as an argument to the Encrypt_Payload function and doesn't modify the original Data value sent through sendData().

Tested on my Feather 32u4 and TTN and received the same payload back every frame, instead of garbled like before.

jerryneedell commented 5 years ago

In the "fix-encrypt" branch the tmpData can only handle a fixed size 10 byte message. This needs to handle any message length up to the maximum of 252 byte (I think). I'm still not convinced that sendData needs to be changed to make a copy- I think it better to have the user take responsibility for creating a fresh packet for every transmission. I don't think this is a real burden since most uses would likely involve changing data from sensors and would be different for every packet anyway.

brentru commented 5 years ago

A patch for this issue has been merged into master. Closing...