pkg: add cn-cbor CBOR implementation plus unit tests #8467

lorenz9314 · 2018-01-27T10:31:33Z

Contribution description

Adds the CBOR implementation cn-cbor to RIOT including unit tests. The test cases have been partially taken from cn-cbors original unit tests.

Issues/PRs references

The package itself has been taken from #8025, commits 772e877, fd7b6ab and ae2c180

kaspar030

I'm wondering, for the unittests you used "cncbor", while the package is called "cn-cbor" (with dash). IMO we should stick with one version of the naming. The former is shorter, but cabo himself put the dash in there...

kaspar030 · 2018-02-06T09:43:00Z

tests/unittests/tests-cncbor/tests-cncbor.c

+  * @file
+  * @brief      Unit tests for pkg cn-cbor.
+  *
+  * @author     Lorenz Hüther


if possible, please add email addresses

Fine with me, I've got to ask Mathias first though.

kaspar030 · 2018-02-06T09:43:56Z

tests/unittests/tests-cncbor/tests-cncbor.c

+} buffer_t;
+
+typedef struct cbor_failure
+{


opening brace goes one line up, like done for buffr_t.

Will be fixed!

kaspar030 · 2018-02-06T09:44:34Z

tests/unittests/tests-cncbor/tests-cncbor.c

+    cn_cbor_error err;
+} cbor_failure;
+
+cn_cbor *cbor;


if these variables are only used in this file, please make them static.

kaspar030 · 2018-02-06T09:46:02Z

tests/pkg_cn-cbor/main.c

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


with the unittests in place, I think the test is not needed anymore...

Alright, noted!

still here?

Yep, seems so. let me check...

kaspar030 · 2018-02-06T09:49:08Z

@lorenz9314 Thanks for taking over!

This is almost good to go, only minor issues.

kaspar030

Please prepend the unittests commit with "tests/unittests: "

kaspar030 · 2018-02-06T09:50:46Z

tests/unittests/tests-cncbor/tests-cncbor.c

+            &inv));
+
+    for (offs = 0; offs < sizeof(tests) / sizeof(cbor_failure); offs++) {
+    TEST_ASSERT(parse_hex(tests[offs].hex));


indent is off

Will be corrected...

lorenz9314 · 2018-02-06T17:20:54Z

@kaspar030
Thanks for the feedback! Regarding the missing dash in cncbor I completely forgot I did that. The reason was that the build would mysteriously fail if a second dash was present. Simply removing it was a quick fix as I had different priorities at the time. I must admit I have not looked into why that would be in great detail after that.

emmanuelsearch · 2018-03-02T15:35:40Z

@lorenz9314 what's the status on this? Looks like we're close to green?

lorenz9314 · 2018-03-06T16:47:10Z

@emmanuelsearch Pretty much ready on my side as well, however I am still unable to rename the tests to tests-cn-cbor as this causes sthe build to fail during linking. I pushed the changes, but like I said, no luck with the name.

emmanuelsearch · 2018-03-06T17:14:21Z

@kaspar030 @bergzand any clues how to fix this issue?

kaspar030 · 2018-03-06T17:14:20Z

tests/pkg_cn-cbor/Makefile

@@ -0,0 +1,10 @@
+APPLICATION = pkg_cn-cbor


please remove this line, it is redundant

kaspar030 · 2018-03-06T17:14:51Z

tests/pkg_cn-cbor/main.c

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


still here?

jnohlgard · 2018-03-06T18:01:33Z

You can not have a dash in the unit test name because of c function naming rules. Name the directory tests-cn_cbor and it should work.

lorenz9314 · 2018-03-07T15:56:59Z

@gebart Alright, done. Thanks!

emmanuelsearch · 2018-03-09T10:44:56Z

@lorenz9314 seems like squashing would make sense at this point?

lorenz9314 · 2018-03-16T12:28:22Z

@emmanuelsearch Done...

kaspar030

Minor things left. Feel free to directly squash.

kaspar030 · 2018-03-16T12:40:49Z

pkg/cn-cbor/Makefile

+.PHONY: all
+
+all: git-download
+	@cp Makefile.cn-cbor $(PKG_BUILDDIR)/src/Makefile


please drop this line and directly use this Makefile in the next, like:

"$(MAKE)" -C $(PKG_BUILDDIR)/src -f $(CURDIR)/Makefile.cn-cbor

kaspar030 · 2018-03-16T12:42:36Z

tests/unittests/tests-cn_cbor/tests-cn_cbor.c

+    unsigned char *pntr;
+} buffer_t;
+
+typedef struct cbor_failure {


I think you can remove the cbor_failure after "struct".

kaspar030

ACK! Thanks for your patience. ;)

lorenz9314 · 2018-03-16T13:27:39Z

@kaspar030 Likewise!

lorenz9314 closed this Jan 27, 2018

lorenz9314 reopened this Jan 27, 2018

smlng added Area: tests Area: tests and testing framework Area: pkg Area: External package ports State: waiting for other PR State: The PR requires another PR to be merged first labels Feb 6, 2018

smlng requested a review from kaspar030 February 6, 2018 08:30

kaspar030 mentioned this pull request Feb 6, 2018

pkg: add cn-cbor CBOR implementation #8025

Closed

kaspar030 changed the title ~~add cn-cbor CBOR implementation plus unit tests~~ pkg: add cn-cbor CBOR implementation plus unit tests Feb 6, 2018

kaspar030 requested changes Feb 6, 2018

View reviewed changes

vincent-d removed the State: waiting for other PR State: The PR requires another PR to be merged first label Feb 27, 2018

lorenz9314 force-pushed the pr_cn_cbor_unit_tests branch from 44e8ad0 to f5d8116 Compare March 6, 2018 16:46

kaspar030 requested changes Mar 6, 2018

View reviewed changes

kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 6, 2018

lorenz9314 force-pushed the pr_cn_cbor_unit_tests branch from 6b0bc90 to 927311e Compare March 16, 2018 08:24

pkg: add cn-cbor CBOR implementation

70f0d84

lorenz9314 force-pushed the pr_cn_cbor_unit_tests branch from 927311e to 7957ef5 Compare March 16, 2018 12:00

kaspar030 requested changes Mar 16, 2018

View reviewed changes

tests/unittests: Added unit tests for pkg cn-cbor, removed old tests.

7117663

lorenz9314 force-pushed the pr_cn_cbor_unit_tests branch from 7957ef5 to 7117663 Compare March 16, 2018 12:54

kaspar030 approved these changes Mar 16, 2018

View reviewed changes

emmanuelsearch merged commit a367e37 into RIOT-OS:master Mar 16, 2018

kaspar030 mentioned this pull request Mar 16, 2018

pkg: cncbor: misc fixes #8791

Merged

nmeum mentioned this pull request Mar 16, 2018

sys/cbor: remove module #8793

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg: add cn-cbor CBOR implementation plus unit tests #8467

pkg: add cn-cbor CBOR implementation plus unit tests #8467

lorenz9314 commented Jan 27, 2018

kaspar030 left a comment

kaspar030 Feb 6, 2018

lorenz9314 Feb 6, 2018

kaspar030 Feb 6, 2018

lorenz9314 Feb 6, 2018

kaspar030 Feb 6, 2018

lorenz9314 Feb 6, 2018

kaspar030 Feb 6, 2018

lorenz9314 Feb 6, 2018

kaspar030 Mar 6, 2018

lorenz9314 Mar 6, 2018

kaspar030 commented Feb 6, 2018

kaspar030 left a comment

kaspar030 Feb 6, 2018

lorenz9314 Feb 6, 2018

lorenz9314 commented Feb 6, 2018

emmanuelsearch commented Mar 2, 2018

lorenz9314 commented Mar 6, 2018

emmanuelsearch commented Mar 6, 2018

kaspar030 Mar 6, 2018

kaspar030 Mar 6, 2018

jnohlgard commented Mar 6, 2018

lorenz9314 commented Mar 7, 2018

emmanuelsearch commented Mar 9, 2018

lorenz9314 commented Mar 16, 2018

kaspar030 left a comment

kaspar030 Mar 16, 2018

kaspar030 Mar 16, 2018

lorenz9314 Mar 16, 2018

kaspar030 left a comment

lorenz9314 commented Mar 16, 2018

pkg: add cn-cbor CBOR implementation plus unit tests #8467

pkg: add cn-cbor CBOR implementation plus unit tests #8467

Conversation

lorenz9314 commented Jan 27, 2018

Contribution description

Issues/PRs references

kaspar030 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaspar030 commented Feb 6, 2018

kaspar030 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorenz9314 commented Feb 6, 2018

emmanuelsearch commented Mar 2, 2018

lorenz9314 commented Mar 6, 2018

emmanuelsearch commented Mar 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnohlgard commented Mar 6, 2018

lorenz9314 commented Mar 7, 2018

emmanuelsearch commented Mar 9, 2018

lorenz9314 commented Mar 16, 2018

kaspar030 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaspar030 left a comment

Choose a reason for hiding this comment

lorenz9314 commented Mar 16, 2018