Use CHAR_BIT instead of hard-coded literal 8 in generated C code #233
Comments
As the example with the nunavutCopyBits function showed, it does not complicate much. Although it is possible that difficulties may arise in other places of the program. Nevertheless, do you intend to implement this feature or not? For example, because of this feature, I don't consider uavcan for my systems. |
|
Neither I nor Scott is likely to work on this in the foreseeable future, but a pull request (from you or anybody else) would be welcome. |
|
I could take the C-code of the program. It seems C is not needed directly, but a python code generator. I don't use a python. Is it enough to fix only the C-code? Maybe I'll fix the C-code, and you'll make changes to Python? |
|
I don't foresee any changes to Python being necessary. Updating the C templates should be enough I think; specifically, look for all occurences of |
|
Templates of jinja2 I know much worse than python |
|
I recommend reading the Jinja docs here (you don't need to read everything, just the intro is enough): https://jinja.palletsprojects.com/en/3.0.x/templates/ |
|
Is the following sequence possible:
Are the tests of items 2) and 5) the same? Are there these tests and where are they? |
|
Yes, but you may need to tweak the build/test workflow a little. The tests are located here: https://github.com/UAVCAN/nunavut/tree/main/verification/c/suite The build recipe is here: https://github.com/UAVCAN/nunavut/blob/main/verification/CMakeLists.txt Normally you would run the verification suite using this helper: https://github.com/UAVCAN/nunavut/blob/main/.github/verify.py Please read the contributing guide to understand how to use all that: https://nunavut.readthedocs.io/en/latest/docs/dev.html The tweak I referred to amounts to changing the verification build recipe such that it does not regenerate the support header on build (since you would be modifying it manually). I mean you can do that but should you really? Editing the template seems like a no-brainer, it's basically the same C code but with a slightly weirder syntax. |
|
Pavel, there is another problem. In some C compilers, the size of double is not necessarily 64 bits. Is it possible to use macros like FLOAT32, FLOAT64 instead of 'float' and 'double' keywords? Can You set these macros in the settings of the nunavut code generator? |
|
Pavel, I have update all functions in serialization.h (see: https://github.com/TYuD/hello_world/blob/master/serialization_2.h) and pass tests Here is test output: 50 Tests 8 Failures 0 Ignored
|
That is not currently supported. If We should perhaps define code-generation-time aliases for float32 and float64, similar to
It is enabled already as you can see here: https://github.com/UAVCAN/nunavut/blob/61232b31b82ba1b6a3b0bf82392975399a541137/verification/CMakeLists.txt#L150 N.B. it's always better to post CLI dumps as text rather than as images. |
|
Also I have much simplify functions nunavutGetIxx. Look at them please. Are they correct? |
Sorry. It's my first activity with GitHub |
Technically they are not because they rely on non-standard implementation-defined behavior: shifting a negative integer to the right is not guaranteed to produce the desired outcome (which is to extend the sign bit). |
I was sure this trick is standard. It would be to make sure additionally |
|
For platforms that don't have small integer types, it should be safe to make small-size getters/setters mere aliases of their larger counterparts. E.g.,
See https://en.cppreference.com/w/c/language/operator_arithmetic and https://en.cppreference.com/w/cpp/language/operator_arithmetic. Actually, I should correct myself: the behavior is implementation-defined in C and undefined in C++. |
I'm not following the instructions temporarily. I want to make sure that the solution exists without wasting time. Then I will study your technology stack. Am I distracting you a lot? |
|
I understand what you're saying but you are at a far greater risk of wasting time when you are not following the instructions. Seriously. Also, I'm not sure if anybody has attempted to develop Nunavut on Windows, so that's another possible source of issues. |
|
until C++20: For negative a, the value of a >> b is implementation-defined (in most implementations, this performs arithmetic right shift, so that the result remains negative). since C++20: The value of a >> b is a/2b, rounded down (in other words, right shift on signed a is arithmetic right shift) |
|
Nunavut aims to support C++14 and C99, so C++20 is not that relevant. |
I'm not going to be a nunavut developer. I just want a local feature for CHAR_BIT > 8 to appear in nunavut |
It is possible with the help of conditional compilation (#if/#else) use a fast algorithm for those compilers that perform an arithmetic shift to the right. For the rest compilers - slow algorithm |
agree. This is a good plan |
I don't agree with this. C++17 is explicitly supported and C++20 implicitly (i.e. you should be able to use the C++17 headers with C++20) |
|
@TYuD , thanks for the effort here. Yes, I don't have access to a windows PC so I've never tried to develop this project in that environment. Please let me know if there are issues. |
|
I do not know the rules of Github. Is our discussion a flood? |
Ok. c/suite/test_support.c works Ok on Windows/Qt. |
There is a nuance here. Both IEEE754 binary32 and IEEE754 binary64 standart can be supported by the compiler, so the protection static_assert(NUNAVUT_PLATFORM_IEEE754_DOUBLE) will pass. But! 'float' and 'double' may be the same. Namely IEEE754 binary32. IEEE754 binary64 may be realized as a 'long double'. Is it possible to exclude float64 (in future) using the code generator options? This is highly desirable for a number of systems |
I have added predefined macros UNITY_SUPPORT_64, UNITY_INCLUDE_FLOAT, UNITY_INCLUDE_DOUBLE. Now all tests passed: 50 Tests 0 Failures 0 Ignored |
It doesn't matter. If you look at the code you will see that the assertion checks will pass if and only if It is possible to make the code rely only on |
|
Hi! I have adapt tests for nunavut/support/serialization.h (test_support.c). Now they are suitable for platforms with CHAR_BIT > 8. I tested them with MinGW32 compiler on PC (CHAR_BIT==8) and with VisualDSP++ Studio on TS-201 processor (CHAR_BIT==32). All of 50 tests passed. Both projects are in nunavut_support_char repository. Now I am going to test it for TMS320F2802xx controllers (CHAR_BIT==16). |
|
Hi! Now tests passed for TI C2000 compiler v5.1.2 (controller TMS320F280023). CHAR_BIT == 16 |
|
I came across a problem with float64 again. The compiler does not support such numbers, only float32. Why an ASSERT compilation error is generated if my system does not use messages with float64 type fields? For example, I can only use integer fields. Even without float32. Why not allow me to translate such messages from DSDL to C? I suggest a simple solution. Instead of ASSERT, use conditional compilation: if the compiler does not support float32/float64, the corresponding sections are excluded from compilation with the help of #if pragmas |
There is option I'm not sure about the conditional compilation because it is prohibited by MISRA/AUTOSAR. @thirtytwobits is it advised that we make a deviation in this case?
This is great. Would you be inclined to submit your patches to the upstream? |
|
Maybe later. To do this, I will have to deal with python, jinja, etc. It is important that the solution exists. If someone is interested, I repeat the link again: https://github.com/TYuD/nunavut_support_char |
|
Greetings. I recently resumed trying to implement CHAR_BIT != 8. Something succeeded. Now the C-code is being generated and all unit tests are being passed. Variants for CHAR_BIT == 8 and CHAR_BIT == 32 was checked. I tried to remain unchanged code for CHAR_BIT == 8. Such sections are wrapped with preprocessor directive #if CHAR_BIT == 8. Unfortunately, I had to change the interface a bit. Instead of: XXX_serialize_( const XXX* obj, uint8_t* buffer, size_t* inout_buffer_size_octets/chars?); I had to write: XXX_serialize_( const XXX* obj, void* buffer, size_t buffer_size_fits, size_t* input_offset_bits); It seems that this is a fundamental problem and it is impossible to get around it. The fact is that with CHAR_BIT > 8, I cannot shift the pointer by 8 bits, but in the message, the data is packed with 8-bit alignment according to the DSDL specification. My version is in the repository https://github.com/TYuD/nunavut Do you have an interest in this direction? |
|
@TYuD Yes, I think this is interesting. Unfortunately, it will likely be necessary to rewrite the serialization routines completely or at least alter them significantly to support these odd large-byte-radix platforms. I would start with the support library, specifically with the functions that (de-)serialize primitives like
I would suggest replacing |
This is done. All tests from the \verification\c\suite folder passed. See https://github.com/TYuD/nunavut
At first, that's what I did: I used unsigned char*. Then I replaced it to void*. This does not limit in any way. You can pass a pointer of any type as a buffer, it is reduced to void* by default. Inside the library, char* is used as the minimum addressable word. |
Sure, but doesn't MISRA C prohibit casting
Okay, I must have misunderstood what you meant earlier when you referred to a fundamental problem that is impossible to get around. I personally would like to see a more generalized serialization code that supports non-octet-byte platforms. @thirtytwobits wdyt? |
The void* type is widely used in the standard library. For example, the memcpy function. This is safe if void* is converted to char*. This is exactly the kind of casting I use. It is very simple to return the type of parameters of the serialize function from void* to char*. But I wouldn't like to. Otherwise, you will have to make an explicit type conversion when calling serialize: XXX_serialize( char* buf, ... ); // declaration |
|
You shouldn't need a type conversion because |
But you widely use memmove with its void*
Good. Then another example: |
Conversion to
|
But internally memmove use reverse cast without problem
Then your code will become platform-dependent. It is necessary to think where uint8_t is, and where it is not. With the pointer 'void*' will always work. Another way is to use 'unsigned char*' always. I don't insist on 'void*'. It's easy to go back to 'unsigned char*' if MISRA is so important |
Exactly! What happens outside of our codebase is none of our concern. memmove may as well summon demons to do the copying, we don't care as long as it behaves up to the C standard and its usage is not prohibited by the applicable high-integrity coding standards.
Indeed it is. |
|
I think that an explicit cast of (T*)p is always correct if T is the type of the primitive and sizeof(T)==sizeof(char). Because char is a minimally addressable memory cell. In particular, the cast (char*)p is always correct. Another thing is that explicit type conversion is not a good practice itself. We could have passed a parameter with one meaning, and we converted it to an object with a different meaning. Maybe it means MISRA? However, for byte-by-byte copying of memmove, serialization, etc., such a limitation is unnecessary. After all, the meaning of such functions is to work with an object as with a piece of memory, and not with its functionality. But I will not argue with MISRA. If you insist, I will return to unsigned char. In my version, there may be other questions besides void*. I will be glad to hear comments |
There's definitely value in this where is unlocks DSPs and other specialized processors. |
|
@TYuD Can you please organize a pull request with your changes so we can discuss this more in-depth? |
Of course I can. Is it a problem that my code still requires a little cosmetic? |
|
Just make sure that the diff is sensible (no unnecessary changes), otherwise there should be no issues. |
It complicates the usage of Nunavut-generated code with those odd (DSP) platforms where CHAR_BIT != 8. See details at OpenCyphal/libcanard#184 (comment)
The text was updated successfully, but these errors were encountered: