OpenCyphal / libcanard

A compact implementation of the Cyphal/CAN protocol in C for high-integrity real-time embedded systems
http://opencyphal.org
MIT License
327 stars 192 forks source link

Some platforms have no uint8_t etc. (one should use uint8_least_t/uint8_fast_t?) #184

Open TYuD opened 2 years ago

TYuD commented 2 years ago

Hi!

I see type uint8_t in your code, but some DSPs and MCUs have size of byte bigger then 8 bit. There are examples of stdint.h file (C/C++, no type uint8_t ):

// -------------------------------------- ADSP TS-201 compiler:

typedef unsigned int uint32_t;

typedef uint32_t uint_least8_t; typedef uint32_t uint_least16_t; typedef uint32_t uint_least32_t;

typedef uint32_t uint_fast8_t; typedef uint32_t uint_fast16_t; typedef uint32_t uint_fast32_t;

// -------------------------------------- TMS320C2804 compiler:

typedef unsigned int uint16_t; typedef unsigned long uint32_t;

typedef uint16_t uint_least8_t; typedef uint16_t uint_least16_t; typedef uint32_t uint_least32_t;

typedef uint16_t uint_fast8_t; typedef uint16_t uint_fast16_t; typedef uint32_t uint_fast32_t;

I think you should use uint8_least_t/uint8_fast_t instead of uint8_t etc.

I'm wrong?

pavel-kirienko commented 2 years ago

I don't think you can easily replace uint8 as it would likely break transfer payload management, which assumes that the buffer memory is byte-addressable. If you experimented with it and reported back your findings, I think that would be useful to many users, as similar issues were raised in the past (e.g., https://github.com/UAVCAN/libcanard/issues/55).

TYuD commented 2 years ago

Sorry. I liked your startup. It is much demanded. But I can't use it.

I use bitfields in structures and bitshifting to pack data:

//---------------------------------------------------- typedef struct { uint_fast16_t TypeFormat : 16; uint_fast8_t SystemFlags: 8 ; uint_fast8_t QueryID : 8 ; uint_fast32_t Reserve : 32; } TPressureSensorQuery; //---------------------------------------------------- typedef struct { uint_fast16_t TypeFormat : 16;

uint_fast8_t Error : 1 ; uint_fast8_t Ready : 1 ; uint_fast8_t SystemFlags: 6 ; uint_fast8_t UserFlags : 8 ;

uint_fast32_t Pressure : 32;

uint_fast32_t Time : 32; uint_fast32_t RareAuxData: 32; uint_fast8_t RareAuxID : 8 ; uint_fast32_t Reserve : 24; uint_fast32_t AuxData1 : 32; uint_fast32_t AuxData2 : 32; } TPressureSensorAnswer;

TYuD commented 2 years ago

Can you highlight typical sections of code where the discussed problem brings difficulties? Together we could think about how to solve them. It would be easier for me if these sections were in C/C++ language.

pavel-kirienko commented 2 years ago

I would recommend you to approach it this way instead: modify the codebase to use uint_least8_t in place of uint8_t and see if that causes breakage. You will need to modify the test suite to run on your platform though, which may prove tedious, so you should start with the most basic tests that validate the fundamentals, like https://github.com/UAVCAN/libcanard/blob/master/tests/test_private_rx.cpp.

TYuD commented 2 years ago

It is difficult for me to understand much amount of your code.

Types like uint8_fast_t I use for a long time. There are no problems with them as variables or structure fields.

Problems can be with pointers. They can not offset to 8 bits (uint8_t) in some platforms. In such a case one must use bit operations to pack/unpack data instead of pointer offset. I think you do just so when pack uintX_t (X<8, DSDL) into a message. In same a way data may be packet into char array, when size of char is bigger 8 bit. Char size may be parameterized in your code. Its value contain CHAR_BIT macro in file limits.h. Also this value can be pointed as a option of DSDL translators like little/big endean. It may be considered like a MTU value in transport layer.

TYuD commented 2 years ago

Hi, Pavel!

I have slightly modified the nunavutCopyBits function (I think this is the core function of serilization). Now it looks the nunavutCopyBits can work on platforms with different byte lengths. Even a little faster (without divisions). In the same way, one can modify the other functions of the library.

It is here: https://github.com/TYuD/hello_world/blob/master/serialization_2.h

Here are some small checks on the PC and on the TigerSHARC processor:

https://github.com/TYuD/hello_world/blob/master/main.cpp

pavel-kirienko commented 2 years ago

I opened a new issue over at Nunavut to track this: https://github.com/UAVCAN/nunavut/issues/233

Even a little faster (without divisions)

With any half-decent compiler, replacing divisions with shifts adds nothing but obscurity.