Protocol Buffer (nanopb) integration

[n2jn]

Integrated nanopb lib, created Makefile for .proto file, remade the example, created a new class TheThingsMessage and new file with new fonctions for the example. #78 #79 #80 #81

[johanstokking]

Also rebase

[FokkeZB]

I think TheThingsMessage should only encode and decode, not have any send methods. How about this API:

#include <TheThingsNetwork.h>
#include <TheThingsMessage.h>

const byte appEui[8] = { 0x70, 0xB3, 0xD5, 0x7E, 0xD0, 0x00, 0x11, 0x86 };
const byte appKey[16] = { 0xEF, 0x4C, 0xAA, 0x85, 0xE1, 0xB2, 0x26, 0x53, 0x98, 0xE2, 0x3C, 0xD1, 0x31, 0x50, 0xCA, 0xDF };

#define loraSerial Serial1
#define debugSerial Serial

TheThingsNetwork ttn(loraSerial, debugSerial, TTN_FP_EU868);

void setup() {
  loraSerial.begin(57600);
  debugSerial.begin(9600);

  ttn.onMessage(message);

  ttn.join(appEui, appKey);
}

void loop() {
  float humidity = 232;
  uint32_t motion = digitalRead(2) == HIGH;
  uint32_t water = 682 - analogRead(A0);
  uint32_t temperature = 345 - analogRead(A0);

  // Where does this `api_Measurement` come from? A standard? I don't like the class name
  api_Measurement measurement = api_Measurement_init_default;

  measurement.motion = motion;
  measurement.water = water;
  measurement.temperature = temperature;
  measurement.humidity = humidity;

  // Constructor to create message from measurement
  TheThingsMessage msg(measurement, all);

  // New TTN (!) method that calls msg.getBytes() and msg.getLength() to use with ttn.sendBytes()
  ttn.sendMessage(msg, 1, true);

  delay(10000);
}

void message(const byte* payload, int length, int port) {

  // Constructor to create message from buffer
  TheThingsMessage msg(payload, length, port);

  api_Measurement measurement = msg.getMeasurement();

  // Do something with the measurement
}

Maybe instead of using two TheThingsMessage constructors, it could also be a setMeasurement(api_Measurement m) and setMeasurement(const byte* payload, int length, int port) so that you can re use the same instance.

[johanstokking]

The reason it's called api_Measurement is because it's a generated file by Nanopb. We might introduce a typedef in TheThingsMessage.h to use a nice alias. Also, we may drop the api package name so it simply becomes Measurement already.

If TheThingsNetwork is going to take TheThingsMessage as parameter type, we have to include the header and all the other Nanopb stuff as well. I wonder if Arduino optimizes this away for users who don't use TheThingsMessage.

Can't we have a static method like size_t TheThingsMessage::encode(Measurement *m, byte *buffer, size_t size) so that the user provides the buffer? Then the user can pass the buffer to TheThingsNetwork or anywhere else. Then, we keep the classes decoupled which makes the optimizer happy.

[FokkeZB]

If the user provides the buffer he needs to know what size to make it right?

Between these two:

byte payload[10]; // how do I know what size?
payload = msg.encode(m, payload, 10);
ttn.sendBytes(payload, 10);

msg.setMeasurement(m);
ttn.sendBytes(msg.getPayload(), msg.getLength);

I find the second much more predictable. Msg takes care of encoding/decoding measurements to/from buffer, the rest is just as current.

[FokkeZB]

On the api_Measurement, I'd definitely drop api_, but would it also make sense to chose a name less prone for type errors by non-natives? Like Data?

[johanstokking]

The size is variable, you don't know it in advance because it will be as small as possible, that's one of the beauties of protobufs. We only know it after encoding.

What about this:

SensorData data = TheThingsMessage::newSensorData();
data.has_temperature_celcius = true;
data.temperature_celcius = analogRead(A0) * ... - ...;

byte *buffer;
size_t size;
TheThingsMessage::encodeSensorData(&data, &buffer, &size);
ttn.sendBytes(buffer, size);

That encode is declared as follows:

static void encodeSensorData(SensorData *data, byte **buffer, size_t *size);

NB 1; I wonder why we need an instance of TheThingsMessage; we can simply use methods defined in the header file or static methods of the class.

NB 2; Regarding naming; there's data back and forth so it's good to distinguish between uplink and downlink. We can make different protos any time. Maybe SensorData is more clear as it's short and the scope is then sensor readings.

NB 3; When it comes to embedded programming, there's not a lot of room to make it really, really clean.

[FokkeZB]

I like that API, but find it confusing that TheThingsMessage isn't really a message anymore, it's more like a encoder/decoder from SensorData to buffer. What I liked about using an instance is that it could even hide the SensorData object from the user:

msg TheThingsMessage;
msg.data.has_temperature_celcius = true; // why do I need this and can't just set the value?
msg.data.temperature_celcius = analogRead(A0) * ... - ...;

ttn.sendBytes(msg.getBuffer(), msg.getLength());

msg.parseBuffer(buffer, length);
debugSerial.print("Temperature: ");
debugSerial.println(msg.data.temperature_celcius);

[johanstokking]

I think it's good to reserve the possibility to introduce other protos after SensorData, so I wouldn't tie TheThingsMessage to SensorData, but the name becomes confusing indeed. What about just TheThingsMessages as static class?

Regarding the has_ prefix: this is how protobufs work, you have to indicate that you have this optional field. If you don't specify it, it won't be send and it won't cost payload. This is what @Nicolasdejean's choseMessage() came from, but I don't really like it.

[FokkeZB]

I've tested the Send example in the PR which now has static values. It successfully encodes and sends 08011D0000F041250000AC422D0000C242. How do I now verify this decoded on the backend @johanstokking?

[n2jn]

@FokkeZB To verify the encoded message I sent the encoded value from the dashboard and received it on the sketch.

[FokkeZB]

Smart :) @johanstokking do you agree we should merge #117 into this branch so that we can test it together?