TheThingsNetwork / arduino-device-lib

Arduino Library for TTN Devices
MIT License
207 stars 96 forks source link

Protocol Buffer(nanopb) Receive example #117

Closed n2jn closed 7 years ago

n2jn commented 7 years ago

Prototype of a Receive example, added an appData.proto for downlink.

FokkeZB commented 7 years ago

We should discuss the nanobuf story with the 3 of us.

An example I've discussed with @johanstokking can be found at: https://gist.github.com/FokkeZB/a311f9b506cffb55a1e71a32f6552294

Still some details to sort out. For me it's not clear if standard messages have a fix port or not for example.

Let's do a call tomorrow.

johanstokking commented 7 years ago

@FokkeZB that example is good. Standard messages go on port 100 or higher.

We can also introduce this:

TheThingsMessage::sendSensorData(ttn, sensorData);

This would encode and send it on port 100.

FokkeZB commented 7 years ago

@Nicolasdejean please rebase on master to fix Travis:

git rebase master
# for every conflict, fix it and then do: git add <pathOfFixedFile> && git rebase --continue
git push --force
FokkeZB commented 7 years ago

We can also introduce this:

TheThingsMessage::sendSensorData(ttn, sensorData);

This would encode and send it on port 100.

I would rather keep this separated.

Could you send sensorData over any port or does it need to be 100 for the handler to know how to decode it?

johanstokking commented 7 years ago

@FokkeZB 100 or higher

FokkeZB commented 7 years ago

Agree that we should first design the API, then code it. Also, should we consider Capt'n proto, which seems much lighter? It's designed by the architect behind protobuf v2.

https://capnproto.org/

johanstokking commented 7 years ago

Definitely good to consider, but the imo the packet size of Protobufs outweighs the faster computation time of Cap 'n Proto by far. Cap 'n Proto has fixed width integers and uses blanks for optional fields, while we need to send as little data as possible. There is support for compression in Cap 'n Proto, but then you're getting really close to what Protobufs already is. And Protobufs has more developer adoption.

The nice thing about the SensorData proto, is that we can put hundreds of (optional) fields in it, and only send the ones that the user uses. With Cap 'n Proto, it would be a massive structure of blanks that need to be compressed.

n2jn commented 7 years ago

Maybe I did it wrong but I think there is also a problem with the size of the sketch for the examples... with the integration of the nanopb: the encoding and decoding fonctions we are at 99% use (~28, 500 bytes), so it doesn't let us much memory for the number of uplink and downlink variables and there is a limited option on how to interpret the downlink messages (AppData variables). Even if we optimize I don't think we can put everything in the examples... How can we solve this ?

Sketch uses 28,530 bytes (99%) of program storage space. Maximum is 28,672 bytes.
Global variables use 1,007 bytes (39%) of dynamic memory, leaving 1,553 bytes for local variables. Maximum is 2,560 bytes.
FokkeZB commented 7 years ago

Mm, I know bananas about Arduino/C/C++ memory efficiency. Maybe @svenhaitjema should take a look?

FokkeZB commented 7 years ago

We should continue this PR after #108 has been merged or merge this PR into #108 so we can work on it as one change.

FokkeZB commented 7 years ago

Please rebase on master now #108 is merged

johanstokking commented 7 years ago

Yep, please rebase and see what we can do about the program storage size. 99% is way too much.

n2jn commented 7 years ago

The 99% is due to the decode and encode functions:

64% is what we have in our normal examples (e.g. normal Receive example). I am not an expert but I think If we find a way to optimize little things inTheThingsNetwork's library we could lower the memory use to 95% max. The real problem is the nanopb library and I think it is already optimized.

n2jn commented 7 years ago

Do we need to change something more in this PR (except for the optimization), or is it ok ?

FokkeZB commented 7 years ago

OK with me!

FokkeZB commented 7 years ago

Time to do a nasty rebase on this one and test and merge it.

n2jn commented 7 years ago

You sure about the rebase ? for me it is up to date when I try one.