cbor: CBOR implementation for RIOT-OS (SWP) #1415

krf · 2014-07-14T14:54:01Z

cbor: CBOR implementation for RIOT-OS                                                                                                                                                                  

This is a malloc-free implementation of the Concise Binary Object                                                                                                                                      
Representation (CBOR) data format for the RIOT-OS.                                                                                                                                                     

This implementation mostly stand-alone, and it should be pretty easy to                                                                                                                                
port to other platforms. We're only using the C STL and some custom                                                                                                                                    
network-related functionaliy which could be easily replaced by depending                                                                                                                               
on arpa/inet.h.                                                                                                                                                                                        

The CBOR API is straight-forward to use and provides encoding/decoding                                                                                                                                 
functionality for all major C types, such as:                                                                                                                                                          
- int                                                                                                                                                                                                  
- uint64_t                                                                                                                                                                                             
- int64_t                                                                                                                                                                                              
- float                                                                                                                                                                                                
- double                                                                                                                                                                                               
- char*                                                                                                                                                                                                
- struct tm                                                                                                                                                                                            
- time_t                                                                                                                                                                                               

It is possible to conditionally compile this module via CFLAGS:                                                                                                                                        
- CBOR_NO_CTIME: All ctime related features removed
- CBOR_NO_FLOAT: All floating-point related features removed
- CBOR_NO_STREAM_DECODE: Generic decoding feature removed
  (cbor_stream_decode normally prints out a CBOR data stream to stdout)

krf · 2014-07-14T14:56:18Z

More squashing wasn't possible without destroying the logical order of the commits. We could still squash, but then we would end up with just a very few commits.

Kijewski · 2014-07-14T15:12:02Z

sys/cbor/cbor.c

+#include <stdlib.h>
+#include <string.h>
+
+#define CBOR_TYPE_MASK          0xE0    // top 3 bits


Please use C style comments (/* … */).

OlegHahm · 2014-07-14T15:14:43Z

Blacklist the failing unittests (with insufficient memory where it applies).

Kijewski · 2014-07-14T15:17:19Z

sys/cbor/cbor.c

+/**
+ * Convert float @p x to network format
+ */
+static float htonf(float x)


This operation does not seem safe for arbitrary ABIs. If a floating point number is passed and/or returned in special FPU registers, then the multiple representations for ±NaN might get broken. I'd use int32_t or char[4] to pass the values.

Fixed that by using the appropriate uintXX_t types for input/output. Please check again.

Kijewski · 2014-07-14T15:45:22Z

What I'd really like for this PR would be feature subsetting, e.g. that the user could decide not to include floating point support.

Kijewski · 2014-07-14T15:52:08Z

Also your code does not need to be defensive. If the user uses the API wrong, then let the application crash mercilessly. We have the macro DEVELHELP that a user can set if defensive checks should be done:

#ifdef DEVELHELP
if (!s) {
    puts("cbor: s==NULL, your application will now crash"); 
    /* No return here. Let it crash. */
}
#endif

Input streams and buffers should still be tested, though, or the remote side could exploit the implementation.

LudwigKnuepfer · 2014-07-14T15:53:17Z

Regarding squashing: I'd say it's OK if there's just one big 'initial import' commit (without looking at the actual change set..).
Edit: I'm still of that opinion after looking at the CS.

krf · 2014-07-14T19:56:25Z

I've introduced the following macros to be able to conditionally leave out parts of the CBOR module:
* CBOR_NO_CTIME: All ctime related features removed
* CBOR_NO_FLOAT: All floating-point related features removed
* CBOR_NO_STREAM_DECODE: Generic decoding feature removed
(cbor_stream_decode normally prints out a CBOR data stream to stdout)

I've squashed the PR into a single commit and extended the commit message.

Regarding "not writing defensive code": Why is that bad? I really don't like code to be crashy just because we don't check user input.

krf · 2014-07-14T20:00:31Z

@OlegHahm "Blacklist the failing unittests" <- I don't quite understand. Isn't the problem that we're packing too many code into the unittest executable? At least that's how I'm interpreting the error message when running BOARD=chronos make:

/usr/lib/gcc/msp430/4.6.3/../../../../msp430/bin/ld: /home/krf/devel/src/RIOT/tests/unittests/bin/chronos/unittests.elf section `.rodata' will not fit in region `rom'
/usr/lib/gcc/msp430/4.6.3/../../../../msp430/bin/ld: section .vectors loaded at [000000000000ff80,000000000000ffff] overlaps section .rodata loaded at [000000000000ec6c,0000000000010879]
/usr/lib/gcc/msp430/4.6.3/../../../../msp430/bin/ld: region `rom' overflowed by 2476 bytes

Kijewski · 2014-07-14T20:21:10Z

Regarding "not writing defensive code": Why is that bad? I really don't like code to be crashy just because we don't check user input.

Normally defensive code is a good thing 👍, but not if you have a very limited ROM 👎 like some platforms we are targeting. Like you saw in this PR, the ROM for chronos is overflowing for the unittests. In such an environment you need to trust the programmer to write good code.

Kijewski · 2014-07-14T20:36:16Z

sys/cbor/cbor.c

+        s->data[s->pos++] = major_type | val;
+        return 1;
+    }
+    else if (val <= 0xff) {


You could refactor everything after the trivial case to first find the length, and then use a loop. Something like (untested):

int result; if (val <= 0xff) { major_type |= CBOR_UINT8_FOLLOWS; result = 2; } else … CBOR_ENSURE_SIZE(s, result); s->data[s->pos++] = major_type; for (int i = result - 1; i >= 0; --i) { s->data[s->pos++] = (val >> (8 * i)) & 0xff; } return result;

Done. Thanks for the hint.

This and my changes to decode_int reduces the code size in about 1K on native. Good.

OlegHahm · 2014-07-15T08:26:44Z

@krf:

@OlegHahm "Blacklist the failing unittests" <- I don't quite understand. Isn't the problem that we're packing too many code into the unittest executable? At least that's how I'm interpreting the error message when running BOARD=chronos make

Yes, you're interpreting it right. Usually, if an example (or test) application doesn't fit into the RAM (or ROM), we add the board to a black list in the Makefile, e.g. BOARD_INSUFFICIENT_RAM := chronos (resp. BOARD_BLACKLIST := chronos). But in the case of the unittests it might indeed make more sense to split them up in a way, that you can compile them and run them for every platform.

krf · 2014-07-15T09:14:02Z

we add the board to a black list in the Makefile, e.g. BOARD_INSUFFICIENT_RAM := chronos (resp. BOARD_BLACKLIST := chronos).

I can only add that to tests/unittests/Makefile. However, then the complete test suite won't be linked on that target. That's undesired, I guess. What can I do as a quick fix in order to get it past the CI?

krf · 2014-07-22T18:42:33Z

Okay. New changeset:

Restructured the unit test code
Introduce CBOR_NO_SEMANTIC_TAGGING

What else can I do to get this merged? :)

Kijewski · 2014-07-22T18:55:30Z

I see no show stoppers in here. Well done 👍

OlegHahm · 2014-07-22T19:08:38Z

That's an ACK?

krf · 2014-07-25T19:20:47Z

Squashed + rebased onto master another time. Let me know if there's anything left I can do.

Kijewski · 2014-07-25T20:16:44Z

Debug printing must be disabled by default.
The unittest for chronos and redbee-econotag must be BOARD_INSUFFICIENT_RAM'd.

Kijewski · 2014-08-01T09:51:50Z

@krf, ping.

krf · 2014-08-04T07:24:16Z

Updated PR.

Note: I'm not sure what is wrong with https://travis-ci.org/RIOT-OS/RIOT/builds/31483750. -- It also happens when running the 'compile_test.py' script for branch 'master'.

OlegHahm · 2014-08-04T08:23:41Z

compile_test.py succeeds on my computer and Travis seems to pass, too - apart from a weird timeout at the end.

OlegHahm · 2014-08-04T08:35:51Z

sys/cbor/cbor.c

+    return bytes_follow + 1;
+}
+
+static size_t encode_bytes(unsigned char major_type, cbor_stream_t *s, const char *data, size_t length)


Line is too long, see https://github.com/RIOT-OS/RIOT/wiki/Coding-conventions#general

krf · 2014-08-05T06:36:33Z

Issues fixed

OlegHahm · 2014-08-05T08:19:25Z

As far as I can see the Travis error happens already in master - seems to be a problem with q386. @Kijewski, can you take a look? make -C ./tests/unittests test BOARD=qemu-i386 generates no sensible output:

`--> make -C ./tests/unittests test BOARD=qemu-i386                                                                                                                                                [10:13:11][0]
make: Entering directory '/home/oleg/git/RIOT/tests/unittests'
/home/oleg/git/RIOT/Makefile.include:140: target '/home/oleg/git/RIOT/tests/unittests/bin/qemu-i386/tlsf.a' given more than once in the same rule
Type 'exit' to exit.

"And Then There Was Silence" (guess the artist).

Kijewski · 2014-08-05T15:06:49Z

No, the build history is happy: https://travis-ci.org/RIOT-OS/RIOT/builds.
I cannot reproduce the error at some, but is reliable occurs in Travis. I don't see any problems in this PR, so a cannot say what the problem is.

OlegHahm · 2014-08-05T15:54:33Z

Hm, so let's merge this?

OlegHahm · 2014-08-05T15:55:19Z

Maybe you can squash the formatting commits.

haukepetersen · 2014-08-05T15:57:47Z

I give an ACK when suqashed

This is a malloc-free implementation of the Concise Binary Object Representation (CBOR) data format for the RIOT-OS. This implementation mostly stand-alone, and it should be pretty easy to port to other platforms. We're only using the C STL and some custom network-related functionaliy which could be easily replaced by depending on arpa/inet.h. The CBOR API is straight-forward to use and provides encoding/decoding functionality for all major C types, such as: - int - uint64_t - int64_t - float - double - char* - struct tm - time_t It is possible to conditionally compile this module via CFLAGS: - CBOR_NO_SEMANTIC_TAGGING: All semantic-tagging features removed - CBOR_NO_CTIME: All ctime related features removed - CBOR_NO_FLOAT: All floating-point related features removed - CBOR_NO_PRINT: All features depending on printf removed

krf · 2014-08-05T19:14:20Z

Squashed.

OlegHahm · 2014-08-05T19:32:24Z

Hooray, we have CBOR! Nice Achievement, @krf and @Jane333!

cbor: CBOR implementation for RIOT-OS (SWP)

Kijewski · 2014-08-06T12:17:20Z

@OlegHahm, you merged this even though there were outstanding Travis errors. Every build since then fails.

miri64 · 2014-08-06T13:10:06Z

Since this is a Travis problem (at least to according to the discussion above) and not a problem with this PR, I think this is alright.

Kijewski · 2014-08-06T13:20:30Z

I cannot really investigate the problem, as I failed to get Travis to email me the compiled .elf file.

OlegHahm · 2014-08-06T13:39:11Z

What @authmillenon says. make -C ./tests/unittests test BOARD=qemu-i386 was failing on master before and after the merge.

Kijewski · 2014-08-06T13:42:10Z

But it wasn't. https://travis-ci.org/RIOT-OS/RIOT/builds

OlegHahm · 2014-08-06T13:52:25Z

I checked locally. Currently I'm not trusting Travis too much.

OlegHahm added new feature labels Jul 14, 2014

OlegHahm assigned haukepetersen Jul 14, 2014

Kijewski reviewed Jul 14, 2014
View reviewed changes

OlegHahm reviewed Aug 4, 2014
View reviewed changes

OlegHahm added a commit that referenced this pull request Aug 5, 2014

Merge pull request #1415 from krf/cbor

6c2b2ce

cbor: CBOR implementation for RIOT-OS (SWP)

OlegHahm merged commit 6c2b2ce into RIOT-OS:master Aug 5, 2014

Kijewski mentioned this pull request Aug 6, 2014

x86: disable --gc-sections for Travis' sake #1552

Merged

cabo mentioned this pull request Oct 22, 2020

Link needs an update cbor/cbor.github.io#63

Open

cbor: CBOR implementation for RIOT-OS (SWP) #1415

cbor: CBOR implementation for RIOT-OS (SWP) #1415

Conversation

krf commented Jul 14, 2014

krf commented Jul 14, 2014

Kijewski Jul 14, 2014

Choose a reason for hiding this comment

OlegHahm commented Jul 14, 2014

Kijewski Jul 14, 2014

Choose a reason for hiding this comment

krf Jul 14, 2014

Choose a reason for hiding this comment

Kijewski commented Jul 14, 2014

Kijewski commented Jul 14, 2014

LudwigKnuepfer commented Jul 14, 2014

krf commented Jul 14, 2014

krf commented Jul 14, 2014

Kijewski commented Jul 14, 2014

Kijewski Jul 14, 2014

Choose a reason for hiding this comment

krf Jul 14, 2014

Choose a reason for hiding this comment

OlegHahm commented Jul 15, 2014

krf commented Jul 15, 2014

krf commented Jul 22, 2014

Kijewski commented Jul 22, 2014

OlegHahm commented Jul 22, 2014

krf commented Jul 25, 2014

Kijewski commented Jul 25, 2014

Kijewski commented Aug 1, 2014

krf commented Aug 4, 2014

OlegHahm commented Aug 4, 2014

OlegHahm Aug 4, 2014

Choose a reason for hiding this comment

krf commented Aug 5, 2014

OlegHahm commented Aug 5, 2014

Kijewski commented Aug 5, 2014

OlegHahm commented Aug 5, 2014

OlegHahm commented Aug 5, 2014

haukepetersen commented Aug 5, 2014

krf commented Aug 5, 2014

OlegHahm commented Aug 5, 2014

Kijewski commented Aug 6, 2014

miri64 commented Aug 6, 2014

Kijewski commented Aug 6, 2014

OlegHahm commented Aug 6, 2014

Kijewski commented Aug 6, 2014

OlegHahm commented Aug 6, 2014