cn-cbor icon indicating copy to clipboard operation
cn-cbor copied to clipboard

cn-cbor not portable for MCU that doesn't support alignment

Open ks156 opened this issue 10 years ago • 6 comments

I would use cn-cbor on a ARM Cortex-M0, and I'm facing a problem with some dependencies.

Here https://github.com/cabo/cn-cbor/blob/master/src/cn-cbor.c#L16 and here https://github.com/cabo/cn-cbor/blob/master/src/cn-encoder.c#L11, arpa/inet.h is included to use ntohl. Problem is that I don't want to include arpa/inet.h, which would be a waste of flash space.

I want then propose a modification : https://gist.github.com/ks156/9b8d1a1d97ec1b38f91e

I can send a pull request if you want, but before, this code must be reviewed. I only started to work with cn-cbor few days ago and I don't have enough experience to know if these changes can have unexpected side effects. Basically, there's no change as long as CBOR_COMPAT is not defined. But if someone can confirm, it would be nice.

BR,

Paul

ks156 avatar May 27 '15 06:05 ks156

I've made several tests with a cortex M0, and this lib seems not compatible with these kind of CPU. I'm experiencing tons of problems, especially with this part : https://github.com/cabo/cn-cbor/blob/master/src/cn-cbor.c#L138-L140

So for now, forget about my previous comment. There's other things to do to be compatible with Cortex MCU..

I'll see if I can do something about that.

ks156 avatar May 27 '15 11:05 ks156

Yes, the ntohxxp functions assume that you can do unaligned reads.
I have pushed a workaround in branch "unaligned". (TODO: This clearly can be done better, and there needs to be a proper way to set CBOR_CAN_DO_UNALIGNED_READS.)

cabo avatar May 27 '15 11:05 cabo

Hello,

Thank you for this branch. I'll try your branch once I'd find time and let you know if it works.

ks156 avatar May 27 '15 15:05 ks156

BTW, just for the historical record: Just including <arpa/inet.h> is not expensive (at least not on my machine):

alma:tmp cabo$ cat ai1.c
#include <arpa/inet.h>

int f() {return 1;}
alma:tmp cabo$ size ai1.o
__TEXT  __DATA  __OBJC  others  dec hex
75  0   0   32  107 6b
alma:tmp cabo$ cat ai2.c
int f() {return 1;}
alma:tmp cabo$ size ai2.o
__TEXT  __DATA  __OBJC  others  dec hex
75  0   0   32  107 6b
alma:tmp cabo$ 

Contiki (where this code was used before) defines its own ntohl. But we did use a Cortex-M3 for those tests, IIRC, so we didn't encounter the unaligned access problem.

cabo avatar May 27 '15 15:05 cabo

Indeed, M3 supports address alignment, which is not the case of M0. If what you've done works as expected, I suggest to fix this code, which will increase the portability of this lib.

ks156 avatar May 27 '15 19:05 ks156

Ok, I've found time to test, and it works like a charm. There's also an include of arpa/inet.h in cn-encoder.c, line 11 (https://github.com/cabo/cn-cbor/blob/unaligned/src/cn-encoder.c#L11) that should also be surrounded by #ifdef CBOR_CAN_DO_UNALIGNED_READS

Do you plan to merge these change in master ?

ks156 avatar May 28 '15 18:05 ks156