pkg/lora-serialization: add support for lora-serialization format by leandrolanzieri · Pull Request #9863 · RIOT-OS/RIOT

leandrolanzieri · 2018-08-30T08:51:50Z

Contribution description

This PR adds support for the lora-serialization format, as a package (found here).

An example showing the lora-serialization format integrated to OpenSenseMap can be found here #9862.

This library is based on this one, but has been implemented withou using dynamic allocation.

This work was done in conjunction with @jia200x.

Testing procedure

cd $(RIOTBASE)/tests/pkg_lora-serialization
make all term

Should output:

main(): This is RIOT! (Version: 2018.10-devel-569-gb199-X1-pr/lora-serialization)
Lora Serialization test

Test 1
Temperature and humidity
---------------------------------
- Writing temperature: 80.12
- Writing humidity: 99.99
- Encoded:  1f 4c 0e 27
- Expected: 1f 4c 0e 27
---------------------------------
SUCCESS

Test 2
Coordinates and unix time
---------------------------------
- Writing coordinates: -33.905052, 151.26641
- Writing unix time: 1467632413
- Encoded:  65 a6 fa fd 6a 24 04 09 1d 4b 7a 57
- Expected: 65 a6 fa fd 6a 24 04 09 1d 4b 7a 57
---------------------------------
SUCCESS

miri64 · 2018-09-03T11:53:58Z

tests/unittests/tests-lora_serialization/Makefile.include

@@ -0,0 +1 @@
+USEPKG += lora-serialization


I know there are already other pkgs in this test application, but embunit can be used outside of the unittests, so can you maybe move them to a separate application?

Moved it to a separate application.

aabadie · 2018-09-04T14:28:55Z

@leandrolanzieri, I don't think unit testing the lora-serialization package within RIOT is required. This is because the library is already unit tested upstream (I saw a tests directory there).
So just adding a simple test application would be enough, like for many other packages.

miri64 · 2018-09-04T15:15:22Z

@leandrolanzieri, I don't think unit testing the lora-serialization package within RIOT is required. This is because the library is already unit tested upstream (I saw a tests directory there).
So just adding a simple test application would be enough, like for many other packages.

I think I tend to agree. There should at least be a test application that includes the pkg and wiggles around some bytes (with cayenne-lpp we encountered some platform-dependent issues e.g. due to the way floats are handled on different platforms).

leandrolanzieri · 2018-09-05T10:10:28Z

I modified the test application, considering the float test cases. Also, a 'real world' example using TTN and openSenseMap can be found here #9862.

leandrolanzieri · 2018-09-07T09:49:49Z

tests/pkg_lora-serialization/tests-lora_serialization.c

+#include "embUnit.h"
+#include "lora_serialization.h"
+
+#if defined(BOARD_NATIVE) && !defined(__clang__)


with cayenne-lpp we encountered some platform-dependent issues e.g. due to the way floats are handled on different platforms

@miri64 you mean this? I encountered problems with floats while running the test as well.

leandrolanzieri · 2018-09-17T08:32:56Z

@miri64 Is the test application ok now?

miri64 · 2018-09-17T13:29:22Z

Yes. Let me run it real quick ;-)

miri64 · 2018-09-17T13:37:10Z

I get an error when compiling with TOOLCHAIN=llvm:

make: Entering directory '/home/mlenders/Repositories/RIOT-OS/RIOT/tests/pkg_lora-serialization'
make[1]: Nothing to be done for 'Makefile.include'.
make: Nothing to be done for 'clean'.
Building application "tests_pkg_lora-serialization" for "native" with MCU "native".

make[1]: Nothing to be done for 'prepare'.
"make" -C /home/mlenders/Repositories/RIOT-OS/RIOT/pkg/lora-serialization
"make" -C /home/mlenders/Repositories/RIOT-OS/RIOT/tests/pkg_lora-serialization/bin/pkg/native/lora-serialization -f /home/mlenders/Repositories/RIOT-OS/RIOT/pkg/lora-serialization/Makefile.lora-serialization
/home/mlenders/Repositories/RIOT-OS/RIOT/tests/pkg_lora-serialization/tests-lora_serialization.c:128:2: error: no newline at end of file [-Werror,-Wnewline-eof]
}
 ^
1 error generated.
/home/mlenders/Repositories/RIOT-OS/RIOT/Makefile.base:83: recipe for target '/home/mlenders/Repositories/RIOT-OS/RIOT/tests/pkg_lora-serialization/bin/native/application_tests_pkg_lora-serialization/tests-lora_serialization.o' failed
make[1]: *** [/home/mlenders/Repositories/RIOT-OS/RIOT/tests/pkg_lora-serialization/bin/native/application_tests_pkg_lora-serialization/tests-lora_serialization.o] Error 1
/home/mlenders/Repositories/RIOT-OS/RIOT/tests/pkg_lora-serialization/../../Makefile.include:434: recipe for target '/home/mlenders/Repositories/RIOT-OS/RIOT/tests/pkg_lora-serialization/bin/native/application_tests_pkg_lora-serialization.a' failed
make: *** [/home/mlenders/Repositories/RIOT-OS/RIOT/tests/pkg_lora-serialization/bin/native/application_tests_pkg_lora-serialization.a] Error 2
make: Leaving directory '/home/mlenders/Repositories/RIOT-OS/RIOT/tests/pkg_lora-serialization'

I think this should be fixed upstream

leandrolanzieri · 2018-09-17T14:01:24Z

Nice one! I was able to reproduce that. I modified the file upstream. It should be working now.

miri64 · 2018-09-17T14:11:39Z

I guess the last commit has the wrong commit message then ;-)

miri64 · 2018-09-17T14:15:48Z

Works now. Tests (which is the only thing that matters in this PR) are fine with me. @aabadie any word from your side?

aabadie

@aabadie any word from your side?

I wanted to ACK, but found one remaining thing. Apart from that it's ok.

aabadie · 2018-09-17T14:21:49Z

tests/pkg_lora-serialization/tests-lora_serialization.c

+
+#include <string.h>
+#include <stdio.h>
+#include "embUnit.h"


embUnit is not used by the test application, so this include can be removed.

Removed the include.

aabadie · 2018-09-17T14:22:15Z

tests/pkg_lora-serialization/Makefile

+include ../Makefile.tests_common
+
+USEPKG += lora-serialization
+USEMODULE += embunit


This module is not needed by the test application

Removed the module.

miri64 · 2018-09-17T14:39:38Z

(please remember to squash before merging ;-))

leandrolanzieri · 2018-09-18T11:34:29Z

(please remember to squash before merging ;-))

@miri64 Do I squash everything to a single commit or keep the one on the tests?

leandrolanzieri · 2018-09-18T16:39:38Z

Squashed the commits to a single one.

danpetry · 2018-09-25T15:34:02Z

Hi @miri64 @aabadie I'm using this PR and #9862, which depends on it.
If you're both ok with the contribution, would it be ok to go ahead and ACK?

miri64

One last comment, apart from that I tested it on samr21-xpro and native, so ACK.

miri64 · 2018-09-25T15:46:26Z

tests/pkg_lora-serialization/tests-lora_serialization.c

@@ -0,0 +1,127 @@
+/*


I'm not sure how nitpicky this is, but usually our c-files containing the main functions are called main.c

Same here, I would prefer a main.c.

aabadie

Found minor remaining things. You can squash and amend directly.

aabadie · 2018-09-25T20:33:52Z

tests/pkg_lora-serialization/Makefile.include

@@ -0,0 +1 @@
+USEPKG += lora-serialization


This file seems useless, is it a leftover from the previous unittest ?

aabadie · 2018-09-25T20:34:09Z

tests/pkg_lora-serialization/tests-lora_serialization.c

@@ -0,0 +1,127 @@
+/*


Same here, I would prefer a main.c.

aabadie · 2018-09-25T20:34:23Z

tests/pkg_lora-serialization/tests-lora_serialization.c

+ */
+
+/**
+ * @ingroup     unittests


Just use tests

tests/pkg_lora-serialization: Add test application

leandrolanzieri · 2018-09-26T07:42:31Z

Applied requested changes and squashed.

aabadie

I've run the test application on native and b-l072z-lrwan1 board., using the test target. It works as expected. Code changes are good now.

Thanks @leandrolanzieri !

ACK

leandrolanzieri mentioned this pull request Aug 30, 2018

examples/opensensemap: Add example of TTN and openSenseMap integration #9862

Closed

miri64 reviewed Sep 3, 2018

View reviewed changes

miri64 mentioned this pull request Sep 3, 2018

lora_serialization: add support for lora-serialization format #9837

Closed

aabadie added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: pkg Area: External package ports Area: LoRa Area: LoRa radio support labels Sep 4, 2018

leandrolanzieri force-pushed the pr/lora-serialization branch 2 times, most recently from fadbb2c to fd81c33 Compare September 5, 2018 10:07

leandrolanzieri commented Sep 7, 2018

View reviewed changes

leandrolanzieri force-pushed the pr/lora-serialization branch from fd81c33 to 738667b Compare September 17, 2018 08:55

leandrolanzieri force-pushed the pr/lora-serialization branch from 738667b to 4730e1d Compare September 17, 2018 13:59

miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 17, 2018

aabadie requested changes Sep 17, 2018

View reviewed changes

leandrolanzieri force-pushed the pr/lora-serialization branch from 353085b to b199036 Compare September 18, 2018 16:16

miri64 approved these changes Sep 25, 2018

View reviewed changes

aabadie requested changes Sep 25, 2018

View reviewed changes

pkg/lora-serialization: Add lora serialization external package

f2bda2b

tests/pkg_lora-serialization: Add test application

leandrolanzieri force-pushed the pr/lora-serialization branch from b199036 to f2bda2b Compare September 26, 2018 07:11

aabadie approved these changes Sep 26, 2018

View reviewed changes

aabadie merged commit ad7ec70 into RIOT-OS:master Sep 26, 2018

leandrolanzieri deleted the pr/lora-serialization branch September 26, 2018 08:03

jia200x added this to the Release 2018.10 milestone Oct 4, 2018

miri64 mentioned this pull request Oct 5, 2018

doc: add serialization group to Doxygen #9840

Merged

Conversation

leandrolanzieri commented Aug 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie commented Sep 4, 2018

Uh oh!

miri64 commented Sep 4, 2018

Uh oh!

leandrolanzieri commented Sep 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri commented Sep 17, 2018

Uh oh!

miri64 commented Sep 17, 2018

Uh oh!

miri64 commented Sep 17, 2018

Uh oh!

leandrolanzieri commented Sep 17, 2018

Uh oh!

miri64 commented Sep 17, 2018

Uh oh!

miri64 commented Sep 17, 2018

Uh oh!

aabadie left a comment • 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!

miri64 commented Sep 17, 2018

Uh oh!

leandrolanzieri commented Sep 18, 2018

Uh oh!

leandrolanzieri commented Sep 18, 2018

Uh oh!

danpetry commented Sep 25, 2018

Uh oh!

miri64 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!

aabadie 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!

leandrolanzieri commented Sep 26, 2018

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

leandrolanzieri commented Aug 30, 2018 •

edited

Loading

aabadie left a comment •

edited

Loading