If you made the variable static const and assigned value of 28000 to it, then the compiler is effectively treating it as if it was a constant.(1) So that would indeed make the issue go away. But I assume your goal was to have it as a variable, right? I quickly looked in the code you provided later and it seems you use it as a constant, though.
In that regard: no. It retains all the complexity of C and adds its own.
Here, I quickly found a serious bug which overruns other variables in .bss:Code: [Select]static int16_t i16_bit_var ;
...
switch(number_bytes)
{
case 0x82 :
mymemcpy(&i16_bit_var , &can_data[4] , number_bytes) ;
You are copying 130(!!!) bytes from something that is only 4 bytes long (can_data[4 to 7]), to something that is only 2 bytes long (int16), overrunning the target by 128 bytes.
Always with memcpy, be super careful with the length, and prefer the pattern of using sizeof target as the final parameter, instead of some int n:
mymemcpy(&i16_bit_var, &can_data[4], sizeof i16_bit_var);
Homework: find the rest of such bugs and fix them.
Now you see how changing compiler settings or adding random keywords causes the compiler/linker to place variables in different order, so that something else gets corrupted and the bug seems to move elsewhere. Therefore by doing such changes, you are wasting time.
oh shit thanks, yes that should have a number_bytes &= 0x0F ; at the start of every case.
static int16_t i16_bit_var ;
...
mymemcpy(&i16_bit_var , &can_data[4] , number_bytes) ;
server_objects[object_search_index].value[node_id ] = (int32_t) i16_bit_var ;
{
int16_t i16_bit_var ;
mymemcpy(&i16_bit_var , &can_data[4] , number_bytes) ;
server_objects[object_search_index].value[node_id ] = (int32_t) i16_bit_var ;
} break; // braces added to create local namespace for i16_bit_var.
case 0x84 :
mymemcpy(&i32_bit_var , &can_data[4] , number_bytes) ;
server_objects[object_search_index].value[ node_id ] = (int32_t) i32_bit_var ;
break;
case 0x04 :
mymemcpy(&i32_bit_var , &can_data[4] , number_bytes) ;
server_objects[object_search_index].value[ node_id ] = (int32_t) i32_bit_var ;
break;
typedef struct
{
uint16_t index ;
uint8_t sub_index ;
uint8_t type ; // 0x80 for signed 0x40 for read only 0x20 for disable initialize in node dictionary
int32_t value[N_CANOPEN_NODES] ;
uint32_t last_received[N_CANOPEN_NODES] ;
uint32_t last_sent[N_CANOPEN_NODES] ;
} nodes_objects_def_t ;
Here, I quickly found a serious bug which overruns other variables in .bss:Code: [Select]static int16_t i16_bit_var ;
...
switch(number_bytes)
{
case 0x82 :
mymemcpy(&i16_bit_var , &can_data[4] , number_bytes) ;
You are copying 130(!!!) bytes from something that is only 4 bytes long (can_data[4 to 7]), to something that is only 2 bytes long (int16), overrunning the target by 128 bytes.
Always with memcpy, be super careful with the length, and prefer the pattern of using sizeof target as the final parameter, instead of some int n:
mymemcpy(&i16_bit_var, &can_data[4], sizeof i16_bit_var);
Homework: find the rest of such bugs and fix them.
Now you see how changing compiler settings or adding random keywords causes the compiler/linker to place variables in different order, so that something else gets corrupted and the bug seems to move elsewhere. Therefore by doing such changes, you are wasting time.
oh shit thanks, yes that should have a number_bytes &= 0x0F ; at the start of every case.
Why are the xxx_bit_vars static, anyway? You use them as temporary variables on two adjacent lines of code:Code: [Select]static int16_t i16_bit_var ;
...
mymemcpy(&i16_bit_var , &can_data[4] , number_bytes) ;
server_objects[object_search_index].value[node_id ] = (int32_t) i16_bit_var ;
should simply becomeCode: [Select]{
int16_t i16_bit_var ;
mymemcpy(&i16_bit_var , &can_data[4] , number_bytes) ;
server_objects[object_search_index].value[node_id ] = (int32_t) i16_bit_var ;
} break; // braces added to create local namespace for i16_bit_var.
and now you would save your precious RAM because compiler doesn't need to reserve global space from .bss for these variables. Only one CASE is entered per function execution, so local variable only uses one variable worth of stack. Compiler could even completely optimize out both the memcpy, and the variable! (Especially if you get rid of mymemcpy and just use standard memcpy, which the compiler can recognize).
And what's best, code becomes much more readable as the variable is declared just before use so you get visual context from reading the code.
What does "number_bytes" mean anyway, clearly it isn't actually number of bytes, but some MSB flag, too? Name it properly.
Then why do both of these CASEs do the exact same thing:Code: [Select]
case 0x84 :
mymemcpy(&i32_bit_var , &can_data[4] , number_bytes) ;
server_objects[object_search_index].value[ node_id ] = (int32_t) i32_bit_var ;
break;
case 0x04 :
mymemcpy(&i32_bit_var , &can_data[4] , number_bytes) ;
server_objects[object_search_index].value[ node_id ] = (int32_t) i32_bit_var ;
break;
In other cases, the highest bit seems to signify the signedness, but not for the 4-byte object. Is this intentional or a copy-pasta mistake? If latter, try to refactor to get rid of copypasta.
I'm not particularly fast/good at finding bugs in other's code. The fact I found these so quickly indicates there probably are many more.
I am not familiar with this compiler or processor - so this comment may not be applicable - but:
When dealing with structures, especially of mixed variable type, like say this example (picked at random)Code: [Select]typedef struct
{
uint16_t index ;
uint8_t sub_index ;
uint8_t type ; // 0x80 for signed 0x40 for read only 0x20 for disable initialize in node dictionary
int32_t value[N_CANOPEN_NODES] ;
uint32_t last_received[N_CANOPEN_NODES] ;
uint32_t last_sent[N_CANOPEN_NODES] ;
} nodes_objects_def_t ;
there is no guarantee that the compiler will put the variables in consecutive memory locations, but instead will word-align them - depending on what it is trying to optimise. This will really screw with your data if you carelessy use byte-copying functions (like memcpy).
If you are copying bytes in or out of a mixed variable structure it would be safer to force the compiler to "pack" the structure such that there are no gaps. In gcc this would mean defining the structure with the attribute __packed__
there is no guarantee that the compiler will put the variables in consecutive memory locations, but instead will word-align them - depending on what it is trying to optimise. This will really screw with your data if you carelessy use byte-copying functions (like memcpy).
So a new problem cropped up, same thing, variable being ignored at random. Making it "static" solves this. The variable in only used in this one C file. So why look at code anywhere else to determine it's usefulness ?
Well I set up static variables as these are needed often so I thought it will be a waste of time to create them every time.
Well I set up static variables as these are needed often so I thought it will be a waste of time to create them every time.
As a general rule if the structure is actually packed out with data that lines up to 4 bytes it will be stored as written. In fact it is my understanding that the pack directive will actually meddle with the structure layout to optimize it as instructed so you cannot then use pointers to copy data to a structure variable as you don't know how the bytes are ordered. Same problem with data being sent over a serial bus. I had to have a lot of arguments with someone about that as we were unable to communicate as he wanted to compact the data as much as possible and totally missed that now we did not know where in many bytes any bit of data was so that it could be recovered at the other end.
there is no guarantee that the compiler will put the variables in consecutive memory locations, but instead will word-align them - depending on what it is trying to optimise.
If you use pack you know that the bytes will be packed in with no padding. You still need to know the endianness and size, but once you do then you know the bytewise layout. You can use memcpy on the entire struct as usual to copy it in or out of a buffer to send over the wire, but you can't use normal pointers to individual members because they might be misaligned.
The "best" way of forcing a specific layout in memory, as long as you use GCC or Clang as a compiler, is to define a struct, give it the packed attribute, and 'pad' members as appropriate to yield the exact layout you want.
(Note that even if you use a different C compiler that doesn't support those attributes, the venerable "#pragma pack(1)" has been available on almost all C compilers I've run into for as long as I can remember (mid-nineties).)
If you use pack you know that the bytes will be packed in with no padding. You still need to know the endianness and size, but once you do then you know the bytewise layout. You can use memcpy on the entire struct as usual to copy it in or out of a buffer to send over the wire, but you can't use normal pointers to individual members because they might be misaligned.
That is itself a contradiction. If I can use memcpy on the structure then pointers are OK. I think you are however referring to copying the entire structure. I am talking about a structure that spans multiple messages so you must know where the data is by putting it there yourself.
there is no guarantee that the compiler will put the variables in consecutive memory locations, but instead will word-align them - depending on what it is trying to optimise.
No, it will not align based on "what it is trying to optimize." it will align them based on the ABI specification for the platform which on modern systems almost always requires natural alignment (alignment of primitive types == their size) for the standard primitive types. Of course, if a particular structure instance is only used as a local variable and proven to not be accessed outside the compilers visibility it is free to do optimizations that include removing the struct entirely while operating under the aegis of the "as-if" rule. But the externally-visible memory layout for programs that have defined behavior is fixed by the ABI and not subject to being changed by the compiler without warning.
Good point too. On a machine with bytes larger than 8 bits this will only copy the lower 8 bits of each byte, leaving 0s in the rest.
The typedef name intN_t designates a signed integer type with width N , no padding
bits, and a two’s complement representation. Thus, int8_t denotes such a signed
integer type with a width of exactly 8 bits.
The typedef name uintN_t designates an unsigned integer type with width N and no
padding bits. Thus, uint24_t denotes such an unsigned integer type with a width of
exactly 24 bits.
These types are optional. However, if an implementation provides integer types with
widths of 8, 16, 32, or 64 bits, no padding bits, and (for the signed types) that have a
two’s complement representation, it shall define the corresponding typedef names.
structure then pointers are OK.
structure then pointers are OK.
Be careful here:
* Pointers to the struct, packed or not, are always OK
* Pointers to the MEMBERS of the struct are always OK for a normal, non-packed struct
* Pointers to the MEMBERS of the packed struct might or might not be OK (depending on struct, object attributes, and even platform), and as such require special care and pretty deep understanding. Avoid.
var = structPtr->member;
My expectation is that this idiom would not be invalidated by having a packed structure. Is that right?
For understanding and clarity, a standard idiom in C is to reference structure members through a base pointer, like this:Code: [Select]var = structPtr->member;
My expectation is that this idiom would not be invalidated by having a packed structure. Is that right?
typedef struct __attribute__((packed))
{
uint8_t a;
uint32_t b;
} s_t;
s_t s;
...
s_t* p_s = &s;
...
p_s->b = 420;
typedef struct __attribute__((packed))
{
uint8_t a;
uint32_t b;
} s_t;
s_t s;
uint32_t * p_b = &s.b;
*p_b = 420;