memcpy() and volatile

[Simon]

My program is totally failing to work in parts and I suspect it is due to some variables being optimized out or in fact some code just being ignored. While the variables can be observed to have values in some places they do not.

Yes they are altered in interrupt routines. If I make the variables volatile every use of memcpy() throws a warning. What do I do?

[langwadt]

if they are changed in interrupt they have to be volatile, so don't use memcpy

https://stackoverflow.com/a/36729638

[Simon]

unfortunately as I am extracting data from CAN bus stream and putting in into the variable of the. type that it is I have little choice but to use memcpy().

The whole point of using volatile is not really for many of the reasons it is used but simply to make the damn compiler do as it is told. I want this calculation doing so do it and don't assume that value is 0 because you decided.

[Jeroen3]

No the compiler doesn't try to know better then you. You just didn't tell him everything. 

The compiler can only see what happens in the main thread. So, all functions called from main().
If it then tries to follow the logic and never sees any change to the `int number = 0` you've put there, it will think it's 0. No point in keeping in memory then.
If anything else changes `int number`, such as an interrupt handler, other thread or hardware. You must tell the compiler to assume this value is different every read. And for good measure, ensure it is written every write. In the exact order you wrote. With the volatile keyword.

CAN registers will be marked volatile by the vendor header file. You cannot use memcpy on hardware registers because,
- memcpy cannot guarantee data access width and alignment. (void*!)
- memcpy cannot guarantee data access order. (this can be implementation specific)
- memcpy cannot guarantee atomicity of the read. (data could change during copy)
^{(it may not even be threadsafe?)}
Thus memcpy takes `void*`, which should be fine to copy within standard application memory.

That the compiler throws you a discard volatile warning is your lucky day saving you from way more possible bugs.
memcpy is not the function for your are looking for.

[Simon]

I also get warnings where I use pointers, again unavoidable.

[Jeroen3]

Yes, you will get warnings anywhere you discard volatile keywords, you must use explicit casts if you are sure it's safe to do.

Can you share a minimal sample of your problematic code?

[SiliconWizard]

But it isn't, because memcpy calls may get optimized out in such contexts.
langwadt gave a link detailing why. Write your own copy function(s) in this case.

[Simon]

No the compiler doesn't try to know better then you. You just didn't tell him everything. 

The compiler can only see what happens in the main thread. So, all functions called from main().
If it then tries to follow the logic and never sees any change to the `int number = 0` you've put there, it will think it's 0. No point in keeping in memory then.
If anything else changes `int number`, such as an interrupt handler, other thread or hardware. You must tell the compiler to assume this value is different every read. And for good measure, ensure it is written every write. In the exact order you wrote. With the volatile keyword.

CAN registers will be marked volatile by the vendor header file. You cannot use memcpy on hardware registers because,
- memcpy cannot guarantee data access width and alignment. (void*!)
- memcpy cannot guarantee data access order. (this can be implementation specific)
- memcpy cannot guarantee atomicity of the read. (data could change during copy)
^{(it may not even be threadsafe?)}
Thus memcpy takes `void*`, which should be fine to copy within standard application memory.

That the compiler throws you a discard volatile warning is your lucky day saving you from way more possible bugs.
memcpy is not the function for your are looking for.

This is a micro controller, so single thread. As I said the problem is that I need the compiler to not assume variables have not changed, basically to not try to optimize that, to do that I have to rewrite everything from what you say. Sorry, but the compiler has caused the compilers own problem because even with no optimization turned on it will cause an issue. Now it is easy to simply disable the interrupt during the memcpy access but the compiler will not understand this either will it? I have no way of helping the compiler.

[Simon]

But it isn't, because memcpy calls may get optimized out in such contexts.
langwadt gave a link detailing why. Write your own copy function(s) in this case.

That is what I am thinking, sure I have been here before. Seen a lot of online discussions. I've found this for the sake of being too lazy to do my own from scratch:

void mymemcpy(void *dest, void *src, size_t n)
{
   // Typecast src and dest addresses to (char *)
   uint8_t *csrc = (uint8_t *)src;
   uint8_t *cdest = (uint8_t *)dest;
   
   // Copy contents of src[] to dest[]
   for (uint8_t i=0; i<n; i++)
   cdest = csrc;
}

Ok they used char variables, so I replaced with the more modern uint8_t

[Simon]

volatile also will not work with pointers...... arrrrrg

I think the problem may have also been missing header file inclusions. I'll retry.

[SiliconWizard]

Note that in C you don't need to cast from and to void *, so just:
uint8_t *csrc = src;
uint8_t *cdest = dest;
is fine.

Also, of course, you'll need to declare the parameters volatile:
void mymemcpy(volatile void *dest, volatile void *src, size_t n)
{
volatile uint8_t *csrc = src;
volatile uint8_t *cdest = dest;
...

That could probably be optimized a bit if there is some alignment guarantee (although in your case unaligned memory accesses may work fine and still more efficient than byte-by-byte copy, don't know.)

[Jeroen3]

A microcontroller with interrupts is at least two threads, and when you have multiple interrupt preemption levels, two or more.

Yes, if you want the compiler to assume values may change anytime you must use the volatile keyword.

A bit of code copying data isn't that complicated. Except your function there will still give you a discard volatile warning.
Make the arguments `volatile uint8_t *` and you eliminate all things in my earlier list except atomicity.

https://github.com/gcc-mirror/gcc/blob/master/libgcc/memcpy.c

example, untested

Code: [Select]

#include <stddef.h>

volatile unsigned char *
volatile_memcpy (volatile unsigned char *dest, volatile unsigned char *src, size_t len)
{
  volatile unsigned char *d = dest;
  volatile const unsigned char *s = src;
  while (len--)
    *d++ = *s++;
  return dest;
}
*return should also be volatile*

[Berni]

This is a micro controller, so single thread. As I said the problem is that I need the compiler to not assume variables have not changed, basically to not try to optimize that, to do that I have to rewrite everything from what you say. Sorry, but the compiler has caused the compilers own problem because even with no optimization turned on it will cause an issue. Now it is easy to simply disable the interrupt during the memcpy access but the compiler will not understand this either will it? I have no way of helping the compiler.

Microcontrollers can be just as multithreaded.

You can run a RTOS on a MCU and have many threads. Even without a RTOS you still have the equivalent of threads when interrupt service routines get called. The ISR could happen at any point in your main thread code, so things could change in between any two instructions. Heck these days MCUs even have CPU cache, so you can even run into cache coherency issues like you can in multicore CPUs (Except here it tends to happen between the CPU and DMA controller). All of this is just part of programing modern computers.

You can always just globally disable all optimizations, but this has a significant speed impact so you likely won't want to do that. So you have to explain to the C compiler what it can and can't assume.

The volatile keyword only sticks to the variable type, not the contents, hence why C will warn you when casting volatiles into other normal types, so after the cast the compiler can't know it is volatile. If you have issues with memcpy, you can always write your own mem copy function that takes volatile as parameters. Inside your custom mem copy you can optimize it as much as you like for what your application can handle, it can be a simple byte by byte FOR loop, or up to a fancy super fast assembler hand optimized function with SIMD instructions.

Just make sure you understand where the problem you are having is coming from. Reading weird values from RAM can be caused by compiler attempting to optimize multitreaded code, however it can also be caused by a buffer overrun somewhere tramping over your memory or a cache coherence issue holding onto old data.

[Simon]

Just make sure you understand where the problem you are having is coming from. Reading weird values from RAM can be caused by compiler attempting to optimize multitreaded code, however it can also be caused by a buffer overrun somewhere tramping over your memory or a cache coherence issue holding onto old data.

Yes you are also right there as the issues are beyond this I think and may be related to the code organization. I need to make sure that the headers with the declarations of the variables are included, but then why does the compiler not complain when a variable is not named? somehow it knows it exists, but as "this" C file does not officially know about it it optimizes.

[JPortici]

This is a micro controller, so single thread.

every interrupt service routine is another thread
if you use a RTOS, every thread is another thread

just cast the pointers, memcpy doesn't care if the data is volatile but the compiler has to warn you that you are passing volatile to something that doesn't expect volatile ---> cast away

To those concerned: I don't remember which part simon is using, but most recent can controllers use regular data ram for the FIFOs, you write the data to regular ram as you wish to the address the peripheral tells you, and then set the appropriate bit and the magic happens. To read, the peripheral tells you the FIFO has new data so you set the bit that you want to read the new entry and read the data from regular ram as you wish from the address the peripheral tells you.

[Simon]

Yes you are correct, this micro controller (SAMC21) uses the BOSCH M_CAN controller that uses RAM memory for all of the working memory.

[Jeroen3]

And then your intent is to copy it from this working memory to somewhere else in your application?

[Simon]

And then your intent is to copy it from this working memory to somewhere else in your application?

Yes, so RAM memory is used for the CAN TX and RX buffers, being just 8 dumb bytes so that anything can come and go the best way to extract the data or put the data in is to use memcpy().

[Psi]

You can turn off/on global interrupts in main() if its just for a short time to perform some steps that you don't want an interrupt happening in the middle of and messing things up. 

Any interrupt that happens while global interrupts are off will still cause that interrupt flag bit to get set, but the interrupt handler wont fire until you reenable global interrupts.  So it gets delayed, not lost. (Unless two of them happen, then yes, first one gets lost. But that is pretty unlikely most of the time.)

Just don't use any delay function while you have global interrupts off.  Figure out exactly where the critical instructions are and turn off global interrupts only for them.

[eutectique]

Yes, so RAM memory is used for the CAN TX and RX buffers, being just 8 dumb bytes so that anything can come and go the best way to extract the data or put the data in is to use memcpy().

If your buffers are word-aligned, threat them as two-word arrays. Use union and uint32_t to read/write.

[Simon]

So the whole problem was down to a variable that I set to a value in a function at start up. Having created it and assigned a value the compiler then decides that it does not apply any more (even when marked volatile) as it has not been changing and makes it 0 and then uses the variable in 4 calculations.

Defining it as const fixes it.

[DavidAlfa]

Disable interrupts before memcpy, then enable then again.

[tggzzz]

So the whole problem was down to a variable that I set to a value in a function at start up. Having created it and assigned a value the compiler then decides that it does not apply any more (even when marked volatile) as it has not been changing and makes it 0 and then uses the variable in 4 calculations.

Defining it as const fixes it.

That seem surprising. I wonder if that will still work if the compiler options change or when the compiler's optimisation algorithms are changed.

There is an awful/aweful amount of confusion about what "volatile" does and does not mean/guarantee.

Have you looked at the generated code to see what the compiler has instructed the processor to do?

[golden_labels]

My take on this is: don’t. No benefit of implementing your own memcpy over using a loop or, where logically identical operation is performed many times, wrapping that entire operation in a function.

memcpy is valuable, because the platform may provide a version better than a plain loop and do it transparently. Usually that’s a better copying algorithm. But also that the compiler knows memcpy semantics and may provide a better code⁽¹⁾. Modern implementations also completely ignore memcpy and replace it with memmove, avoiding potential errors from the programmer not being aware of memcpy limitations.

Your own code will not be able to offer that. In particular memmove can’t be reasonably implemented in pure C. So your gain is nothing more than one line less in code. The cost: having to write another function, less flexibility, sometimes making code bigger and slower.

⁽¹⁾ For example memcpy(*singlebyte, *singleOtherByte, 1) may easily be replaced with a single move instruction. In some cases even a zero-cost registers reassignment.

[eutectique]

So the whole problem was down to a variable that I set to a value in a function at start up. Having created it and assigned a value the compiler then decides that it does not apply any more (even when marked volatile) as it has not been changing and makes it 0 and then uses the variable in 4 calculations.

Defining it as const fixes it.

This sounds like a broken compiler. Can you show any code?

[IanB]

This is a micro controller, so single thread. As I said the problem is that I need the compiler to not assume variables have not changed, basically to not try to optimize that, to do that I have to rewrite everything from what you say.

The compiler removing code is an optimization. If you don't want the compiler to do such optimizations, have you tried changing the optimization flags on the compiler?

[IanB]

So the whole problem was down to a variable that I set to a value in a function at start up. Having created it and assigned a value the compiler then decides that it does not apply any more (even when marked volatile) as it has not been changing and makes it 0 and then uses the variable in 4 calculations.

Defining it as const fixes it.

Are you sure it was not a scoping problem? It is possible for variables of a given name to "hide" other variables with the same name due to visible scope. It may therefore appear that after you assign a value to a variable that the value disappears on return from a function.

[Berni]

So the whole problem was down to a variable that I set to a value in a function at start up. Having created it and assigned a value the compiler then decides that it does not apply any more (even when marked volatile) as it has not been changing and makes it 0 and then uses the variable in 4 calculations.

Defining it as const fixes it.

That does sound like RAM being corrupted by something writing out of bounds. Often these are a real b***h to find, but since you have a variable you normally write once and it very repeatably gets trampled, this makes it easy to catch by simply placing a memory write breakpoint onto it. Whenever it breakpoints on code that does not look like it should be writing to it, you have your out of bounds corruption culprit.

[golden_labels]

So the whole problem was down to a variable that I set to a value in a function (…)Defining it as const fixes it.

That sounds like accidental hiding of a bug in code, not a fix.

Can you provide a minimal code reproducing the issue, preferably with the corresponding assembly output? And the compiler used, including version?

[Simon]

This is a micro controller, so single thread. As I said the problem is that I need the compiler to not assume variables have not changed, basically to not try to optimize that, to do that I have to rewrite everything from what you say.

The compiler removing code is an optimization. If you don't want the compiler to do such optimizations, have you tried changing the optimization flags on the compiler?

All of this trashing of my code has been done with no optimizations set. It is utterly ridiculous that clear and plain statements are just blatantly ignored. I'm sure it is not the first time I have been around this sort of stupid circuit, even volatile does naf all.

It is the standard compiler with microchip studio.

[Simon]

So the whole problem was down to a variable that I set to a value in a function (…)Defining it as const fixes it.

That sounds like accidental hiding of a bug in code, not a fix.

Can you provide a minimal code reproducing the issue, preferably with the corresponding assembly output? And the compiler used, including version?

Don't be silly, at this size of project sniff near it and it blows up in your face. It will be a task in itself to reproduce the error as a slimmed down version as that will completely change everything.

[Andy Watson]

So the whole problem was down to a variable that I set to a value in a function (…)Defining it as const fixes it.

Where, exactly have you declared the offending variable(s)? Some of your replies hint of a potential scope problem. If you can't show the code, could you atleast give a minimal outline.

[IanB]

All of this trashing of my code has been done with no optimizations set. It is utterly ridiculous that clear and plain statements are just blatantly ignored. I'm sure it is not the first time I have been around this sort of stupid circuit, even volatile does naf all.

It is the standard compiler with microchip studio.

Well, be careful with assumptions here. Typically a compiler will have some optimizations turned on by default. If you want no optimizations applied, you would normally have to explicitly turn them off, for example by using the "-O0" flag or whatever applies for the particular compiler.

"No optimizations set" is not the same thing as "Disable optimizations".

[gf]

Code: [Select]

void mymemcpy(void *dest, void *src, size_t n)
{
// Typecast src and dest addresses to (char *)
uint8_t *csrc = (uint8_t *)src;
uint8_t *cdest = (uint8_t *)dest;

// Copy contents of src[] to dest[]
for (uint8_t i=0; i<n; i++)
cdest[i] = csrc[i];
}


Why do you declare the loop variable uint8_t, and not size_t?
For n >= 256, it will loop vorever.

[SiliconWizard]

Code: [Select]

void mymemcpy(void *dest, void *src, size_t n)
{
// Typecast src and dest addresses to (char *)
uint8_t *csrc = (uint8_t *)src;
uint8_t *cdest = (uint8_t *)dest;

// Copy contents of src[] to dest[]
for (uint8_t i=0; i<n; i++)
cdest[i] = csrc[i];
}


Why do you declare the loop variable uint8_t, and not size_t?
For n >= 256, it will loop vorever.

Which is arguably fun.

[golden_labels]

All of this trashing of my code has been done with no optimizations set. It is utterly ridiculous that clear and plain statements are just blatantly ignored.

That response alone makes me believe, that there is a discrepancy between what the code actually means and what you believe it means. Which is very common, because C is a unexpectedly abstract language, falsely advertised as close to bare metal, and also horrendously complicated. Not only you wouldn’t be the first person to fall in this trap, but this happens to seasoned experts.

Of course I don’t press you to post any code. But in my eyes this fits the pattern of accidental hiding of a bug.

[brucehoult]

Code: [Select]

void mymemcpy(void *dest, void *src, size_t n)
{
// Typecast src and dest addresses to (char *)
uint8_t *csrc = (uint8_t *)src;
uint8_t *cdest = (uint8_t *)dest;

// Copy contents of src[] to dest[]
for (uint8_t i=0; i<n; i++)
cdest[i] = csrc[i];
}


Why do you declare the loop variable uint8_t, and not size_t?
For n >= 256, it will loop vorever.

Using uint8_t also makes the code less efficient because on a 32 bit machine it has to add (at least) an extra `uxtb` (or `and #0xFF` etc) to the loop.

[IanB]

Ok they used char variables, so I replaced with the more modern uint8_t

Why would you do that? uint8_t is not more modern, it is simply different.

"char" represents the basic storage unit of memory, which is appropriate when copying memory around. It is an amount of memory that can hold one character (memory size = 1).

"uint8_t" represents an unsigned integer exactly 8 bits long, which is appropriate when doing unsigned arithmetic with numbers that fit in the range 0 to 255.

Since memcpy is copying memory, it uses char, deliberately so. Changing a memory copy routine to work with numbers instead of the particular object specified to be one unit of memory does not make good sense.

[brucehoult]

Ok they used char variables, so I replaced with the more modern uint8_t

Why would you do that? uint8_t is not more modern, it is simply different.

"char" represents the basic storage unit of memory, which is appropriate when copying memory around. It is an amount of memory that can hold one character (memory size = 1).

"uint8_t" represents an unsigned integer exactly 8 bits long, which is appropriate when doing unsigned arithmetic with numbers that fit in the range 0 to 255.

Since memcpy is copying memory, it uses char, deliberately so. Changing a memory copy routine to work with numbers instead of the particular object specified to be one unit of memory does not make good sense.

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.

[Simon]

Code: [Select]

void mymemcpy(void *dest, void *src, size_t n)
{
// Typecast src and dest addresses to (char *)
uint8_t *csrc = (uint8_t *)src;
uint8_t *cdest = (uint8_t *)dest;

// Copy contents of src[] to dest[]
for (uint8_t i=0; i<n; i++)
cdest[i] = csrc[i];
}


Why do you declare the loop variable uint8_t, and not size_t?
For n >= 256, it will loop vorever.

Using uint8_t also makes the code less efficient because on a 32 bit machine it has to add (at least) an extra `uxtb` (or `and #0xFF` etc) to the loop.

OK so the original function I copied off the net used "char" or "int"  (can't remember), my compiler complained, so I changed to uint8_t to shut it up, in my mass renaming of memcpy() calls I also altered the header file that the original memcpy() is in so the compiler saw 2 definitions with different arguments pointing at size_t, before I figured this I changed that to uint8_t.

So I need to change that back to size_t. My copy operations are on only a few bytes at a time at most 4 but yes I should provide more scope to the function.

Using uint8_t also makes the code less efficient because on a 32 bit machine it has to add (at least) an extra `uxtb` (or `and #0xFF` etc) to the loop.

What does this mean? I'm interested. Are you saying that 16 bits is the smallest a 32 bit (ARM) machine will work with anyway? I am aware that an ARM0+ has the ability to transfer two peices af data on it's 32 bit bus at once due to the thumb instruction set (and that is as clever as I get) so I tend to not oversize variables where practical.

[Simon]

All of this trashing of my code has been done with no optimizations set. It is utterly ridiculous that clear and plain statements are just blatantly ignored.

That response alone makes me believe, that there is a discrepancy between what the code actually means and what you believe it means. Which is very common, because C is a unexpectedly abstract language, falsely advertised as close to bare metal, and also horrendously complicated. Not only you wouldn’t be the first person to fall in this trap, but this happens to seasoned experts.

Of course I don’t press you to post any code. But in my eyes this fits the pattern of accidental hiding of a bug.

Is C++ better in this respect? I have been trying to get started in it for a long time and it's a natural step.

[Simon]

So code wise. To simplify by ignoring the files that are not used with this variable.

I have a main.c file, a motor.c file and a motor.h file. motor.h is declared in main.c.

The variable is only defined in motor.c as "volatile int32_t nominal_current".

A function defined in motor.c and declared in motor.h has a line that says:

nominal_current = 28000 ;

The function which sets up other variables too that are possibly also being optimized out but not causing an issue is called in main() before while(1){main program loop}.

A function called control() defined in motor.c and declared in motor.h uses nominal_current in 4 calculations and is called in the while(1) loop in main()

So if I pause after the function has run to set the variable up it has a value of 28000, if I pause in the control() function it's value is now "0".

[Simon]

All of this trashing of my code has been done with no optimizations set. It is utterly ridiculous that clear and plain statements are just blatantly ignored. I'm sure it is not the first time I have been around this sort of stupid circuit, even volatile does naf all.

It is the standard compiler with microchip studio.

Well, be careful with assumptions here. Typically a compiler will have some optimizations turned on by default. If you want no optimizations applied, you would normally have to explicitly turn them off, for example by using the "-O0" flag or whatever applies for the particular compiler.

"No optimizations set" is not the same thing as "Disable optimizations".

Optimization is set to "none (-O0)" I realize that some optimization may be taking place but this seems be another problem.

[Simon]

These are the compiler options I don't know if I need the first two, for a micro controller application they sound like trouble.

[tggzzz]

All of this trashing of my code has been done with no optimizations set. It is utterly ridiculous that clear and plain statements are just blatantly ignored.

That response alone makes me believe, that there is a discrepancy between what the code actually means and what you believe it means. Which is very common, because C is a unexpectedly abstract language, falsely advertised as close to bare metal, and also horrendously complicated. Not only you wouldn’t be the first person to fall in this trap, but this happens to seasoned experts.

Of course I don’t press you to post any code. But in my eyes this fits the pattern of accidental hiding of a bug.

Is C++ better in this respect? I have been trying to get started in it for a long time and it's a natural step.

Read https://yosefk.com/c++fqa/index.html and you will have your answer. I particularly like the bit on "const", since I remember the C++ committee spending years discussing whether it should be possible or impossible to "cast away constness".

IMNSHO if you haven't fully grokked C, then C++ isn't for you.

[tggzzz]

All of this trashing of my code has been done with no optimizations set. It is utterly ridiculous that clear and plain statements are just blatantly ignored.

That response alone makes me believe, that there is a discrepancy between what the code actually means and what you believe it means. Which is very common, because C is a unexpectedly abstract language, falsely advertised as close to bare metal, and also horrendously complicated. Not only you wouldn’t be the first person to fall in this trap, but this happens to seasoned experts.

Of course I don’t press you to post any code. But in my eyes this fits the pattern of accidental hiding of a bug.

Precisely, in all respects.

[Siwastaja]

I also get warnings where I use pointers, again unavoidable.

Add -Werror on command line, problem solved. Now you have to fix your code for it to compile.

[Siwastaja]

So the whole problem was down to a variable that I set to a value in a function at start up. Having created it and assigned a value the compiler then decides that it does not apply any more (even when marked volatile) as it has not been changing and makes it 0 and then uses the variable in 4 calculations.

Defining it as const fixes it.

This sounds like a broken compiler. Can you show any code?

Compilers are rarely broken. Knowing the history of this poster, it is not very good idea to suggest that. The code probably has large issues with the very basics all over it, and any random change is going to trigger a random observation of something "starting to work" or "stopping from working". As we never get any code, it isn't very helpful to reply to these threads.

[Siwastaja]

The compiler removing code is an optimization. If you don't want the compiler to do such optimizations, have you tried changing the optimization flags on the compiler?

Do not even suggest this. This is NEVER the solution. Code needs to be fixed instead. Compiler never removes code which has a purpose in the C abstract machine (except for rare cases of actual compiler bugs).

[Simon]

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 ?

[Simon]

this is where it all happens. The variables under runtime now all set static are the ones that love to just come and go. The main escapade is with torque_target  as a simple int32_t it would be reset to 0 on certain runs, the same when making it volatile, as a static it works. I thought a variable declared in a C file was implicitly static and at any rate it cannot be modified from outside that file unless declared in a header file as extern. Yet when I forcibly state it, it works.

Code: [Select]

/*
* motor_control.c
*
* Created: 16/02/2024 13:44:02
*  Author: cbsadmin
*/

#include <math.h>
#include "samc21.h"
#include "functions.h"
#include "rtc.h"
#include "system.h"
#include "canopen.h"
#include "canopen_object_dictionary.h"
#include "canopen_sdo.h"
#include "canopen_pdo.h"
#include "canopen_dsp402.h"

#include "device_jetnado_ecu.h"
#include "motor_control.h"



/***************** CONSTANTS **************************/

const int16_t min_belt_speed_setting = 5   ; // if changed update min_motor_rpm
const int16_t mpm_to_rpm               = 38  ; // if changed update min_motor_rpm
const int32_t motor_current_limit = 28000 ;
const int16_t torque_limit = 1000 ;
const int32_t starting_current = 2250 ;
const int32_t av_speed_factor = 10 ;
const int32_t av_torque_factor = 20 ;
const float tacho_length_m           = 0.0160792 ; // was 0.00788  // m 12 pulses/rev
//float tacho_length_m           = 0.0080396 ; // was 0.00788  // m 12 pulses/rev


////////////////////////////////// RUNTIME VARIABLES /////////////////////////

static int32_t motor_user_current_limit  ; // user current limit
static int32_t speed_actual   ;
static int32_t torque_actual  ;
static int32_t av_speed_actual ;
static int32_t av_torque_actual ;
static int32_t voltage ;
static int32_t torque_target  ;
static int32_t speed_target   ;

static int32_t starting_torque ;
static int32_t direction ;



volatile time_recrord_t time_record ;

void motor_control(uint16_t timeout)
{
static volatile uint32_t last_run = 0 ;

if ((time_ms - last_run) > timeout)
{
last_run = time_ms ;
// start code body

//***************************************************** safe to move ***********************************************

voltage = (server_objects[DRIVE_VOLTAGE_ACTUAL_VALUE].value[1] + server_objects[DRIVE_VOLTAGE_ACTUAL_VALUE].value[2]) / 2 ;

if ( ENABLE_UNDERVOLTAGE_LOCKOUT && (voltage > 0 ) && (voltage < MINIMUM_SUPPLY_VOLTAGE ) )
ecu_data.low_voltage = 1 ;
else ecu_data.low_voltage = 0 ;

if (network_status[MASTER_NODE_ID].heartbeat_present ) ecu_data.comms_failure = 0 ; // comms check
else ecu_data.comms_failure = 1 ;

if (GUARD_SAFETY_ENABLE) ecu_data.guard_open = !pin_read(guard_switch_pin) ; // Update guard switch status

if(!pin_read(estop_pin))  // if e-stop pressed update ecu flag
{
ecu_data.estop_pressed = 1 ;
ecu_data.stopped = 1       ;
}

// Ascertain motor safe to move
if (ecu_data.comms_failure || ecu_data.low_voltage || ecu_data.estop_pressed || ecu_data.guard_open )
{
ecu_data.safe_to_move = 0 ;

torque_target         = 0 ;
ecu_data.starting     = 0 ;
ecu_data.stopping     = 0 ;
ecu_data.running      = 0 ;
ecu_data.stopped      = 1 ;
}
else
{
ecu_data.safe_to_move = 1 ;
}

// end safety checks *************************************************************

ecu_data.airPressureCurrent = (uint8_t) (ret_map( adc_result[1][6] , 10 , 4000 , 0 , 160 ) ) ;

ecu_data.distance = (int32_t) (cable_taco_counts * tacho_length_m * 10) ;

//ecu_data.cable_speed = ecu_data.cable_speed ;

if (ecu_data.enabled)
{
speed_actual =  ( ret_absolute_value_int(server_objects[VELOCITY_ACTUAL_VALUE].value[1])
+ ret_absolute_value_int(server_objects[VELOCITY_ACTUAL_VALUE].value[2]) ) / 2 ;

av_speed_actual = (av_speed_actual * (av_speed_factor - 1) + speed_actual) / av_speed_factor ;

torque_actual = (ret_absolute_value_int( server_objects[TORQUE_ACTUAL_VALUE].value[1] )
+ ret_absolute_value_int(server_objects[TORQUE_ACTUAL_VALUE].value[2]) ) / 2  ;

av_torque_actual =  (av_torque_actual * (av_torque_factor - 1) + torque_actual ) / av_torque_factor ;

ecu_data.cable_speed = (int8_t) average_cable_speed ;
ecu_data.speed = (int8_t) (speed_actual / mpm_to_rpm ) ; // sent to HMI
ecu_data.torque = av_torque_actual * motor_user_current_limit / motor_current_limit ;
}


if (!ecu_data.enabled )
{
ecu_data.torque = 0 ;
ecu_data.speed = 0 ;

motor_user_current_limit = (motor_current_limit * hmi_cmd.torqueSetpoint) / 1000U  ; // workout unity fraction of max current and multiply by max current

server_objects[NOMINAL_CURRENT].value[1] = (motor_current_limit * hmi_cmd.torqueSetpoint) / 10000U ; // update nominal current in units of cA
server_objects[NOMINAL_CURRENT].value[2] = (motor_current_limit * hmi_cmd.torqueSetpoint) / 10000U ;

starting_torque = starting_current * 1000 / motor_user_current_limit ;

speed_actual = 0 ;
}

// end code body
}
}

void speed_update(uint16_t timeout)
{
static uint32_t last_run = 0 ;
static int32_t speed_error   ;
static int32_t adjustment ;

if (ecu_data.enabled && ((time_ms - last_run) > timeout))
{
last_run = time_ms ;
// start code body
// speed control

speed_target = (int32_t) (hmi_cmd.speedSetpoint * mpm_to_rpm) ;
// Direction setup
if (speed_target > 0)
direction = 1 ;

if (speed_target < 0) // if a reverse speed has been set
{
speed_target *= -1 ;     // make the target speed positive again but
direction  = -1 ; // set the reverse factor used to invert the final torque settings
}

if(speed_target > 0)
{
ecu_data.running = 1 ;

speed_error = ret_absolute_value_int(speed_actual - speed_target)  ;
//adjustment =  ret_map_int(speed_error, 1, 1000, 0, 100 )  ;

if (speed_error < 200) adjustment  = 1 ;
if ((speed_error > 199) && (speed_error < 500)) adjustment = 5 ;
if (speed_error > 499) adjustment = 20 ;

if (speed_actual > speed_target)   // slow down
torque_target -=  adjustment ;

if (speed_actual < speed_target )  // speed up
torque_target += adjustment ;

if (torque_target > torque_limit)
torque_target = torque_limit ;

if (torque_target < 0 )
torque_target = 0 ;
}
/*
if (speed_target == 0)
torque_target = 0 ;
*/
server_objects[TARGET_TORQUE].value[1] = (int32_t) (torque_target * direction * -1) ;
server_objects[TARGET_TORQUE].value[2] = (int32_t) (torque_target * direction     ) ;

// end code body
}
}



void ecu_data_initialization(void)
{
ecu_data.stopped      = 1 ;
/*
ecu_data.cable_speed  = 0 ;

ecu_data.stopping     = 0 ;
ecu_data.starting     = 0 ;
ecu_data.running      = 0 ;
ecu_data.enabled      = 0 ;
ecu_data.user_enable  = 0 ;
ecu_data.safe_to_move = 0 ;


av_speed_actual = 0 ;
av_torque_actual = 0 ;*/
}



void motor_ds402_control(uint16_t run_interval)
{
static uint32_t last_run_time = 0 ;

if ((time_ms - last_run_time) > run_interval)
{
last_run_time = time_ms ;

// motor power enable
if ( ecu_data.user_enable && !ecu_data.enabled && ecu_data.safe_to_move)
{
dsp402_power_on( 1) ;
dsp402_power_on( 2) ;

if (network_status[1].powered_up && network_status[2].powered_up)
{
ecu_data.enabled = 1 ;
}
}

// motor power disable
if (ecu_data.stopped && ecu_data.enabled && !ecu_data.user_enable)
{
dsp402_power_off( 1) ;
dsp402_power_off( 2) ;

if (network_status[1].powered_down && network_status[2].powered_down)
{
ecu_data.enabled = 0 ;
ecu_data.running = 0 ;
}
}
}
}



void motor_start(uint16_t run_interval)
{
static uint32_t last_run_time = 0 ;

//starting_torque = starting_current_mA / 28 ;

if (ecu_data.enabled && !ecu_data.running && !network_status[0].powering_down && ((time_ms - last_run_time) > run_interval))
{
// start code body
if ( !ecu_data.starting && (speed_target > 0) )
{
ecu_data.starting = 1 ;
ecu_data.stopped = 0 ;
ecu_data.stopping = 0 ;
torque_target = starting_torque ;
}

// Motor starting COULD BENEFIT FROM USING START BUTTON TO OVER RIDE AND FOR TORQUE INCREASE
if ( ecu_data.starting && (speed_actual < speed_target)
&& (torque_target < torque_limit)  )
{
torque_target += 10 ;
}

// Switch from starting to running
if ( ecu_data.starting &&  ( speed_actual > ( min_belt_speed_setting * mpm_to_rpm ) ) )
{
ecu_data.running = 1 ;
ecu_data.starting = 0 ;
}

server_objects[TARGET_TORQUE].value[1] = (int32_t) (torque_target * direction * -1) ;
server_objects[TARGET_TORQUE].value[2] = (int32_t) (torque_target * direction     ) ;

// end code body
last_run_time = time_ms ;
}
}



void user_button_ctrl(uint16_t run_interval)
{
static uint32_t last_run_time = 0 ;

if ((time_ms - last_run_time) > run_interval)
{
// start code body

if (hmi_cmd.machineStart) // User enabling
{
ecu_data.user_enable = 1 ;
ecu_data.cable_stall = 0 ;
ecu_data.belt_slip = 0 ;
}

if (hmi_cmd.machineStop) // User disabling
{
ecu_data.user_enable = 0 ;
}

// end code body
last_run_time = time_ms ;
}
}



void motor_stopping(uint16_t run_interval)
{
static uint32_t last_run_time = 0 ;

if ((time_ms - last_run_time) > run_interval)
{
time_record.last_stop = time_ms ;
// start code body

// Stopping
if ( (ecu_data.running || ecu_data.starting) && !ecu_data.user_enable && !ecu_data.stopped )
{
time_record.last_stopping = time_ms ;

ecu_data.stopping = 1 ;
ecu_data.running  = 0 ;
ecu_data.starting = 0 ;
}

if (ecu_data.stopping)
{
// Ramp torque down
if (torque_target >= 200)
{
torque_target -= 20 ;
}
else // stop!
{

torque_target  = 0 ;
ecu_data.stopping = 0 ;
ecu_data.stopped  = 1 ;
}
}

server_objects[TARGET_TORQUE].value[1] = (int32_t) (torque_target * direction * -1) ;
server_objects[TARGET_TORQUE].value[2] = (int32_t) (torque_target * direction     ) ;

// end code body
}
}



void belt_slip_detection(uint16_t run_interval , uint16_t slip_detection_grace , float belt_slip_factor)
{
static uint32_t last_run_time = 0 ;

if (ENABLE_SLIP_DETECTION && ENABLE_TACHO_OUTPUT && ((time_ms - last_run_time) > run_interval))
{
last_run_time = time_ms ;
// code body

static int16_t belt_slip_time   = 0 ;

uint16_t grace_step = slip_detection_grace / run_interval ;

if (!grace_step) grace_step = 1 ;

if ((( (float) ecu_data.cable_speed / (float)(speed_actual / mpm_to_rpm) ) < belt_slip_factor)
&& ecu_data.running )
{
belt_slip_time += run_interval ;

if (belt_slip_time > slip_detection_grace)
{
ecu_data.belt_slip = 1 ;
ecu_data.starting = 0 ;
ecu_data.running = 0 ;
ecu_data.stopping = 0 ;
ecu_data.stopped = 1 ;
ecu_data.user_enable = 0 ;
torque_target = 0 ;

belt_slip_time = 0 ;
}
}
else if (belt_slip_time >= grace_step)
{
belt_slip_time -= grace_step ;
}
}
}


//const float stall_belt_speed_factor  = 0.5 ;
void cable_stall_detection(uint16_t run_interval , int16_t min_cable_speed)
{
static uint32_t last_run_time = 0 ;

if (ENABLE_STALL_DETECTION && ENABLE_TACHO_OUTPUT && (time_ms - last_run_time) > run_interval)
{
last_run_time = time_ms ;
// code body

if ((ecu_data.cable_speed < min_cable_speed) && ecu_data.running && torque_target > 990 )
{
ecu_data.cable_stall = 1 ;
ecu_data.starting = 0 ;
ecu_data.running = 0 ;
ecu_data.stopping = 0 ;
ecu_data.stopped = 1 ;
ecu_data.user_enable = 0 ;
torque_target = 0 ;
}
}
}


[tggzzz]

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 ?

Making it static doesn't solve it: it hides it. Disturbing any other piece of the code/compiler could unhide it.

As others have said, it strongly looks like your code isn't stating what you think it is stating. That contention is supported by this statement of yours:

OK so the original function I copied off the net used "char" or "int"  (can't remember), my compiler complained, so I changed to uint8_t to shut it up

[brucehoult]

Using uint8_t also makes the code less efficient because on a 32 bit machine it has to add (at least) an extra `uxtb` (or `and #0xFF` etc) to the loop.

What does this mean? I'm interested. Are you saying that 16 bits is the smallest a 32 bit (ARM) machine will work with anyway? I am aware that an ARM0+ has the ability to transfer two peices af data on it's 32 bit bus at once due to the thumb instruction set (and that is as clever as I get) so I tend to not oversize variables where practical.

No, 32 bits is the smallest size 32 bit Arm machines (or MIPS, RISC-V, Power, SPARC etc) have arithmetic operations for.

If you specify that some variable can only have values from 0 to 255, and then you add 1 to it, then the 32 bit add (the only kind it has) can result in an answer of 256, and extra work is needed to check for that and convert overflowed values back into range.

[eutectique]

Compilers are rarely broken. [...] As we never get any code, it isn't very helpful to reply to these threads.

Agree. That was me attempting to instigate the OP to post a minimal piece of code showing what is included where, declared where, and defined where. Anything at all.

[eutectique]

These are the compiler options I don't know if I need the first two, for a micro controller application they sound like trouble.

If you are talking about -ffunction-sections and -fdata-sections, they are extremely useful for MCUs with limited resources.

[Simon]

These are the compiler options I don't know if I need the first two, for a micro controller application they sound like trouble.

If you are talking about -ffunction-sections and -fdata-sections, they are extremely useful for MCUs with limited resources.
(Attachment Link)

So they eliminate dead code. Well all of my code is bare metal written by me. So are we talking about actual statements in my code that are deemed unnecessary or is this cleaning up code generated by the compiler? Basically will try to optimize out things?

What effect does the debug type of compile have? this was set. I have now changed it to release.

[Andy Watson]

Code: [Select]


void motor_control(uint16_t timeout)
{
static volatile uint32_t last_run = 0 ;


This line does not make sense. Declaring last_run as volatile does not make it global. Variables defined within a function are local to that function.

[Simon]

Code: [Select]


void motor_control(uint16_t timeout)
{
static volatile uint32_t last_run = 0 ;


This line does not make sense. Declaring last_run as volatile does not make it global. Variables defined within a function are local to that function.

Correct, that variable is static to that function, every function has the exact same variable used to determine after how many ms is should next run. time_ms is incremented by one every ms from the systic timer.

I made it volatile probably unnecessarily to ensure it was not optimized out.

[Simon]

Using uint8_t also makes the code less efficient because on a 32 bit machine it has to add (at least) an extra `uxtb` (or `and #0xFF` etc) to the loop.

What does this mean? I'm interested. Are you saying that 16 bits is the smallest a 32 bit (ARM) machine will work with anyway? I am aware that an ARM0+ has the ability to transfer two peices af data on it's 32 bit bus at once due to the thumb instruction set (and that is as clever as I get) so I tend to not oversize variables where practical.

No, 32 bits is the smallest size 32 bit Arm machines (or MIPS, RISC-V, Power, SPARC etc) have arithmetic operations for.

If you specify that some variable can only have values from 0 to 255, and then you add 1 to it, then the 32 bit add (the only kind it has) can result in an answer of 256, and extra work is needed to check for that and convert overflowed values back into range.

You mean performance I may as well use int32_t throughout unless I have concerns about RAM usage.

[eutectique]

So they eliminate dead code. Well all of my code is bare metal written by me. So are we talking about actual statements in my code that are deemed unnecessary or is this cleaning up code generated by the compiler? Basically will try to optimize out things?

I am pretty sure you love your hand-crafted well-thought-out code (who doesn't!) and would despise anyone who dares to call it "dead code"

Normally, the compiler generates the single .text section for code (and constants, does not matter now) and the single .data section for variables. The linker then places these sections as needed by the steering file. The consequence -- everything which is a function or a variable, will end up in your RAM/ROM on your MCU. Think of STM32 HAL, for example. You only use SPI, and still several kilobytes of flash are wasted for 64-bit division function calculating UART baud rate.

-ffunction-sections instructs the compiler to create a separate section for each function, say .text.main, .text.this, .text.that ...  Similarly, -fdata-sections places each variable into its own section. Then comes the linker, with its --gc-sections option. If a function or a variable is not referenced -- it is "dead code" and does not deserve the place in the binary.

What effect does the debug type of compile have? this was set. I have now changed it to release.

No effect whatsoever. Debug information, optimisation, and dead-code elimination are orthogonal concepts. They are controlled by separate options.

[Simon]

I am pretty sure you love your hand-crafted well-thought-out code (who doesn't!) and would despise anyone who dares to call it "dead code"

Well it's not a case of loving my code, I try to write efficiently as I don't want the compiler to have to do my job. What I am saying is, are these options likely to lead to any issues? You know like increasing the optimization settings do sometimes.

[eutectique]

To the best of my knowledge and experience, -ffunction-sections option works on per-function basis -- a function either makes its way into the final binary, or not. The option does not clip any code from a function itself. It does not re-arrange if() or while() blocks, or similar.

[Simon]

Well i have been using them all afternoon with no issue.

[gf]

To the best of my knowledge and experience, -ffunction-sections option works on per-function basis -- a function either makes its way into the final binary, or not. The option does not clip any code from a function itself. It does not re-arrange if() or while() blocks, or similar.

Yes, with --gc-sections, the linker discards only complete sections of a .o file, and only if none of the symbols exported from that section are referenced. Dead (unreachable) code inside functions is eliminated by the compiler.

[Siwastaja]

Well it's not a case of loving my code, I try to write efficiently as I don't want the compiler to have to do my job. What I am saying is, are these options likely to lead to any issues? You know like increasing the optimization settings do sometimes.

Neither using these options, or using higher optimization levels, is causing your problems, but you barking at the wrong tree is preventing you from seeing the problems, which are solely in your code, and can only be solved by fixing your code. Compiler and compiler options are all fine, do not touch them.

You should be working hard to find the issue WITHIN YOUR CODE and YOUR CODE ALONE. If you feel like being stuck and getting nowhere, then:
1) SIMPLIFY,
2) REFACTOR, or maybe even
3) START OVER

Others have asked you to post minimum example reproducing your problem. You think it's extra work, but it isn't. It forces you to investigate.

-ffunction-sections and -gc-sections are very useful even if you wrote everything in the codebase. You don't want to be manually commenting out functions you wrote but do not (currently) use to save flash. Let the compiler+linker automate it for you. There are no downsides.

[Simon]

Attached is the entire project.

it is equivalent to about 4% of the bible! Yes I have been going through this code and refining for the last 2 months as part of the development process.

I am not saying that any optimization will be a problem. But as you say my code has an issue and I am trying to move my project forward as I also discover issues. I don't want to hit a brick wall due to some optimization and go round in circles, it is easier to work on working code.

[Siwastaja]

I don't want to hit a brick wall due to some optimization

You are not hitting a wall due to compiler optimization. Compiler is doing exactly what you tell it to do. There is no way doing random changes (e.g., adding random keywords to code, or making random changes to compiler settings) would be helpful at all. This is exactly why you are going "in circles". Random changes sometimes seem to help, but problems tend to come back and such changes guide you to wrong conclusions.

You should stop worrying about compiler settings, and use that time to work with the code instead. Strip parts out of it; remove features (e.g., by commenting code out) to reduce number of lines, until you are down to the smallest possible project which still shows the problem. Then you can go through you code line by line, and compare against C standard (or a good book, or even Stack Overflow discussions would be better than nothing) to see if your expectations of what you think it does matches with what it actually does.

[Siwastaja]

Also, while we are focusing on the usage of volatile qualifier and thinking about possible race conditions and failures to do atomic accesses, let's not forget good old memory corruption due to overindexing an array or miscalculating pointers or data sizes (e.g. in memcpy call or similar).

If torque_target is corrupted, then list the symbols in .data and .bss using:
arm-none-eabi-objdump -t firmware.elf | grep "\.data\|\.bss"
objdump sorts by address, so look at variables close (usually before for overindexing) to torque_target. Maybe some buffer which you were unsure about pops up there.

But reducing the code by stripping features is very effective because at some point, you are likely to remove the part that is causing memory corruption. As a side effect, you are creating a minimum example which demonstrates the problem, so even if you finally still need help, people can actually help you without spending weeks looking at your whole project.

The problem is this: posting small excerpts where you think the problem is does not help because the problem likely isn't there. And if you post the whole project, no one has time to go though it. Reduction is thus the key.

[Simon]

it is complicated as 2 months of CAN Open code are required to run anything. The vast majority of the code (120'000 chars when I last looked) are simply to get me to a point where I can run a motor.

Alternatively I need to find a way to simulate the issue, at that point though when I substantially change the code the problem may will magically go away but I will still be none the wiser.

I have "The C programming language", it's a brief book. I don't consider it to describe all aspects. The most infuriating thing I find is that all books don't explain how a feature works, the talk about an example that uses it, they get you so far but if there is information out of the scope of that particular example you don't get the information.

One of my most useful books (I have many) is pocket size, it is so useful as it just tells you straight what the syntax is without using long winded examples that you have to infer from.

Is the C standard publicly available?

[Siwastaja]

I can see you have lot of functions where you access an array using a function argument or variable coming from somewhere, without checking the argument. Maybe you are very careful when you are calling such functions, but I strongly suggest you always do runtime checks, it costs a tiny bit of CPU time and flash memory but will save a day.

E.g.

Code: [Select]

void can_exd_filter_setup(Can * instance , uint32_t id , uint8_t filter_element_n , void * variable )
{
can_0_rx_variable_index[filter_element_n + CAN_0_STD_MESSAGE_FILTER_NUM ] = variable ;

Modify to:

Code: [Select]


void panic_fail(int errcode)
{
    // simple error handler for the whole project.
    // print errcode to uart, blink led errcode times, beep a sound, whatever.
}

void can_exd_filter_setup(Can * instance , uint32_t id , uint8_t filter_element_n , void * variable )
{
                if(filter_element_n >= N_ELEM(can_0_rx_variable_index))
                    panic_fail(42);
can_0_rx_variable_index[filter_element_n + CAN_0_STD_MESSAGE_FILTER_NUM ] = variable ;


where N_ELEM() is the classic macro
#define N_ELEM(a) (sizeof (a) / sizeof (a)[0])

[Siwastaja]

it is complicated as 2 months of CAN Open code are required to run anything. The vast majority of the code (120'000 chars when I last looked) are simply to get me to a point where I can run a motor.

Of course this isn't true. Of course you can strip the CAN part out completely and just set the motor control variables manually, from code. Is UI giving a command? Just give that command from the beginning of main() instead. Or after a delay. Now is torque_target still behaving odd? Do you need belt slip detection to run the motor? Probably not, remove, comment out the code completely. And so on and so on. You can always reduce.

[Siwastaja]

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, and if you don't test hard enough, or with extremely good luck, the bug can appear completely fixed; but truly isn't. Therefore by doing such changes, you are wasting time.

[Simon]

OK, think I have a copy.

[golden_labels]

Is C++ better in this respect? I have been trying to get started in it for a long time and it's a natural step.

In that regard: no. It retains all the complexity of C and adds its own.

The variable is only defined in motor.c as "volatile int32_t nominal_current".

A function defined in motor.c and declared in motor.h has a line that says:

nominal_current = 28000 ;

The function which sets up other variables too that are possibly also being optimized out but not causing an issue is called in main() before while(1){main program loop}.

A function called control() defined in motor.c and declared in motor.h uses nominal_current in 4 calculations and is called in the while(1) loop in main()

So if I pause after the function has run to set the variable up it has a value of 28000, if I pause in the control() function it's value is now "0".

This means the nominal_current variable is actually not initialized. And then you assign to it 28000. But that assignment may be not atomic, so it’s possible control sees garbage, 0, 28000, or any unexpected value arising from the assignment not being atomic.

volatile imposes an order of operations accessing volatile variables. But doesn’t guarantee operation’s atomicity or visibility, and in particular the expected order may not be seen in multiple threads of execution. Coincidentally this trap is present not only in C or C++, but may also appear in other languages with a similar feature. The definition and behavior or volatile is so muddy, misleading and unreliable, that in fact it’s discouraged in multithreaded code and to be replaced with platform-specific threading features. Not saying you should not use it; but each time you feel the temptation: think thrice.

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.⁽¹⁾ 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.

The reason for “minimal code reproducing the issue” is not only reader’s convenience, but — more importantly — because in the process of shrinking down the code the culprit is being found and understood.

⁽¹⁾ Modern C also has constexpr, BTW. It’s how one makes “true” constants in C.

[Simon]

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.

[Simon]

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.⁽¹⁾ 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.

Well I was happy to leave it to the compiler to optimise it, really it should have been a constant.

[Simon]

In that regard: no. It retains all the complexity of C and adds its own.

well that was my assumption. So I have continued with C and put C++ on the back burner as i can probably work without it and have much more important things to get into like linux programming at which point i will learn the background that i am missing now.

[Siwastaja]

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 become

Code: [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.

[Andy Watson]

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__

[Simon]

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 become

Code: [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.

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. but then given the time it takes to find the correct array element I am probably saving no time. I am up to 63% of my 16kB RAM so I'm not too precious about it but yes I may as well have those created on the fly, the same applies to another function.

number_bytes is broadly the number of byes but 3 MSb are used as flags. As you correctly guessed bit 7 is to indicate a signed variable.

So if you look in canopen_dictionary.h you will find a structure typdef, this is the building block of the dictionary, there is an array of these structures. The structure for each dictionary element holds the index and subindex of the can open system. The type variable specifies the number of bytes, whether it is signed, if it is a read only variable and if it is to be sent as part of the initialization of the can open device(s).

This little setup allows the data to be handled in multiple ways. Yes the int32 for both 0x04 and 0x84 is correct. Somewhere there is a thread I started where I presented my reasoning.

Basically it makes sense to have the data in one array, but the data can be any of the 6 types. Initially I experimented with an array of pointers to the various types of variables but this was not the best either and in any case I wound still need to use 32 bits of storage for the pointers. So I reasoned that each variable shall be stored as an int32 as this will store anything the other 5 types store if the uint32 does not exceed 2^31 for positive numbers. I am yet to find a number in the CAN open devices I am using that exceeds this. In fact interestingly one of the objects I use is stated in the manual to be a uint32 with a maximum number of 2^31, I think the person who programmed these devices also followed my logic.

[Simon]

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__

This data is not being messed about with by pointers. I use pointers to the array element but I don't tend to point into the structure.

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.

[Siwastaja]

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

More accurately - aligns each member to its own natural alignment requirement (that would imply word alignment only for word-wide members). In any case, padding should not affect memcpy when used in sane way: always use sizeof struct. But indeed, if one goes through extra hassle of adding up member sizes manually and using that as n for memcpy, things fail because of padding.

Utilize sizeof, instead of magic numbers, makes your life much easier.

[Simon]

I always pad structures out to align manually, habit of that first time I used a structure to hold can bus data and had to know where the data was so manual alignment was the only option. The person that wanted to use the pack thing just made it into a bit soup as we no longer knew where any of the data was which does not work when you communicate between devices.

[ejeffrey]

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 ?

Oh my god stop this.  Randomly permuting the code to see what makes it "work" according to your expectations is a recipe for disaster. 

Instead, do this (this advice is specific for you, not an approach that should be taken 100% by everyone):

First: assume the compiler is correct.  It's definitely true that compilers have bugs, but the vast, vast majority of problems are user errors, not compiler bugs.  So get that out of your head, and also get out that the compiler is "not doing what you tell it" -- this is basically equivalent to calling it a compiler bug.  The compiler always does what you tell it, often people just don't know what they are telling it.  With your mindset fixed, follow this procedure:

Comment out all casts and all uses of volatile in the entire code base.  Compile with -Wall -Werror.   Maybe -Wextra for good measure. Keep *fixing* bugs until your code compiles -- not working around, but actually fixing.  If you absolutely have to add back in a cast, write a comment about why it is correct, and ask an expert if there is a way around it.  If they say yes, even if it includes changing a bunch of code, do it.  volatile should *only ever* be used for hardware registers and global variables accessed by interrupt handlers.  All other users should not be allowed under any circumstances.  Then if you can possibly run your code with valgrind, ubsan or any other kind of memory testing.  This is usually hard on embedded systems, but if you can run in an emulator or cross compile for a PC, even for parts of the code do so, and keep *fixing* bugs until those run without complaint.

Now you have code that is reasonably likely to have well defined behavior.  It might not be correct, but it is code worth debugging.  Code that changes behavior when you make meaningless changes is not worth debugging.  My recommendation then is to now skip directly to -O2.  When you find a bug, then switch back to -Og or -O0 to debug it.  The reason to do this is mostly that it will be easier to understand what is going on in an interactive debugger.  It doesn't matter what debugging tool you use: gdb, printf, toggling GPIO pins, or likely a combination of all three and more. What is good is that all of the work you did above will make it much less likely that random permutations of your code during debugging will cause the issue to go away.  This is good: ultimately predictable systems are easy to debug and unpredictable ones difficult.  What you have gained by following these steps is both made your code more predictable (in that it avoids undefined or many types of surprising behavior) and you have improved your ability to predict the C language.  Starting with -O2 helps you because you never get to a situation of having a program that works with a debug build and you are afraid to turn on optimizations because it might break.  In addition, if the behavior changes when you turn off optimizations, then you have already determined that either the problem is undefined behavior or a timing error.  That means you are already well on your way to diagnosing and fixing it.

But the #1 piece of advice I would distill from this is: use volatile only for hardware registers and global variables used to communicate between interrupt handlers (or async signal handlers) and the main thread or other normal threads.  Any other use is guaranteed incorrect.

[langwadt]

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.

setting them as static likely just wastes memory and the cycles to restore and store them in memory, when it isn't needed and the compiler would likely just use a register instead if it wasn't static

[IanB]

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.

It may be that the opposite is true. For the compiler, an automatic variable is "normal" and a static variable is "special". The compiler has to do extra work to allocate, preserve and access a static variable. An automatic variable may take no work at all. Either the compiler will just place it in a spare register, or if not it will just increment the stack pointer to make a slot for it on the stack. The cost of incrementing and decrementing the stack pointer is trivial.

[SiliconWizard]

A *local* variable that is qualified static is basically just like a static global variable, except that it's only visible locally (so, only the visibility differs).

[ejeffrey]

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.

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.  So the easiest way to get the data into a format that you can use more flexibly is to make a non-packed version of the structure and do element-by-element assignment from one to the other.  The compiler should warn you about misusing packed structures unless you silence those with an explicit cast, so don't do that.  I'm not sure exactly what you want to do with "cannot use pointers to copy data to a structure variable" -- that might or might not be OK depending on what you mean. 

[ejeffrey]

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.

[SiliconWizard]

Also, the only way you can ensure a specific layout of global variables in memory is to write an appropriate linker script, if said variables are distinct.
But the very simple way not requiring that would be to define those "variables" that need specific ordering in a global struct, which guarantees the ordering of its members in C. And then, you can either rely on the alignment ensured by the ABI, or specify one with alignas directives on the struct members (or a 'packed' attribute for the structure, which is not standard C, but supported both by GCC and Clang.) alignas, OTOH, is standard C11. But while alignas guarantees a certain alignment, the compiler is free to use a larger alignment than what's specified (which would still comply), so not a failsafe way of forcing specific 'offsets' for struct members.

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

[Simon]

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.

The idea is that no matter how many messages are needed to send the data you will always have a single structure for all data sent and one for all data received. I am not taking about can open type data here but limited information that is exchanged between two devices for example the speed demand from a HMI and the actual speed sent to the HMI. We ended up with up to 24 bytes of this sort of stuff and to avoid struct1. struct2. struct3. the simplest way is to lay the data out manually and know where it is. Then you know exactly what you send in each message (J1939 style) and the program is easier to manage.

[DiTBho]

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

That's the case for { Sierra-C, Avocet-C, Panasonic-SH-C, ... }
(talking about DOS-v5, Windows-3.*, Linux-v2.* era, so old tools, but still in use ...)

They don't support gcc-attributes, and they don't even support #pragma pack.
so, my solution was/is:
define structs in assembly, as "module" (.S -> .o) to yield the exact layout you want
define symbols in a header file (extern)
make good use, every C-compiler will adapt

[gf]

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.

You missed the point: Assigning the address of a member of type T inside a packed structure object to a T* pointer is problematic. The pointer value may not be properly aligned for a subsequent access to the member if alignof(T) > 1.

[DiTBho]

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.

Yup, ABI-spec rules everything.

[Simon]

so if I turn pedantic warnings on to try and weed stuff out I get errors likes:

Warning      ISO C forbids conversion of function pointer to object pointer type [-Wpedantic]   SAMC21J starter project

But this is in the micro controller header files so very much not an issue or at least not one I can deal with.

what version of C should I be using? it seems to be set to: -std=gnu99

[newbrain]

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.

I know I'm late, and the thread has moved on, but my inner pedant was itching since I saw this yesterday...

In C99 through C23, the exact width integer types (intN_t, uintN_t) are defined to have no padding bits.
So, if the byte (i.e. char, the minimum amount of memory) is larger than 8 bits, int8_t and uint8_t types cannot exist.

Conversely they must exist if the machine can provide them - still, with no padding, and since C11 with 2's complement representation.

Ref: 7.[18,20,22].1.1 Exact-width integer types, respectively in C99, C11 and C23 (draft).
From C11:

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.

[Siwastaja]

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.

Really, just compile with -Wall -Werror. Taking pointer of packet struct which might produce unaligned access generates compiler warning. At least usually.

[IanB]

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.

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?

[Siwastaja]

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?

Correct, that works fine, because compiler sees type of structPtr, and as such, knows it's packed and generates the correct instructions to access unaligned data.

Safe:

Code: [Select]

typedef struct __attribute__((packed))
{
   uint8_t a;
   uint32_t b;
} s_t;

s_t s;
...
s_t* p_s = &s;
...
p_s->b = 420;

Not safe:

Code: [Select]

typedef struct __attribute__((packed))
{
   uint8_t a;
   uint32_t b;
} s_t;

s_t s;

uint32_t * p_b = &s.b;
*p_b = 420;


[Simon]

   mpm_to_rpm   0x20002768   const int32_t(static storage at address 0x0.)

This is from the watch window regarding a variable called "mpm_to_rpm" do i read correctly that it is located at address 0 ? I know enough to know that that is bad!

[ejeffrey]

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.

That's why I specifically said you can use memcpy on the entire structure.  Generating a pointer to a sub-element is where you get into trouble.  But you can just use assignment to copy elements in and out.  You actually can use memcpy, which is guaranteed to work with any alignment and works with byte alignment, you just need to carefully generate the pointer to it using the offsetof operator.

Code: [Select]

struct mystruct {
    int16_t a;
    int32_t b[4];
} __attribute__(packed);

void copy_b_to_dest(struct mystruct *foo, int *dest)
{
    char *raw_buf = foo;
    memcpy(dest, raw_buf + offsetof(struct mystruct, b), sizeof(foo->b));
}

Note I haven't compiled this code, it may be buggy and a bit of a silly example.  It's the idea that I'm trying to show how to use memcpy to get stuff out of a packed structure without having alignment problems.

[Andy Watson]

   mpm_to_rpm   0x20002768   const int32_t(static storage at address 0x0.)

This is from the watch window regarding a variable called "mpm_to_rpm" do i read correctly that it is located at address 0 ?

Probably compiler/debugger short-hand for saying that mpm_to_rpm does not require storage space. Its value is completely resolved and can be used directly at compile time - it doesn't need to be stored - at least not in data space.

Defined here at line 26 in motor_control.c:

Code: [Select]

const int16_t mpm_to_rpm               = 38  ; // if changed update min_motor_rpm
it tells the compiler all it needs to know to use the value 38 in the subsequent five references.

[Siwastaja]

you can just use assignment to copy elements in and out.

Which is, btw, what you should be doing most of the time. And not only elements - assigning structs, too.

People overuse stuff like memcpy, memset etc. ignoring the fact that C is high level language.

You can do nice things like copy a struct into another struct of the same type by assignment, clear struct by assigning {0} to it, return a struct from a function call (this is how you can return more stuff than "single variable"(!), pass struct to a function by value, not by pointer...

... and when you do this, many stupid mistakes simply go away, and C becomes a high-level language. These are features people obviously expect from other languages, yet somehow think they don't exist in C. It's weird.

Also don't forget unions. And combinations of unions and structs. You can access the same part of memory in many different ways by using unions. And the access is always correct, and mistakes which are common with memcpy and sizeof become impossible.

When you do that, it pretty much leaves you classical overindexing of array. For which you can develop a habit of always checking before accessing, and using the classic N_ELEMENTS macro I posted above, to further reduce mistakes.

[Simon]

   mpm_to_rpm   0x20002768   const int32_t(static storage at address 0x0.)

This is from the watch window regarding a variable called "mpm_to_rpm" do i read correctly that it is located at address 0 ?

Probably compiler/debugger short-hand for saying that mpm_to_rpm does not require storage space. Its value is completely resolved and can be used directly at compile time - it doesn't need to be stored - at least not in data space.

Defined here at line 26 in motor_control.c:

Code: [Select]

const int16_t mpm_to_rpm               = 38  ; // if changed update min_motor_rpm
it tells the compiler all it needs to know to use the value 38 in the subsequent five references.

Right, so the debugger tries to tell me that the value is effectively optimized (which is what const is for) by telling me that it just put that value at an address it should not be at and with a value that is totally not what I set it too which leaves me wasting hours diagnosing a thing that is not there.

If I take const off again I get the correct value at a RAM address which is what I believe the const "value" above is.

[IanB]

   mpm_to_rpm   0x20002768   const int32_t(static storage at address 0x0.)

This is from the watch window regarding a variable called "mpm_to_rpm" do i read correctly that it is located at address 0 ?

Probably compiler/debugger short-hand for saying that mpm_to_rpm does not require storage space. Its value is completely resolved and can be used directly at compile time - it doesn't need to be stored - at least not in data space.

Defined here at line 26 in motor_control.c:

Code: [Select]

const int16_t mpm_to_rpm               = 38  ; // if changed update min_motor_rpm
it tells the compiler all it needs to know to use the value 38 in the subsequent five references.

Right, so the debugger tries to tell me that the value is effectively optimized (which is what const is for) by telling me that it just put that value at an address it should not be at and with a value that is totally not what I set it too which leaves me wasting hours diagnosing a thing that is not there.

If I take const off again I get the correct value at a RAM address which is what I believe the const "value" above is.

Every debugger I have ever used would tell me that "mpm_to_rpm" is of type int16_t and has a value of 38 decimal.

I do not quite understand where that information you showed came from?

[Simon]

you can just use assignment to copy elements in and out.

Which is, btw, what you should be doing most of the time. And not only elements - assigning structs, too.

People overuse stuff like memcpy, memset etc. ignoring the fact that C is high level language.

You can do nice things like copy a struct into another struct of the same type by assignment, clear struct by assigning {0} to it, return a struct from a function call (this is how you can return more stuff than "single variable"(!), pass struct to a function by value, not by pointer...

... and when you do this, many stupid mistakes simply go away, and C becomes a high-level language. These are features people obviously expect from other languages, yet somehow think they don't exist in C. It's weird.

Also don't forget unions. And combinations of unions and structs. You can access the same part of memory in many different ways by using unions. And the access is always correct, and mistakes which are common with memcpy and sizeof become impossible.

When you do that, it pretty much leaves you classical overindexing of array. For which you can develop a habit of always checking before accessing, and using the classic N_ELEMENTS macro I posted above, to further reduce mistakes.

I only use memcpy where I need to transfer data without casting it. In this program it is used only for handling the data in and out of the CAN system which is why I set up a library of int32 value to simply use throughout the program.

With any structure that I use memcpy on I can't pack it even if I use the offset as you say because the data must arrive at the other end in the same order so that two totally different devices running different programs and are different architectures can communicate.

So take any of my structures, I have 24 bytes to send they are sent in 3 messages of 8 bytes each. each message has a different ID (J1939 / PDO style), this is so that the HMI unit knows what the data is by the message ID and the location in each message of the data. Now if I pack that data and it moves it's game over. If I use the offset to variables in the structure I essentially end up with a custom peice of code for each and every one of my 3 messages and memcpy is really only being used because I am copying into single bytes variables that could have 4 bytes. Same on reception.

The only way around this is an array of variables so that they are all the same type. This is the same problem as the micro controller registers and the chip manufacturers have no problems with the bit fields staying in the right place.

[Simon]

   mpm_to_rpm   0x20002768   const int32_t(static storage at address 0x0.)

This is from the watch window regarding a variable called "mpm_to_rpm" do i read correctly that it is located at address 0 ?

Probably compiler/debugger short-hand for saying that mpm_to_rpm does not require storage space. Its value is completely resolved and can be used directly at compile time - it doesn't need to be stored - at least not in data space.

Defined here at line 26 in motor_control.c:

Code: [Select]

const int16_t mpm_to_rpm               = 38  ; // if changed update min_motor_rpm
it tells the compiler all it needs to know to use the value 38 in the subsequent five references.

Right, so the debugger tries to tell me that the value is effectively optimized (which is what const is for) by telling me that it just put that value at an address it should not be at and with a value that is totally not what I set it too which leaves me wasting hours diagnosing a thing that is not there.

If I take const off again I get the correct value at a RAM address which is what I believe the const "value" above is.

Every debugger I have ever used would tell me that "mpm_to_rpm" is of type int16_t and has a value of 38 decimal.

I do not quite understand where that information you showed came from?

that line is copied and pasted from the microchip studio watch1 window. I'm going to try and convert over to mplabx as I have other issues with studio like it remembering any file ever opened so that if you take a copy of your project and reopen it, it will look at the files in the other copy unless you rename the folder of the previous copy.

[Mechatrommer]

...pass struct to a function by value, not by pointer...

passing struct by pointer has its place for performance reason.. ymmv..

[IanB]

I only use memcpy where I need to transfer data without casting it. In this program it is used only for handling the data in and out of the CAN system which is why I set up a library of int32 value to simply use throughout the program.

With any structure that I use memcpy on I can't pack it even if I use the offset as you say because the data must arrive at the other end in the same order so that two totally different devices running different programs and are different architectures can communicate.

So take any of my structures, I have 24 bytes to send they are sent in 3 messages of 8 bytes each. each message has a different ID (J1939 / PDO style), this is so that the HMI unit knows what the data is by the message ID and the location in each message of the data. Now if I pack that data and it moves it's game over. If I use the offset to variables in the structure I essentially end up with a custom peice of code for each and every one of my 3 messages and memcpy is really only being used because I am copying into single bytes variables that could have 4 bytes. Same on reception.

The only way around this is an array of variables so that they are all the same type. This is the same problem as the micro controller registers and the chip manufacturers have no problems with the bit fields staying in the right place.

I suggest the best way to deal with this is to have a send buffer and a receive buffer, and have pack and unpack routines to put the data in and out of the buffer. The pack and unpack routines would explicitly handle the bit sequence of the data in the payload, take care of endianness as required, and basically would make sure to fill and empty a buffer exactly according to the specifications of the communication protocol.

You are then free to define structs in your code for message content in any natural way, with no worries about packing or alignment, and just have the pack and unpack routines put data in and out of the structs as needed.

This way you will have abstracted the I/O layer into a hardware specific interface, and the rest of your code can be arranged neatly without knowing about the hardware it is running on.

An additional benefit is that you can mock that I/O abstraction layer for testing, and you can compile and test the rest of your code on a platform of your choice like Windows or Unix, and debug the logic using a nice high level IDE with friendly debugging tools.

[Andy Watson]

Right, so the debugger tries to tell me that the value is effectively optimized (which is what const is for) by telling me that it just put that value at an address it should not be at and with a value that is totally not what I set it to.

Not quite. The constant is baked-in to the code at compile time. The value must exist somewhere beacuse a) it is used (5 times), and b) you can create a pointer to it and read back the value ¹. I've just tried writing to a const value, via a pointer (on PC - i686) and, not surprisingly, it seg-faults because the pointer is (I guess) pointing to code memory.

If I take const off again I get the correct value at a RAM address which is what I believe the const "value" above is.

Yes, because you've changed mem_to_rpm into a variable.


1.- I don't know how the value is read-back via a pointer - perhaps the compiler has enough intelligence to be able to resolve a level of indirection?

[tggzzz]

Greybeards remember the truth in an old saying: "cc is half a compiler, the other half is lint". In other words, use lint to detect the fluffs in your code, then remove the fluffs.

Still true, even if lint has been superseded by -Wall and -Wreallyreallyall etc

[DiTBho]

Greybeards remember the truth in an old saying: "cc is half a compiler, the other half is lint". In other words, use lint to detect the fluffs in your code, then remove the fluffs.

Still true, even if lint has been superseded by -Wall and -Wreallyreallyall etc

 

[Perkele]

Still true, even if lint has been superseded by -Wall and -Wreallyreallyall etc

Begin rant.
We're still not there. I'm paying annually four times more for a static analyser than for IDE + compiler suite.
When selecting a static analyser, I evaluated about 12 of them for use with plain C code.
C99 or C11, nothing more, nothing weird, standard code.
Half were completely useless, some of really expensive ones had results worse than splint.
Splint was obsoleted 15 years ago and can't even parse C99 code properly.
End rant.

[tggzzz]

Still true, even if lint has been superseded by -Wall and -Wreallyreallyall etc

Begin rant.
We're still not there. I'm paying annually four times more for a static analyser than for IDE + compiler suite.
When selecting a static analyser, I evaluated about 12 of them for use with plain C code.
C99 or C11, nothing more, nothing weird, standard code.
Half were completely useless, some of really expensive ones had results worse than splint.
Splint was obsoleted 15 years ago and can't even parse C99 code properly.
End rant.

There are many products/processes designed to stabilise sand, e.g. freezing it, injecting it with concrete, driving piles, etc.

But it is better to choose not to build castles on sand.

[Simon]

I only use memcpy where I need to transfer data without casting it. In this program it is used only for handling the data in and out of the CAN system which is why I set up a library of int32 value to simply use throughout the program.

With any structure that I use memcpy on I can't pack it even if I use the offset as you say because the data must arrive at the other end in the same order so that two totally different devices running different programs and are different architectures can communicate.

So take any of my structures, I have 24 bytes to send they are sent in 3 messages of 8 bytes each. each message has a different ID (J1939 / PDO style), this is so that the HMI unit knows what the data is by the message ID and the location in each message of the data. Now if I pack that data and it moves it's game over. If I use the offset to variables in the structure I essentially end up with a custom peice of code for each and every one of my 3 messages and memcpy is really only being used because I am copying into single bytes variables that could have 4 bytes. Same on reception.

The only way around this is an array of variables so that they are all the same type. This is the same problem as the micro controller registers and the chip manufacturers have no problems with the bit fields staying in the right place.

I suggest the best way to deal with this is to have a send buffer and a receive buffer, and have pack and unpack routines to put the data in and out of the buffer. The pack and unpack routines would explicitly handle the bit sequence of the data in the payload, take care of endianness as required, and basically would make sure to fill and empty a buffer exactly according to the specifications of the communication protocol.

You are then free to define structs in your code for message content in any natural way, with no worries about packing or alignment, and just have the pack and unpack routines put data in and out of the structs as needed.

This way you will have abstracted the I/O layer into a hardware specific interface, and the rest of your code can be arranged neatly without knowing about the hardware it is running on.

An additional benefit is that you can mock that I/O abstraction layer for testing, and you can compile and test the rest of your code on a platform of your choice like Windows or Unix, and debug the logic using a nice high level IDE with friendly debugging tools.

You still don't understand, the data being sent could be: int8, uint8, int16, uint16, int32, uint32, bit fields. If you have many variables to send the only way to do this is work out a way where you data to send is stored consecutively so that you can copy it from a start address to an end address. Options that I am aware of are structs and arrays. every message will be potentially  unique in terms of the data. unless you want to create a message specific function for every message it will be a mess.

Again micro controller registers are mapped with structures, if it was not possible to write a structure and have the data appear in memory as written then lots of code will not work! but it does. So all this packed stuff is not essential if you write them properly!

[PlainName]

Options that I am aware of are structs and arrays

Perhaps unions would help with "unless you want to create a message specific function for every message".

http://www.jnkvv.org/PDF/25042020093559244201357.pdf

[IanB]

You still don't understand...

Telling me that I don't understand, when I have been writing software for 40-odd years, is not going to help you.

People have been sending data over the wire for a very long time. It's not a new thing. That's why the OSI model exists.

You may not need all of the layers in every case, but the concept is helpful. Abstraction is key.

And yes, different message types may need to be packed and unpacked differently. This is going to happen no matter how you approach it.

But, as the saying goes, "it's your funeral".

[Perkele]

Still true, even if lint has been superseded by -Wall and -Wreallyreallyall etc

Begin rant.
We're still not there. I'm paying annually four times more for a static analyser than for IDE + compiler suite.
When selecting a static analyser, I evaluated about 12 of them for use with plain C code.
C99 or C11, nothing more, nothing weird, standard code.
Half were completely useless, some of really expensive ones had results worse than splint.
Splint was obsoleted 15 years ago and can't even parse C99 code properly.
End rant.

There are many products/processes designed to stabilise sand, e.g. freezing it, injecting it with concrete, driving piles, etc.

But it is better to choose not to build castles on sand.

Time machines are not available to general population, regardless of project's budget.
Device in question was built a decade ago, and there's about 100k lines of code to maintain.

[SiliconWizard]

Greybeards remember the truth in an old saying: "cc is half a compiler, the other half is lint". In other words, use lint to detect the fluffs in your code, then remove the fluffs.

Still true, even if lint has been superseded by -Wall and -Wreallyreallyall etc

Yes, well, there are "many" static analyzers way better than lint ever was now, some "free", some very expensive. They'll "beat" any compiler's analysis, even Clang, which has already a much better static analyzer than GCC.
So definitely worth using them. Compilers' warnings are better than nothing, but definitely pretty basic in terms of analysis.

[tggzzz]

Still true, even if lint has been superseded by -Wall and -Wreallyreallyall etc

Begin rant.
We're still not there. I'm paying annually four times more for a static analyser than for IDE + compiler suite.
When selecting a static analyser, I evaluated about 12 of them for use with plain C code.
C99 or C11, nothing more, nothing weird, standard code.
Half were completely useless, some of really expensive ones had results worse than splint.
Splint was obsoleted 15 years ago and can't even parse C99 code properly.
End rant.

There are many products/processes designed to stabilise sand, e.g. freezing it, injecting it with concrete, driving piles, etc.

But it is better to choose not to build castles on sand.

Time machines are not available to general population, regardless of project's budget.
Device in question was built a decade ago, and there's about 100k lines of code to maintain.

Am I hallucinating, or does the Linux kernel still mandate C89?

Some long-lived projects are in the position of having to continue using an ~35yo language.

At least they avoid running into the trap of having a valid program that must never finish compiling - because compiling the program causes the compiler to emit the sequence of prime numbers as a side effect!

[SiliconWizard]

Am I hallucinating, or does the Linux kernel still mandate C89?

I haven't checked that, but it may be possible... if just for the fact that they still need to support platforms for which there may not be any C compiler supporting anything other that C89. (Which, if you read DiTBho's posts, wouldn't be so surprising.)

That would be pretty unfortunate indeed, as C99 has brought many nice features, and C11 even more so.

[golden_labels]

What Linux uses is currently GNU C11 (-std=gnu11 option to gcc).

[ejeffrey]

So take any of my structures, I have 24 bytes to send they are sent in 3 messages of 8 bytes each. each message has a different ID (J1939 / PDO style), this is so that the HMI unit knows what the data is by the message ID and the location in each message of the data. Now if I pack that data and it moves it's game over. If I use the offset to variables in the structure I essentially end up with a custom peice of code for each and every one of my 3 messages and memcpy is really only being used because I am copying into single bytes variables that could have 4 bytes. Same on reception.

This is incomprehensible without a code example.  Also three individual serialization routines for three different 24 byte messages is 15 minutes of work and you have been ranting about this for weeks, if not longer.  If that's really what this is about then that seems totally out of proportion.

[Simon]

So take any of my structures, I have 24 bytes to send they are sent in 3 messages of 8 bytes each. each message has a different ID (J1939 / PDO style), this is so that the HMI unit knows what the data is by the message ID and the location in each message of the data. Now if I pack that data and it moves it's game over. If I use the offset to variables in the structure I essentially end up with a custom peice of code for each and every one of my 3 messages and memcpy is really only being used because I am copying into single bytes variables that could have 4 bytes. Same on reception.

This is incomprehensible without a code example.  Also three individual serialization routines for three different 24 byte messages is 15 minutes of work and you have been ranting about this for weeks, if not longer.  If that's really what this is about then that seems totally out of proportion.

It's the principle! remember, you do it today because although not a scalable solution it's a small problem. Then in the future the problem scales, the solution cannot and you have to reinvent the wheel. Tell me, how do J1939 auto ECU's manage this problem? they have several hundred messages of no same format, you are telling me that someone wrote a message handler for each one? I may be inexperienced but I not stupid!

My point about the mapping of hardware registers is constantly being ignored. Fact! hardware registers are mapped into structs. No one has problems with the data being in the location it is supposed to be do they? They don't use pack.

You can either offer a proper answer and stop telling me I am ranting for weeks or continue to belittle me. I'll tell you something, every time I have an issue and come on here I get the same types of responses from  experienced people, and every time the answerer are different from last time. Everyone has an opinion and they treat it as fact. The language has been going for years and has been used in many situations, but no one seems to realize that the model of their little part of life with C is not the same as the one someone else may be in. I am sick of conflicting answers and people ranting about something else,

[IanB]

It's the principle! remember, you do it today because although not a scalable solution it's a small problem. Then in the future the problem scales, the solution cannot and you have to reinvent the wheel. Tell me, how do J1939 auto ECU's manage this problem? they have several hundred messages of no same format, you are telling me that someone wrote a message handler for each one?

In a sense, maybe yes.

In at the hardware level, on a message bus, each device will only pick out and process messages it is interested in. It will look at the PGN and SPN to decide this, and if it doesn't care it will ignore the message. So it only locally has to handle a few kinds of message.

On the other hand, if someone is writing a debugger or bus sniffer that needs to decode and display all kinds of messages, then it will indeed have to handle dozens or hundreds of message types. But it won't do this with huge amounts of code, it will use a database where it looks up the message type and gets the decode information from the database, so it can automate the process. It will use this database to figure out how to unpack and decode the contents of each message.

[brucehoult]

It's the principle! remember, you do it today because although not a scalable solution it's a small problem. Then in the future the problem scales, the solution cannot and you have to reinvent the wheel. Tell me, how do J1939 auto ECU's manage this problem? they have several hundred messages of no same format, you are telling me that someone wrote a message handler for each one?

Only hundreds? They may well have written them by hand.

Or, they might have written a small program to parse some description of the messages and generate the code that would otherwise have been written by hand.

Premature generalisation is a sin. "YAGNI" as the eXtreme Programming people say. People such as John Carmack agree: "It is hard for less experienced developers to appreciate how rarely architecting for future requirements / applications turns out net-positive."

[Simon]

Premature generalisation is a sin. "YAGNI" as the eXtreme Programming people say. People such as John Carmack agree: "It is hard for less experienced developers to appreciate how rarely architecting for future requirements / applications turns out net-positive."

In my experience of both hardware and software, as soon as you have a working solution it becomes the way it is done, the first time you get it to work is the way you will go on to make it work. You cannot turn around to someone in management or sales and say that this thing that works right now, we have to scrap it for the next project and start again. They don't understand, they have no idea of the many ways of doing things and how just a small change in requirement in their opinion is a massive shift in what they are now asking for.

At my last job I worked with a very good contracting engineer, we had 5 hour straight meetings and thoroughly hammered out everything, we were always grateful we did as come the next project we already had solutions that had already been used and simply needed to add to our arsenal, again, with consideration for how to generalize the solution slightly.

[Simon]

It's the principle! remember, you do it today because although not a scalable solution it's a small problem. Then in the future the problem scales, the solution cannot and you have to reinvent the wheel. Tell me, how do J1939 auto ECU's manage this problem? they have several hundred messages of no same format, you are telling me that someone wrote a message handler for each one?

In a sense, maybe yes.

In at the hardware level, on a message bus, each device will only pick out and process messages it is interested in. It will look at the PGN and SPN to decide this, and if it doesn't care it will ignore the message. So it only locally has to handle a few kinds of message.

On the other hand, if someone is writing a debugger or bus sniffer that needs to decode and display all kinds of messages, then it will indeed have to handle dozens or hundreds of message types. But it won't do this with huge amounts of code, it will use a database where it looks up the message type and gets the decode information from the database, so it can automate the process. It will use this database to figure out how to unpack and decode the contents of each message.

Yea sure, a temperature sensor can do what it damn well likes, the software in it is definitely limited in scope. I am talking about an ECU on which I already run CAN open. What you are saying is that a J1939 ECU is basically running each message like a CAN open PDO message, just with different message ID's. I already handle PDO messages as you describe so yes I can effectively convert the 3 messages in question to run on the PDO handling code.

But the fact still remains that hardware registers in micro controllers are mapped with structures. As they are hardware registers it is not a question of arguing about how the struct memory is allocated. It is already allocated by hardware and these systems work. I understand that maybe RAM allocated structures will be created differently but then I want to know why and how. I can't just keep followring recipes.

If you want an example of my frustration, here I have been told that I should not define variables as static unless inside a function. In another thread months ago I was told I absolutely should declare anything static that I did not want to be global, you see my problem? The language was created in the 70's and not formally standardized until 1989, and then standardized about ever 5 years. People tend to stick with what they learnt when they were young and then don't change, so opinions are always mixed.

[brucehoult]

If you want an example of my frustration, here I have been told that I should not define variables as static unless inside a function.

I can't find any such thing in this thread.

In another thread months ago I was told I absolutely should declare anything static that I did not want to be global

If you have code that uses global variables with the same name in different files and you don't want them to actually be the same bytes in memory then, yes, all of them (or all but one) need to be static. Exporting the name, and the linker merging all global variables with the same name, is the default.

[IanB]

But the fact still remains that hardware registers in micro controllers are mapped with structures. As they are hardware registers it is not a question of arguing about how the struct memory is allocated. It is already allocated by hardware and these systems work. I understand that maybe RAM allocated structures will be created differently but then I want to know why and how. I can't just keep followring recipes.

Well, yes, in many systems, hardware I/O registers are mapped to particular memory addresses, and then it is possible to overlay a structure on that section of memory to make it easy to access the registers by name. However, it is still essential to define the struct carefully to ensure the structure elements align properly. When these structure definitions are provided in a header file, someone has gone to great care to make sure this works.

If you want an example of my frustration, here I have been told that I should not define variables as static unless inside a function. In another thread months ago I was told I absolutely should declare anything static that I did not want to be global, you see my problem? The language was created in the 70's and not formally standardized until 1989, and then standardized about ever 5 years. People tend to stick with what they learnt when they were young and then don't change, so opinions are always mixed.

Well "should" or "should not" are much too broad and general to be treated as good advice. If someone tells me I should not eat red meat, the first question I am going to ask is "why not?"

It is an unfortunate cock-up in C that "static" at file scope has a different meaning from "static" at function scope.

Declaring a variable static at file scope causes it to be local to the file and invisible outside of it. Whereas a variable not declared static at file scope is still static in the sense of maintaining its value, but it is also visible to other files in the linkage unit (it can be referenced with the extern keyword). This means all variables at file scope should be declared static unless you want them to be visible externally.

On the other hand, static inside a function has nothing to do with visibility, and just causes the value to be persistent between function calls. There are a few cases where this is needed, but not many. More often than not it is a really bad idea and should be avoided as it can prevent recursion and re-entrant behaviour.

[Simon]

If you have code that uses global variables with the same name in different files and you don't want them to actually be the same bytes in memory then, yes, all of them (or all but one) need to be static.

So I won't call that a contradiction but from what you say you assume that all variables not defined in a function are normally accepted to be global at inception unless qualified with static, when I do use global variables unless I declare them as extern in a header file that is included where ever they are used or I end up with issues with the link you describe not necessarily happening and strange stuff or the compiler outright complaining (hopefully). so the compiler has to be explicitly told either way or you get undefined behavior, unless of course you are setting up a variable that will be interpreted as global but that is used in that file alone and there is no other named variable.

[Simon]

On the other hand, static inside a function has nothing to do with visibility, and just causes the value to be persistent between function calls. There are a few cases where this is needed, but not many. More often than not it is a really bad idea and should be avoided as it can prevent recursion and re-entrant behaviour.

On the other hand I find static variables in functions incredibly useful being able to put variables that need to be remembered between function calls but that are used only by that function in that function so that I have a single unit of code with everything it needs. Classic example in my project is the last_run variable that any function called in main has. It would be a nightmare to have many of these defined outside of the function. I believe that this is a good use of a static variable in a function and it was an eye opener when I found out about it, as you say as the word has two meanings I was initially only aware of the file level one.

[Simon]

But the fact still remains that hardware registers in micro controllers are mapped with structures. As they are hardware registers it is not a question of arguing about how the struct memory is allocated. It is already allocated by hardware and these systems work. I understand that maybe RAM allocated structures will be created differently but then I want to know why and how. I can't just keep followring recipes.

Well, yes, in many systems, hardware I/O registers are mapped to particular memory addresses, and then it is possible to overlay a structure on that section of memory to make it easy to access the registers by name. However, it is still essential to define the struct carefully to ensure the structure elements align properly. When these structure definitions are provided in a header file, someone has gone to great care to make sure this works.

And I was equally careful in creating my structures to send CAN data but as things will scale and this is an error prone way to do it at scale I will convert to PDO handling as that system is already written.

[Mechatrommer]

Premature generalisation is a sin. "YAGNI" as the eXtreme Programming people say. People such as John Carmack agree: "It is hard for less experienced developers to appreciate how rarely architecting for future requirements / applications turns out net-positive."

things like "refactoring" is imho one of "you are going to need it in the future" in mind. following YAGNI, whats the point of refactoring? as long as it work? but its very well accepted concept. experienced programmers know how to refactor well since they can anticipate which one is more reusable than the others, there is no way i know of describing this in words (experience) you need to look at the code wholly and you know what to do. this is important when laying out a programming from scratch, usually on first stage version 1.0.0, very well generalized / refactored code to start with can ease scalability (upgrade, expansion or bugs fix) process later and sometime done by another new programmer, and can enforce or as a guideline example on how good coding practice is/should be done in that particular project, if uniformity of style can be maintained throughout the process it can help new maintainer to understand the code and paradigm quickly. let alone basic bugs like incorrect casting or memory overrun/leak issue, inconsistent declaration etc, coupled with unorganized spaghetti code will be a nightmare to fix. ymmv.
https://www.freecodecamp.org/news/what-exactly-is-a-programming-paradigm/

[Simon]

This program as it is has been through much rewriting, simply because I have had to learn about CAN Open the hard way. Every time I learnt something else that was needed to make the system work I had to go back and rewrite functionality to support the improvement. There was always the temptation to just get it to work but I always saw that this would just create even more convoluted code that I would be stuck with and it would actually take as long as the rewrite, but some one could point at what I had and say why change?

We had this argument about the whole machine. it has 2 analogue inputs that can be used to run the motors, but I did not like this, but I was told, yet but it works.... I said no. This is a very bad way of doing it in ways that you don't understand because you are not an electronics engineer. We must take on this CAN open thing because sending a voltage that has to survive interference and be interpreted the same by multiple motors is a fools way when I can send a number, and that number will always arrive as sent.

The previous iteration of the machine used PWM outputs, put through digital isolators, low pass filtered and then sent to the motor driver analogue inputs. A very sorry state of affairs. I put my foot down and here I am 2 10 weeks later with a barely functioning CAN Open implementation that I am still ironing the bugs out of but not for a moment would I give it up and go back to what we had.

[Psi]

Why CAN and not something much simpler like RS485 where you roll your own small frame-format/protocol to only do what you need for your application. Instead of learning tons of CAN stuff

I'm not saying you shouldn't use CAN or you should use RS485. I don't know enough about your application. Just curious why CAN.

[DiTBho]

Why CAN and not something much simpler like RS485 where you roll your own small frame-format/protocol to only do what you need for your application. Instead of learning tons of CAN stuff

Think, I worked on a quadcopter project for semi-professional aerial 4K shots + FPV, and we used canBUS for the ESCs that drive the motors as well as for the gimbal that positions the camera

Same question - Why?  - I asked the project manager - because I tell you, and because the customer wants it that way - he replied

So 
 
RS485 seemed much, much simpler to me, both on the hw, fw and sw side. But nothing to do. No dice.
And years later... I found myself in exactly the same situation with electric actuators on bicycle derailleurs.

but in this case, for my project, I will not use canBUS, even because I still have the PCI32-CAN card HBA I used for the quadroter and its kernel drivers stuck in kernel v3, which no one has ever wanted to update and... they are so complicated that I don't even want to.

[DiTBho]

Premature generalisation is a sin. "YAGNI" as the eXtreme Programming people say. People such as John Carmack agree: "It is hard for less experienced developers to appreciate how rarely architecting for future requirements / applications turns out net-positive."

I do agree, too 

[Simon]

Why CAN and not something much simpler like RS485 where you roll your own small frame-format/protocol to only do what you need for your application. Instead of learning tons of CAN stuff

I'm not saying you shouldn't use CAN or you should use RS485. I don't know enough about your application. Just curious why CAN.

The motor drivers use CAN Open and that is the end of that. I can't change that and anything motor wise off the shelf is much more likely to be CAN Open than anything else.

As for the implementation of CAN bus itself at a low level you have to have a good long think (hours not minutes) at how you solve all the problems the CAN bus controller inherently solves in hardware that you have to work out for yourself and code in software with RS485 or any other USART type port bus.

Yes the CAN bus controller peripheral on the SAMC is complex, but good news, it was designed by BOSCH and is licensed into many micro controllers and PC hardware. So having a working knowledge of that particular controller is very portable.

You are of course confusing CAN bus with CAN Open in your comparison to RS485. RS485 is simple enough in principle but the whole reason you use serial communication is so that you have an easily scalable system to create a network that can increase in size by a sensible amount. Even with just two devices you will have lots of fun with RS485. Unlike CAN bus RS485 is not collision tolerant, there is no bus arbitration in hardware that resolves conflicts automatically so that the winner transmits successfully and the looser tries again automatically. All that CAN bus handles automatically in hardware with no CPU cycles used is absent in RS485.

Once you try to solve the problems of bus contention you soon start scheming up quite a complex system of master slave messaging, one that may not rival CAN open in size but certainly means you are not just writing code to follow some rules, you have to make the rules up yourself, make sure they are fool proof and then implement them whilst wishing you were not spending so much of you CPU time on communication rather than your application. You soon start to miss that co processor that is the CAN bus controller.

If I used RS485 then rather than CAN Open I would be implementing something like MODBUS on RS485 as that is what any motor driver would have. I don't know how complicated it is but once again I'd be writing as much code nearly as I have on CAN Open. I also believe that CAN Open is also implemented on RS485 too....

No matter how you try to solve these systems the problems are universal and the total solution will always amount to about the same, but CAN bus does more in hardware for you alleviating the load.

Lin bus was invented as a companion to CAN bus, the reason? to save money in cars, first cars saved money in wiring looms and connectors by implementing CAN bus, then they realized that this was a lot of hardware to put into a window motor and every single micro controller has a UART but not a CAN controller as a UART is really simple, and if it is just a window motor or a sensor then it's OK to spend most of your CPU time on processing. But Lin bus networks are just small networks that connect to a CAN bus enabled Lin master that uses CAN bus to talk to the rest of the car but now only one sub system has to have the "expensive" CAN controller. it was worth the mammoth software task of writing the Lin stack because if you add up all the little 8 bit micros in a car and multiply them by the amount of cars made each year it is a lot of money and a totally different effort/reward analysis.

[Simon]

And just to say, I am updating my motor drivers every 7ms at the moment, try that on RS485.....

[langwadt]

And just to say, I am updating my motor drivers every 7ms at the moment, try that on RS485.....

what do you imagine would be the issue?

[DiTBho]

@Simon
what do you use on your GNU/Linux computer as a canBUS HBA?
USB-to-CAN? 

[Simon]

And just to say, I am updating my motor drivers every 7ms at the moment, try that on RS485.....

what do you imagine would be the issue?

Well I don't know that there would be any for sure but I do know that RS485 would come with extra overheads for the CPU compared to CAN bus, so I would be careful about going mad like that.

So there are 2 messages being sent to each driver and the driver responds with 3 messages, so every time I send a sync message onto the bus that is a total of 11 messages, there is also a heartbeat message x3 being exchanged every 500ms and some background SDO communication once per second. Decoding an SDO message is a lot of work, it takes longer than a message to be sent.

On top of that the micro controller CPU has to do all of the running of the system, working out what to send to the motor drivers next and keep the HMI on it's own CAN bus updated.

If I were doing this on RS485 I would have to do the error checking calculations on both sending and receiving. This would be pretty simple algorithms simply because I know no better. CRC is not something I have done. The CAN controllers do CRC for themselves. I would also have to totally manage the bus access of every message from the ECU program. So with CAN open I just chuck 4 messages at the CAN controller and leave it to transmit each one and then send a single sync message which again I just send without having to care about bus contention.

Once the sync message is received 4 responses come back, with an RS485 system these could not just respond like that, each one would have to be triggered, so 4 request messages instead of one sync message.

Remember, we are only talking about 2 drives here, CAN open supports up to 127 slaves, and the PDO messages do not have to be sent every sync cycle. Now could an RS485 based system where the application code has to include all that the CAN controller does, manage so many slaves at speed? I would be looking carefully at an RS485 system that had to do this as I would have one CPU, not the equivalent of 2

Of course, you forget one important aspect, I can't make this decision. Like all drive manufacturers, these are designed to work on CAN Open that sits on a CAN bus layer, I could not use RS485 if I wanted to, and I can see why by default RS485 is not used.

The other advantage of CAN bus is that it is easier to add more nodes. Because on a UART based system you MUST have a strict master slave scheme or there will be bus clashes. Many drivers these days will cope with this electrically, but if it is a normal condition then that is more code writing, more overhead, more development time to replace hardware with software. So if you use RS485 either you need to update the master code to include any new device or you need a system of scanning the bus at start up to see who responds, another mountain of code. If you were to use predefined ID's to represent devices you soon end up with more ID's than the system will ever have as you must include scope to scale the system or the whole point of the system is defeated by constant rewrites.

You see I am not joking when I say that I have given all of this many hours of thought, over the last few years days of thought in total. But when the day came that I had to argue about using CAN bus / CAN Open and upset all of my management right up to the boss of our billion dollar group as I committed 2 months of full time work to what was thought to be a folly, I knew that I was right, even though, I still had no choice as there was no RS485 option.

Well I think the drive manufacturer has an RS485 addon but looking around it is clear to me that CAN bus is at least as prevalent if not more preferred than RS485 by all motor driver manufacturers and in my line of work it is motors all the way. At the end of the day it is not my choice, but I am happy with the choice.

The CAN bus layer took 2 weeks to develop. If this was an RS485 system there would have been more development time to deal with the stuff that a CAN bus controller does in hardware. Then and only then I would have started to tackle any of the actual higher level protocol stuff for I assume MOD bus.

Oh, another CAN bus advantage, it has it's own dedicated DMA with the system RAM, pretty neat, so not only a coprocessor but dual channel RAM access....

[Simon]

@Simon
what do you use on your GNU/Linux computer as a canBUS HBA?
USB-to-CAN? 

I have not got to the Linux stuff yet.

Before you go on to make a clever point, remember, it is the motor driver manufacturer that chose the CAN bus option for me.

On a linux machine i would have a who SBC of resources at my disposal, I would also not be writing low level code, so the choices are about entirely different issues. it's about what is natively supported.... ooh, I hear there is this thing where CAN bus controller IC's on SPI have a driver baked right into the linux kernel and there is something about socket CAN. But this is all for another day so don't worry, tons of threads on that when I actually gfet to it right after this mess

[ejeffrey]

You can either offer a proper answer and stop telling me I am ranting for weeks or continue to belittle me.

I don't know what your question is and you haven't provided a clear *example* (that means code) of what you are doing.  You verbally describe 1-2 lines of code at a time without context and I for one am completely lost at what you are actually doing.

[Simon]

You can either offer a proper answer and stop telling me I am ranting for weeks or continue to belittle me.

I don't know what your question is and you haven't provided a clear *example* (that means code) of what you are doing.  You verbally describe 1-2 lines of code at a time without context and I for one am completely lost at what you are actually doing.

I attached the entire project. Then I was told that this was also not good as it is too much to go through. Funnily enough that is what I tried to explain. I explained the bare bones of what was involved with the mysterious issue but apparently that is too complicated to understand. Sure if I spend time recreating a project with just that small piece the issue will likely go away because as we have established there is a wider problem somewhere.

[Simon]

I have found another place where too much data is being copied by memcpy, that is likely the culprit.

[brucehoult]

Premature generalisation is a sin. "YAGNI" as the eXtreme Programming people say. People such as John Carmack agree: "It is hard for less experienced developers to appreciate how rarely architecting for future requirements / applications turns out net-positive."

things like "refactoring" is imho one of "you are going to need it in the future" in mind. following YAGNI, whats the point of refactoring? as long as it work?

Refactoring is what you do later on when you discover an ACTUAL requirement that can not be easily implemented in the current design.

You refactor the code -- that is, change its structure without changing the functionality AT ALL -- until it is in a form where the now known to be needed functionality is easy to implement.

And you do and commit that in TWO steps, not one. In fact preferably three:

- refactor, verify that all tests still pass

- add tests for new desired functionality, verify that they fail

- add new functionality, verify that the new tests now pass, and that old ones still do.

[DiTBho]

And just to say, I am updating my motor drivers every 7ms at the moment, try that on RS485.....

what do you imagine would be the issue?

ehehe, I worked on some industrial cutters connected to reels-winding, machines of several tons, as big as a room that make shopping bags for supermarkets, unroll a reel of thin plastic film, cut it, and seal it hot, also making 16 bags in parallel.

I happened to have to design a perimeter Lidar 2D, and a part of the unit that manages both the pneumatic part and the blades array, and then have to interface them with the "computer" that manages the machine, which I have no idea what it is (I couldn't take a peek (1)), but I found EtherCAT, which I also found in avionics, as well as AFDX, in UDP/IP based applications with custom go-back-n algorithms.

They { EtherCAT, AFDX } are both ethernet based, but have been modified to be not only deterministic, but also lightweight, and in the work I have done, the response time was within 5 msec.

I found Tulip chips (ex Digital tecnology), which, in Linux and VxWorks terms, they "offload the CPU" - written like this in the manuals -  by performing lowlevel (LLC) checksums and performing networking operations autonomously, so the CPU, doesn't matter if it's a superscalar PPC460 with 4 cores and 32MByte of L2 cache!!!, rather than a non-superscalar MIPS-I wheelbarrow, with neither cache nor pipeline, the CPU always sees "the network coprocessor" as interrupt-driven and DMA-driven partner.

Similarly, in the world of perimeter security (both civil and military, from avionics to naval), I have seen engineers with 30 years of experience, i.e. people who started working in 1990, developing solutions based on RS485.

They know how to do it because they have been doing it for a long time, and they are solutions that couple the RS485 with ad-hoc intelligent cards that practically do "offload" networking functionalities.

Pretty like Tulips (used in { EtherCAT, AFDX, ... }) and CanBUS.

- - -

So, in my opinion today there is a lot of emphasis on canBUS because it presents itself as a ready-made package, as well as Bosch is a very solid and serious company that offers solid guarantees, while these 90s engineers are close to retirement age, so it would cost more to train new engineers, also considering that it is a rather old technology.

It costs much less, and that's not something to underestimate 


(1) the security guy was watching me closely, and he was much bigger than me - "don't touch that panel!!!" - I risked getting slapped several times!
I was abroad on a business, so … somehow I managed to keep my curiosity in my pockets.

[Simon]

The simple fact is that if you have onboard CAN bus on your micro controller you effectively have a comms co-processor. One thing I forgot to mention earlier about Lin was that it runs at a mere 20kbps with a fast option at 80kbps. Lin also does not need a crystal for the clock as it can self synchronize at the start of every message. So another huge cost taken out. at such a low speed getting the system RC oscillator to resync is probably easy as there is lots of resolution between a few MHz and the 20-80kHz. It was designed for a specific task and has the trade off of those tasks.

As I said I have thought long and hard about how to implement an entire network off a UART and it is not easy. At my last job we did it or rather the subcontractor did it for us after using CAN bus on previous projects. But those were customer specific systems that would not change and we knew what the limit devices were on the bus so the ID space was simple enough.

In this job I work on any motor driver that I can lay my hands on and CAN Open is very much the industrial standard to the point that it works on Ethernet and RS485. I don't know how they do this but the manual for the Drive covers both ethercat and CAN Open at the same time.

I am guessing that if you want even more speed and more devices ethernet is the physical layer designed for that, CAN FD is still in it's infancy with CAN Open but that does increase the speed 10 times and I think you can implement the extended ID system if you really want to. If anything CAN bus as a physical layer is designed to run both standard and extended ID's and I beleive CAN and CAN FD can work together. I base this on the Bosch CAN controller have different CANS FD modes, one where is sends the header at normal speed but the data at the faster rate and one where it sends the lot at the higher speed.

I think these are all the things that keep CAN in the industry, it is so robust but also able to coexist whilst reducing development time. In my line of work if the micro controller with CAN costs twice the price it is still a no brainer, our volumes are so low that development time is more expensive. So I have spent 2 months on CAN Open simply because after two and a half years at this company it has become very clear that CAN Open dominates in the supply chain we use so I either get with the game or get left behind. There are so many projects I have to work on in the future that involve motors that having to design bespoke circuitry for each motor will just slow everything down. My current experience has been that I have tried 3 motors for this project, 2 of those spoke CAN Open, one of them I rejected at the time as clearly they just did not have decent alternative IO options and I did not have CAN Open as an option. Now I do, if I had been confident enough at the time to put a hold on the project and develop CAN Open at the outset, the job would be done by now. That is the power of standardization. The next project I will work on I will simply plug the same electronics in and get it working.

[Nominal Animal]

Refactoring is what you do later on when you discover an ACTUAL requirement that can not be easily implemented in the current design.

Or when the current design maintenance cost is higher than the cost of replacing it with a simpler, easier to maintain design.

You refactor the code -- that is, change its structure without changing the functionality AT ALL -- until it is in a form where the now known to be needed functionality is easy to implement.

Without changing the used functionality; unused features and parts of the design are (or should be) removed.


Refactoring things to minimize code complexity and cost of code maintenance is a very important part of my work flow.

Story time, feel free to skip/ignore:

My own design approach differs from the most common ones, in that I like to test important features in modular test programs/firmwares before using them in a design.

For example, let's say you have an embedded design where for some reason, you need many lightweight timeouts that often/typically get cancelled before they elapse.  (This is very common in systems programming, especially in any kind of internet or web service, and one I have lots of experience with; that's why I'm using it as an example here.)  My preferred solution is a combination of a binary min-heap in a fixed-size array, with each entry containing the timeout time (when it elapses/triggers) and the timeout index/id.  In parallel, there is an array with the same number of events, with each entry containing the current min-heap index, plus any associated information needed for the implementation (like elapsed flag, or whatever happens when the timeout elapses).

I start by creating a basic min-heap implementation, with the entries using appropriate types for the target processor and maximum number of concurrent events.  (I typically use an unsigned integer type for the timestamp.  If the maximum allowed timeout (duration) is N-bit, then N+2 bits suffices for the timestamp, giving me absolute reliability according to tests I've done before.  Others can do it in N+1 bits, but often end up having off-by-one errors near the maximum duration timeouts.  With non-power of two durations, three times the maximum timeout duration suffices for timestamp range.)
The test implementation is run on a fully-hosted system (Linux), with inputs supplied from files, so I can easily construct test cases, including the worst cases I can imagine – those being the most important to test for in my opinion.
When the basic min-heap works as I need it to, I save it as a separate unit test, but continue with adding the identifier array for canceling timeouts and triggering timeout events.  If there are different event mechanisms, I test those separately, without the min-heap at all.

When that sub-system has been tested and works, I add my comments on my current ideas on how it will be used in the final firmware/application, but do not integrate it yet.  I make sure I have all the important modules working in isolation first.

When I have all the modules tested, I start the integration work.  While one could just add all the modules at once, I prefer to do it one module at a time, because that way I minimize the area where bugs and errors and issues can arise.  After adding a module, I do full testing, including the new tests for the added module, of course.

If the firmware/application is complex enough, I often realize that two (or three) modules could/should be merged, because that would yield better, easier to maintain code.  (If it adds to the code complexity, I consider merging having negative value, because the code I write tends to get used for a long time, so cost of maintenance is as important as cost of creation to me.)  I end up refactoring the modules in a separate unit test/module; usually not within the actual firmware/application directly.  (I tend to have learned more about exactly what features matter, so I do tend to rewrite the tests, instead of just reusing the existing ones from the modules; but test coverage should always expand, and not shrink, here.)  Reintegrating into the firmware/application is then done just like for any other module.

This leads to things like having to carefully document the internal interfaces to each subsystem.  This makes refactoring easier, because from the interfaces actually used in the firmware/application, you can tell if parts of the "modules" are unused, and can be discarded or refactored.  It also explains why I find modular programming and software minimalism (and their combination, the Unix philosophy) so useful: as a proven historical track record, this approach produces good results.

This serves me well, but makes me somewhat less productive in terms of 'lines of code of new features per day'.  My bug rate is much lower than typical, and I tend to keep core documentation up to date because I rely on it myself, so I consider this well worth it; but many, especially manager types worrying about the production costs, often disagree.  (I still do struggle with writing truly useful code comments, though.  Should have learned to do that from the get go, instead of later on!)  So, I do not recommend adopting this for those who do paid commercial work, because it can reduce their attractiveness in employers eyes.  I am not limited by that, as I only work for free or for myself, currently.

[DiTBho]

@Nominal Animal
I did/do the same, so I totally understand you!

let's say that among the various approaches (which are all correct), even this is a good way to proceed, but it's special because at least it garanties that bad surprises are avoided, and things move forward in a methodological way, which is excellent especially if you work as a team!

as a side effect then, it favors documentation, precisely because you focus on one module at a time, and documentation is necessarily useful when stitching everything together.

[DiTBho]

The simple fact is that if you have onboard CAN bus on your micro controller you effectively have a comms co-processor.

yeah, this is an extra-bonus 

[Simon]

The simple fact is that if you have onboard CAN bus on your micro controller you effectively have a comms co-processor.

yeah, this is an extra-bonus 

In my case a double bonus as there are two independent CAN bus controllers and as I am stuck with the design of the HMI until I replace it I can run it on one bus and the motor drivers on another bus so I do not risk upsetting anything.

The other thing I forgot to list that the CPU would have to do with a UART based bus is filter every message. Every message must be received and filtered by the program. With dedicated CAN bus controllers not only is the CRC stuff done by the controller but it also worry's about whether or not you wanted that data and it only informs the main program about messages that the main program actually wants. on a network of many devices using most of the time you spend on comms rejecting messages that are not wanted would seriously overshadow the time spent on the messages that the program does want if you use a UART based bus.

[Mechatrommer]

Refactoring is what you do later on when you discover an ACTUAL requirement that can not be easily implemented in the current design.
You refactor the code -- that is, change its structure without changing the functionality AT ALL -- until it is in a form where the now known to be needed functionality is easy to implement.

i dont know you people in professional industry, but me with my hobby when i start a project, i usually just code whats needed, but when it grow a little bit larger i figured my functions keep duplicating the same thing, or too many little little separated units/functions/modules, thats where i quickly refactor separate or combine. and since some experience in belt, when there is a function needed and i know from the past that function is commonly called, then i will do it in separate function in first place. refactor when the code is already huge is quite a task since you need to keep all functions in memory to know what to stripped off and what to add or combine etc. sometime it took 2 or 3 times refactoring during code development, but thats just me.

[Siwastaja]

The motor drivers use CAN Open and that is the end of that.

Then don't implement fully generic CANOpen. You don't need other profiles than the motor drivers. Just support the message types needed by the motor drivers.

[Simon]

The motor drivers use CAN Open and that is the end of that.

Then don't implement fully generic CANOpen. You don't need other profiles than the motor drivers. Just support the message types needed by the motor drivers.

Other profiles? do you know how many there are? tons I think. Of course I have only implemented what is needed by the motor drivers. They need the CAN Open comms to work as expected. All that the profile is about is the list of things stored at various index locations and any other functionality like foc DS402 the power up.

But CAN Open communication is no simple matter.

With my massive 16kB of RAM I can only setup the objects that i will actually use, if I were running it on computer like a raspberry pi I'd have an easier time just allocating the entire dictionary space and just using using it or not.

[SiliconWizard]

Just a thought here regarding your last sentence - there's a very wide range of MCUs out there in terms of RAM size, between 16KB and the several GB a SBC supports.
Surely you must have had a reason for choosing this limited MCU for this application, maybe the choice isn't the wisest, I don't know. Even a RP2040 which is less than $1 has 264KB of RAM. Anyway. Not that what you wanted to achieve all along is impossible with 16KB - looks like a few people have given tips and approaches. But if that really makes your development this uncomfortable, selecting a different MCU may have been a relatively cheap approach.

[Simon]

Again it was politics. I need to develop a HMI and master ECU around a single board computer but that would have been a step too far, as it is it has taken 2 months just to get to understand what CAN Open requires and I still need this micro controller with the CAN bus set up on it. So I only rocked the boat as much as I needed to.

No micro controller will have the RAM for a full easy CAN Open master. The definition of the CAN Open object dictionary is that each object is addressed by an index of 0-2^16 and a sub index that can go up to 255.

If you take this to the extreme this 2^24 of combinations is 16million. If I were to use the system I came up with of assuming int32 will contain any object that is 64MB per slave device. So to replicate the slaves object dictionary in the master I could take the easy way of an array of:

int32 objects[65535][255] ;

or better for the entire network:

int32 objects[n_nodes][65535][255] ;

that is quickly a huge amount of RAM. Now of course I will never need to actually map the full 2^16 indexes or 2^8 sub indexes and I could have 3 arrays, one for each of the sections of object dictionary: communication settings, manufacturer specific section and profile section with only the range that I actually use although this is making the code less universal.

So the solution here was instead to simply have each object in a structure where the index and sub index is stored as well and these are in an array. I then had to create an index of the object names as an enum list so that I can search through my limited dictionary.

No microcontroller will have the MB of RAM needed to create the dictionary the easy way. I am also in a condition where ironically my lower power CPU has more work to do to address an object as there is no natural way to call the index and sub index, but it is often not a problem. On something with enough RAM I could simply use the index and sub index. So finding the object that an SDO message addresses is very fast as I would simply use the index and sub index in the message to address the array rather than have to search through the indexes slowly with a slower CPU. A SBC with enough RAM would have a much easier job as it can simply address the dictionary instead of search.

So any micro that does not have many MB of RAM will have to go through what I have here. The SAMC is simply what I know and have code for, there is a 32kB version but there were not available when we bought stock, there are other compatible MCU's higher up the product range but it's still a case of kB versus 10's of MB which is the difference between a micro controller and a computer that can run an OS like linux.

[gf]

How many entries does the dictionary actually contain? Tens, hundreds, thousands,...?
Since you say that an array implementation would be far too sparse and take up too much memory, have you considered a hash table or tree implementation for the dictionary?

[Berni]

You need to implement a dictionary structure rather than a simple array. You can get those in C++ and all the modern ARM compilers can take C++ code.

Also if you do want lots of RAM in a MCU, that's no problem. Lots of larger MCUs support an external memory bus that can run SRAM or SDRAM memory. The amount they can actually address varies a lot but should be somewhere in the range of 8 to 256 MB

Still none the less this sounds like a bad excuse for needing that much RAM

[brucehoult]

You refactor the code -- that is, change its structure without changing the functionality AT ALL -- until it is in a form where the now known to be needed functionality is easy to implement.

Without changing the used functionality; unused features and parts of the design are (or should be) removed.

STRONGLY disagree!

Adding or removing functionality of any kind is not a legitimate part of refactoring

Removing unused functionality may certainly be a good thing, at times, but it's not refactoring. It's before and/or after refactoring.

i.e. refactoring may make it easier to remove unused functionality, and removing unused functionality may enable a refactoring simplification.

But they are separate steps and separate commits.

[Nominal Animal]

You refactor the code -- that is, change its structure without changing the functionality AT ALL -- until it is in a form where the now known to be needed functionality is easy to implement.

Without changing the used functionality; unused features and parts of the design are (or should be) removed.

STRONGLY disagree!

I specifically refer to omitting unused/dead code, not adding new functionality.  To me, "refactoring" = "replacing an existing chunk of code with its functional equivalent", i.e. not changing its externally observable behaviour.

I take it you prefer the stricter definition, where that is called "rewriting" or "replacing", and "refactoring" only refers to functionally identical code?

I keep the modules so small I get no benefit from splitting the functionality removal and the rewriting into separate steps.  Now, I do admit, I sometimes even modify the function call interfaces at the same time –– which is definitely beyond refactoring in my opinion –– if the modules are small enough and I've documented the interfaces well enough.

But they are separate steps and separate commits.

Separate commits definitely makes sense, so that at each commit the source code is in as valid state.

I don't often use source control for my own stuff, as I do better documentation when working on module-sized chunks (compared to changelogs).

Writing descriptive comments, using source control, writing descriptive changelogs, and then separating dead code removal from functional rewrites makes a lot of sense, and is indeed what I recommend others do.  What I do is definitely not the optimal or recommended way.  There is always room for improvement!

[Mechatrommer]

i think refactoring is including cleaning up once used but now unused code, but not adding extra functionalities, the purpose is more readable/maintainable to what is "already existed" https://refactoring.guru/refactoring

[DiTBho]

I take it you prefer the stricter definition, where that is called "rewriting" or "replacing", and "refactoring" only refers to functionally identical code?

"Doors" terminology, applied to a project under DO178B:

• "refactoring"(modules) -> modules must have exactly the same interfaces and global variables
• "sweep"(modules) -> dead-code (unused { functions, global variables, header structures and const}, ...) removed

two steps, two documents, to be sent to the QA guys

for my personal projects I do both things in parallel, or first I do sweep, and then refactor.
Probably it's not correct, but I do this for the simple fact that I have less work to do,
and that I do these things in my spare time.

(1) it's like Git, but commercial, used in Avionics.

[Nominal Animal]

i think refactoring is including cleaning up once used but now unused code, but not adding extra functionalities, the purpose is more readable/maintainable to what is "already existed" https://refactoring.guru/refactoring

The exact definitions of the terms are not that important, as long as everybody understands what is meant.

brucehoult's core point that when using version management, you should first remove the unused code as a separate commit, before replacing the code in another commit, is quite valid: that way, each commit yields a functional, compilable, testable state.

This is particularly important/useful when hunting for complex or rare interaction bugs that require commit bisection (binary search for the commit that changes the behaviour wrt. the bug or issue): it minimizes the changeset per commit, while keeping the tree in compilable, testable state in each commit.

[Simon]

How many entries does the dictionary actually contain? Tens, hundreds, thousands,...?
Since you say that an array implementation would be far too sparse and take up too much memory, have you considered a hash table or tree implementation for the dictionary?

I don't quite understand what those are but I suspect what I have implemented is close to one of them. I have a structure type that stores the index, subindex type and values. I have set up an array of these structures. I have a list (enum) of the names that I use to index the array in English.

[Simon]

You need to implement a dictionary structure rather than a simple array. You can get those in C++ and all the modern ARM compilers can take C++ code.

Also if you do want lots of RAM in a MCU, that's no problem. Lots of larger MCUs support an external memory bus that can run SRAM or SDRAM memory. The amount they can actually address varies a lot but should be somewhere in the range of 8 to 256 MB

Still none the less this sounds like a bad excuse for needing that much RAM

Ultimately if I am seriously implementing CAN Open and I want to scale I need something more suitable. The cost of a SBC is trivial in my line of work and I don't want to keep changing implementation. I also need to run a screen anyway and will be figuring out programming under linux soon.

As for actual RAM needs my current method will probably also work quite well without much overhead. I had misunderstood the SDO protocol at the start, now that I know better I don't need to keep track of the index I sent and the the one that I got back, I need to remember the last slot I sent (the array index) and when I get a response I check to see if the received index corresponds to that of the one I saved the index of when I sent, no searching required. I totally over egged it.