Skip to content
This repository has been archived by the owner on Apr 9, 2020. It is now read-only.

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

Open
ks156 opened this issue May 27, 2015 · 6 comments
Open

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

ks156 opened this issue May 27, 2015 · 6 comments

Comments

@ks156
Copy link

ks156 commented May 27, 2015

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
Copy link
Author

ks156 commented May 27, 2015

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.

cabo added a commit that referenced this issue May 27, 2015
@cabo
Copy link
Owner

cabo commented May 27, 2015

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.)

@ks156
Copy link
Author

ks156 commented May 27, 2015

Hello,

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

@cabo
Copy link
Owner

cabo commented May 27, 2015

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.

@ks156
Copy link
Author

ks156 commented May 27, 2015

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
Copy link
Author

ks156 commented May 28, 2015

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 ks156 changed the title cn-cbor not portable for small MCU cn-cbor not portable for MCU that doesn't support alignment May 28, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants