When you declare your structure and your typedef, you are only telling the compiler what one of these things will look like, but not actually allocating any memory for it.

Yes, I know. However, I thought so long as I create an instance of it as a global variable then memory will be allocated at that point.

 

Yes, I know. However, I thought so long as I create an instance of it as a global variable then memory will be allocated at that point.

Yes, but in general you should only use global variables if you have a really damn good reason. Damn good reasons DO exist, but in three decades of programming in C I have come across a damn good reason two, maybe three times. Now, in the embedded world the rules are somewhat different and so damn good reasons are likely more plentiful there.

 

In general you want to avoid returning structures for the same reason that you want to avoid passing them by value -- they will chew up your stack and it takes time to push and pop them from the stack.

Instead, pass two pointers to your function, one for the new and one for the old.

Any time that you are using the dot derefence operator (i.e., temp.i_a), you are probably not doing it the way you should (there are always exceptions).

Instead, work with pointers to the structures.

void calibrate_measurements(measurements *cal, measurements *raw)
{
    cal->i_a = (raw->i_a - CURRENT_A_OFFSET) * CURRENT_A_GAIN;
    cal->i_b = (raw->i_b - CURRENT_B_OFFSET) * CURRENT_B_GAIN;
    cal->i_c = (raw->i_c- CURRENT_C_OFFSET) * CURRENT_C_GAIN;
}

The key to remember is that if you only work with pointers to structures, you need to dynamically allocate those structures before you use them the first time.

I have been working on this today and only now have I realised the significance of your last sentence @WBahn

I had simply written:

measurements *raw;  // Contains the raw ADC conversion results
measurements *cal; // Contains the calibrated measurements

and then started using these pointers without ever allocating memory for them to point at. This has thrown up a couple of further questions for me. The code compiled and ran fine and I spent a lot of time today trying to figure out why I wasn't getting the results I expected.
If I simply declared a the two pointers as above and then started trying to assign values to members of the struct using the -> operator, would the program simply write that data to whatever random location that pointer happened to be pointing at since I hadn't assigned any memory for it? Also, is there a reason why the compiler doesn't warn you that you've created a pointer to "nothing"?
Finally, I have seen two ways to solve this problem and I am a little unsure of the pros and cons. Option A, initialise an instance of the struct first, then make a pointer to it:

measurements raw;
measurements * p_raw = &raw;

Or option B, malloc() memory of the required size:

measurements * raw = (measurements*)malloc(sizeof *raw);

As I understand it, option A would be best since the size of raw doesn't change at runtime so I may as well let the compiler allocate it for me. However, if I used option B, would the compiler realise that the size of raw never changes and just allocate the memory at compile time anyway?

 

Storage is allocated if you simply declare:

measurements raw;
measurements cal;

 

I have been working on this today and only now have I realised the significance of your last sentence @WBahn

I had simply written:

measurements *raw;  // Contains the raw ADC conversion results
measurements *cal; // Contains the calibrated measurements

and then started using these pointers without ever allocating memory for them to point at. This has thrown up a couple of further questions for me. The code compiled and ran fine and I spent a lot of time today trying to figure out why I wasn't getting the results I expected.

Engineering is both an art and a science. The art is learning from your own mistakes and the science is learning from the mistakes of others. Textbooks and things like my prior post represent the science. You chose to practice the art.

Truth be told, practicing the art is a much more effective teacher, albeit she's a pretty harsh bitch.

The first time I ran afoul of this (not properly allocating memory and initializing pointers) was back in 1991 (back before the internet was on my radar) and I spent sixteen hours trying to figure out what was happening -- and I mean sixteen hours in which my butt did not break contact with the chair. Of course, this was because the assignment was due the next day. The next time I made the mistake it took me an hour and a half to find it. I still make this mistake from time to time, but now it seldom takes me more than five minutes to spot it and I can usually spot the mistake in other people's code without running it -- I'm that tuned in to looking for it as a matter of habit.

If I simply declared a the two pointers as above and then started trying to assign values to members of the struct using the -> operator, would the program simply write that data to whatever random location that pointer happened to be pointing at since I hadn't assigned any memory for it? Also, is there a reason why the compiler doesn't warn you that you've created a pointer to "nothing"?

If the pointers are local variables, then they are uninitialized and if their values happen to be within the memory space your program has been assigned the operating system will be more than happy to let your program do whatever it does.

One thing about C, in general, is that it assumes you know what you are doing and so if it can compile the code, it will. In exchange for getting the authority to write extremely powerful programs, you have to accept that it is more than willing to give you plenty of rope with which to hang yourself.

Finally, I have seen two ways to solve this problem and I am a little unsure of the pros and cons. Option A, initialise an instance of the struct first, then make a pointer to it:

measurements raw;
measurements * p_raw = &raw;

Or option B, malloc() memory of the required size:

measurements * raw = (measurements*)malloc(sizeof *raw);

As I understand it, option A would be best since the size of raw doesn't change at runtime so I may as well let the compiler allocate it for me. However, if I used option B, would the compiler realise that the size of raw never changes and just allocate the memory at compile time anyway?

A better way to write this one is:

measurements *raw = (measurements*) malloc(sizeof(measurements));

I prefer to declare the structure and then a typedef for that structure. After that, I use the structure.

 

I have been working on this today and only now have I realised the significance of your last sentence @WBahn


I had simply written:

measurements *raw;  // Contains the raw ADC conversion results

measurements *cal; // Contains the calibrated measurements

and then started using these pointers without ever allocating memory for them to point at. This has thrown up a couple of further questions for me. The code compiled and ran fine and I spent a lot of time today trying to figure out why I wasn't getting the results I expected.

If I simply declared a the two pointers as above and then started trying to assign values to members of the struct using the -> operator, would the program simply write that data to whatever random location that pointer happened to be pointing at since I hadn't assigned any memory for it? Also, is there a reason why the compiler doesn't warn you that you've created a pointer to "nothing"?

Turn up your warnings. Compiling with gcc's -Wall option would have caught that one.

Finally, I have seen two ways to solve this problem and I am a little unsure of the pros and cons. Option A, initialise an instance of the struct first, then make a pointer to it:

measurements raw;

measurements * p_raw = &raw;

Or option B, malloc() memory of the required size:

measurements * raw = (measurements*)malloc(sizeof *raw);

As I understand it, option A would be best since the size of raw doesn't change at runtime so I may as well let the compiler allocate it for me. However, if I used option B, would the compiler realise that the size of raw never changes and just allocate the memory at compile time anyway?

The p_raw declaration isn't really necessary. Just pass the address of the structure and you're done.

And you don't need to cast the result of malloc either. In C, a void* will always implicitly cast to a pointer to whatever.

Here's an example of some common use cases btw:

#include <stdlib.h> // malloc, free
#include <memory.h> // memset

typedef struct
{
  int data;
}
thing;

void increment(thing* value)
{
  ++value->data;
}

int main(void)
{
/* Option #1 */

  thing local = { 0 }; // Don't forget to initize member(s)
  increment(&local);

/* Option #2 */

  thing* dynamic = malloc(sizeof(thing));
  memset(dynamic, 0, sizeof(thing)); // Zero-initialize all bytes
  increment(dynamic);
  free(dynamic); // Free memory when done

/* Option #3 */

  thing* initialized = calloc(1, sizeof(thing)); // Also initializes all bytes to zero
  increment(initialized);
  free(initialized); // Free memory when done
}

 

Turn up your warnings. Compiling with gcc's -Wall option would have caught that one.

A lot of the compilers for embedded systems have very few options that they support.

And you don't need to cast the result of malloc either. In C, a void* will always implicitly cast to a pointer to whatever.

It's not required, but I recommend it (and there are people who recommend against it -- it can be argued either way).

I don't like assigning void pointers unless the reason I am doing so is specifically because it's a void pointer.

If I do

Bob *bobPtr;

bobPtr = malloc(sizeof(Sue));

This will work just fine -- no warnings even with Wall because, as you say, it will implicitly cast it, as is required by the language spec.

But if I do

Bob *bobPtr;
bobPtr = (Sue *) malloc(sizeof(Sue));

I will get a warning because now I'm trying to assign a value of type (Sue *) to a variable of type (Bob *).

I WANT that warning!

 

A lot of the compilers for embedded systems have very few options that they support.

Ah, good point. Compatible microprocessors are getting cheaper and smaller though, so hopefully that won't be the case for much longer. (GCC for Arm embedded for example.)

It's not required, but I recommend it (and there are people who recommend against it -- it can be argued either way).


I don't like assigning void pointers unless the reason I am doing so is specifically because it's a void pointer.


If I do


Bob *bobPtr;


bobPtr = malloc(sizeof(Sue));


This will work just fine -- no warnings even with Wall because, as you say, it will implicitly cast it, as is required by the language spec.


But if I do


Bob *bobPtr;

bobPtr = (Sue *) malloc(sizeof(Sue));


I will get a warning because now I'm trying to assign a value of type (Sue *) to a variable of type (Bob *).


I WANT that warning!

Another approach that eliminates the need for such warnings:

bobPtr = malloc(sizeof(*bobPtr));

It's completely safe because the compiler doesn't actually dereference the pointer within the sizeof expression.

It can also be used declarations:

Bob *bobPtr = malloc(sizeof(*bobPtr));

 

Thanks a lot to all who have taken the time to contribute. I have learned a great deal from this thread.

I had been doing all of this on a microprocessor in the lab. I decided to install an IDE on my home PC just to check that some of the functions I had written were mathematically correct and that was when I discovered this issue. The GCC compiler in Codeblocks threw up a warning that the TI compiler for the microprocessor did not which helped me figure out this issue.

All in all, I'm glad I made this mistake and I'm glad I decided to test some functions in CodeBlocks. Without that compiler warning and this thread, I would have been chasing the strange behaviour of the microprocessor for a very long time.

 

Another approach that eliminates the need for such warnings:

bobPtr = malloc(sizeof(*bobPtr));

I go a bit back and forth on this one. Yes, it ensures that the pointers are compatible, but it doesn't help catch when I used bobPtr when I meant to use suePtr, it just ensures that my mistake is internally consistent and thus can't get caught by the compiler.

I prefer to use the data type in the sizeof macro. It is usually the case that I know what data type I want to allocate for and that I won't make a mistake about that, much more likely that I will mess up and use the wrong variable, especially if I'm doing a copy/paste/modify. I think it also makes my code more self-documenting.

Having said that, it does open up a maintainability issue if I change datatypes later. For instance

int *fred;
...
fred = (int *) malloc(sizeof(int));

if I later change fred to a long *, now I have to chase down and change the allocations.

So it's pros and cons either way and my stance and preference are mutable. But since I have a strong preference for coding in a way that turns logic errors into syntax errors (or warnings), I'll probably stick with this for the foreseeable future, especially since changing data types on pointers is pretty rare for me since they are usually pointers to structures, though I do occasionally rename structures, but when I do that I can usually do a global search and replace to update the code.