21
\$\begingroup\$

A firmware style question concerning C.

I have some legacy code I am tidying up. One of the nasty features is global variables being scattered through the source files. I can make a few of them locals, which is fine. However, how to deal with the rest. I favour creating a struct and putting them inside it.

I know that there is theoretically another level of indirection when accessing them, but from a style POV is this better or worse than putting them in files globals.c and/or globals.h?

\$\endgroup\$
9
  • 3
    \$\begingroup\$ Dupe? stackoverflow.com/questions/2868651/… "Only declarations in .h files and the definitions in .c files." \$\endgroup\$ – Tony Stewart EE75 Nov 19 '19 at 14:26
  • 1
    \$\begingroup\$ @TonyStewartSunnyskyguyEE75 Sort of. The accepted answer there is use a struct. Still, there are even more evil things in the code, including code in #define and implicit operator priority being used eg x = y/1/2/3/4; \$\endgroup\$ – Dirk Bruere Nov 19 '19 at 14:37
  • 5
    \$\begingroup\$ Not an answer but an opinion: I prefer to have "globals" grouped in some way, like typedef struct { static const int baudrate = 115200; static const int TX_Pin = 11; static const int RX_Pin = 12; } UART; That way I can call UART::TX_Pin or something like that (pseudo code) This also allows the different groups to be placed where they best fit (no more need for globals.h ) \$\endgroup\$ – Florian Humblot Nov 19 '19 at 14:54
  • 1
    \$\begingroup\$ @DirkBruere Thought as much. Best way to move forward really is to get rid of as much as you can but document copiously where you can't. \$\endgroup\$ – John Go-Soco Nov 21 '19 at 14:13
  • 1
    \$\begingroup\$ Dirk, did you forget to accept an answer? \$\endgroup\$ – Marcus Müller Dec 18 '19 at 19:17
13
\$\begingroup\$

If you can't get rid of the globals, I'd say you should only pack them together in a struct if they are actually related. If not, then I'd keep them separate or in smaller structs.

Also, I wouldn't like a globals.h file either. Keep them at the top of the source file where they most belong. This way when navigating through the code, you likely stay in the place where you were or go to the place you likely wanted to go.

\$\endgroup\$
5
  • 2
    \$\begingroup\$ Also: putting them in a struct without neccessity might misslead future programmers (including yourself) to allocate multiple instances of such structs; and then it gets really confusing \$\endgroup\$ – Curd Nov 19 '19 at 14:38
  • \$\begingroup\$ @Curd On the other hand, if the code is written modular enough, it allows easily instantiating such structs multiple times without being a nuisance. \$\endgroup\$ – Bergi Nov 20 '19 at 0:15
  • \$\begingroup\$ But from the context I assume that it is not only unlikely that it is neccessary but even harmful if those structs existed mulitple times. \$\endgroup\$ – Curd Nov 20 '19 at 13:27
  • \$\begingroup\$ This sounds like the most pragmatic approach to me. The OP clearly would prefer not to have any sort of globals, so if they could be removed without a huge amount of refactoring, the question wouldn't be asked. \$\endgroup\$ – John Go-Soco Nov 21 '19 at 13:34
  • 2
    \$\begingroup\$ In general there is nothing wrong with putting certain global variables together in a struct - if they are related to each other (e.g. belonging to the same interface). It's no good idea, however, putting any globals together in a struct for no other reason than being globals. \$\endgroup\$ – Curd Nov 22 '19 at 14:32
24
\$\begingroup\$

It is well-known bad practice to have a "super header" file like "globals.h" or "includes.h" etc, because in addition to globals being bad in the first place, this also creates a tight coupling dependency between every single, unrelated file in your project.

Lets say you have a PWM driver and a RS-232 debug print module. You want to grab the RS-232 debug print module and re-use it in another project. Suddenly you find yourself needing some PWM.h which you haven't got a clue about what it is or where the need comes from. You'll find yourself asking why on earth you need a PWM driver to run RS-232.

And then we haven't even considered re-entrancy of this messy global struct. Lets not even do that.


The correct way to untangle global spaghetti would rather be something like this:

  • Is the variable used in the first place? If not, remove. (This is quite common)
  • Can the variable be moved to local scope inside a function?
  • Can the variable be moved to local .c file scope by making it static? Can you reduce access to it from outside the .c file by implementing setter/getter functions?

If all of the above failed, then the variable you find yourself looking at should either be a memory-mapped hardware register part of a register map, or it is some real-time critical dirty fix that was added during maintenance.

\$\endgroup\$
7
  • 7
    \$\begingroup\$ What's wrong with putting related variables together in a struct though? Let's say you want to grab the PWM driver module and re-use it in another project where you need to instantiate two of them. \$\endgroup\$ – Bergi Nov 20 '19 at 0:20
  • 2
    \$\begingroup\$ @Bergi No problem at all if they are in the .c file. If they are global in a .h file, placing them in a struct is no obvious improvement. If they belong together they could be in a struct, regardless of where instances of that struct are declared. \$\endgroup\$ – Lundin Nov 20 '19 at 7:25
  • 2
    \$\begingroup\$ @danmcb: I guess with link-time optimization it hopefully doesn't cost any performance, and stops you from taking the address of the global. IDK if some global function call is much more readable than a variable name, depending on the names chosen, but it does make it look different from other variables. Making it a static file-scope with getter/setter doesn't really make things better if it's still accesses all over the place spaghetti-style. This answer is suggesting that's useful if most of the use can be contained to one file, but you still need some outside access. \$\endgroup\$ – Peter Cordes Nov 21 '19 at 3:47
  • 1
    \$\begingroup\$ Worth pointing out that "local scope" can still have static storage class. Just beware that a non-const initializer is no longer a compile-time error like it is at global scope in C, and instead costs you runtime performance to check an already-initialized flag. (And on the first call to do thread-safe init.) \$\endgroup\$ – Peter Cordes Nov 21 '19 at 3:49
  • 6
    \$\begingroup\$ @PeterCordes One obvious benefit with setter/getters is that lots of file scope variables in embedded systems are there because they are used for communicating with an ISR. Then roughly 90% of all embedded software written fails to properly protect such variables against race conditions. When the programmer eventually discovers that their rare intermittent bugs are caused by this, they need to implement re-entrancy and then the setter/getters are the perfect place to do this. Impossible to do that properly in spaghettiware that lets other translation units communicate directly with the ISR. \$\endgroup\$ – Lundin Nov 21 '19 at 7:47
14
\$\begingroup\$

It would work to define a struct that you instantiate as the single global variable. Accesses on the form 'the_global.the_var' won't add run-time overhead and can clarify that it's indeed a global. As https://stackoverflow.com/questions/2868651/including-c-header-file-with-lots-of-global-variables mentions, it saves you from separate declarations and definitions.

Personally I wouldn't bother to make a struct, instead preferring to sort globals into the header files where I feel they logically belong and use a common prefix for each header file. Example: File calculate.h declaring "extern int calc_result;" and calculate.c defining "int calc_result;"

Other variables get away with being file local, i.e. "static int result;" in the .c file.

Since you have legacy code that I guess you won't work a lot with beyond tidying it up, I'd say that the quickest solution that makes for a clear structure is the best.

\$\endgroup\$
1
  • 12
    \$\begingroup\$ +1 don't over do it on legacy code. Of course you could do it better with a rewrite etc (even the original authors likely could), but let me assure you there is plenty more code to write in your career. Focus on getting it to a more maintainable stage as quickly as possible and move on. \$\endgroup\$ – Jon Nov 19 '19 at 16:04
10
\$\begingroup\$

Actually, it's not better, the only advantage is that you 'know' which are global. So you have all globals in one place, but still they are scattered around.

C

For each global:

  • Find the initial file where it is created or used.
  • Make it static in that file
  • In case the global is used in many places, use it externally from other files (using the extern keyword).
  • But the preferable way is to pass the variable to the places where it's needed.

C++

For each global:

  • Find the initial place where it is created or used.
  • Find the class it fits best to.
  • Move it to that class (as field, connected to an object).
  • Now create a Get/Set method (functions). Preferably the Set method is protected or private).
  • Instead of Get and Set methods you also can pass them through method arguments.
  • Use Get/Set methods from other classes.
  • In some cases you will see it cannot a field of a class, for example when there can be only 'one'. In this case, use the Singleton design pattern. (Beware of this; it's still kind of a global, so make sure in the future you will never make more of them; otherwise it's a global space in disguise).
\$\endgroup\$
12
  • 5
    \$\begingroup\$ @DirkBruere static file scope is a perfectly fine way of private encapsulation in single-core, single-process MCU systems. Most of the time, you don't need OO private encapsulation, but if you do, look at the concept of opaque type over at SO. \$\endgroup\$ – Lundin Nov 19 '19 at 14:49
  • 1
    \$\begingroup\$ "C ... Make it static in that class" - should that be "C ... Make it static in that file"? \$\endgroup\$ – Digital Trauma Nov 20 '19 at 0:47
  • 2
    \$\begingroup\$ @DigitalTrauma OO concepts like classes are language agnostic. C doesn't have a class keyword but you can have classes in any program language. It is a program design term. \$\endgroup\$ – Lundin Nov 20 '19 at 7:27
  • 2
    \$\begingroup\$ @DirkBruere, I don't know the specifics of your environment, but going from C to C++ does not in general have to mean a total rewrite. It's not generally too difficult to translate code from C to the common subset of C and C++, then you can switch the compiler over and start using C++ features in new code. \$\endgroup\$ – Peter Green Nov 20 '19 at 11:22
  • 5
    \$\begingroup\$ I feel stackoverflow often feeds a false dichtomy that one should write "plain C" or "modern C++" and that intermediate points between the two are somehow invalid. \$\endgroup\$ – Peter Green Nov 20 '19 at 11:27
6
\$\begingroup\$

The big advantages to putting global variables in a struct are:

  1. It's clear when you are using a global vs. a local variable.
  2. It's impossible to mask by accident a global variable with a local variable of the same name.

Lesser avantages are :

  1. It's easier to debug overflows in a global array or memory buffer because the memory layout can be deduced from the struct.
  2. The global structure can be very simply saved to / loaded from non-volatile storage.

Sensible use of global variables can reduce stack requirements, but increases the risk of error if their usage is not well documented and the rules respected.

\$\endgroup\$
2
\$\begingroup\$

Why should there be another level of indirection accessing variables in a struct? If you declare your struct like this:

typedef struct tagUART {
    int field1;
    int field2;
} UART;

Then declare a global:

UART uart;

Instead of declaring these globals:

int uartField1;
int uartField2;

The compiler, as crappy as it could be, has no excuse to introduce another level of indirection if you access your variable through uart.field1 than it would with an access to uartField1. The compiler knows the starting address of your structure, and the struct declaration defines the field offset within the structure. The compiler knows everything at the reference point.

Now, the extra level of indirection would happen if you start passing pointers to uart around as function arguments, and the function calls do not get inlined. There, the compiler loses the fact that "Hey, the fields have a well-known address".

\$\endgroup\$
1
\$\begingroup\$

I know that there is theoretically another level of indirection when accessing them,

Well it depends, a really dumb compiler may introduce extra levels of indirection for a structure, but a smart compiler can actually produce better code if a structure is involved.

Contrary to other answers the compiler DOES NOT know the address of global variables. For global variables in the same compilation unit it knows their location relative to each other, but not their absolute location. For variables in other compilation units it doesn't know anything about their location in either absolute or relative terms.

Instead the code generated by the compiler will have placeholders for the locations of the global variables, those will then be replaced by actual locations when the final address is determined (traditionally when the program is linked, but sometimes not until it is run).

Furthermore, while there are some CPUs (like the 6502) that can access a global memory location directly without any prior set-up there are many that can't. On arm for example, to access a global variable the compiler must typically first load the address of the variable into a register, either using a literal pool or, on more recent versions of arm, using movw/movt. Then access the global variable using a register-relative load instruction.

This means for variables outside the compilation unit accessing multiple elements of the same global structure is likely more efficient than accessing individual global variables.

To test this I fed the following code into godbolt with ARM gcc 8.2 and -O3 optimization.

extern int a;
extern int b;

struct Foo {
    int a;
    int b;
};

extern struct Foo foo;

void f1(void) {
    a = 1;
    b = 2;
}

void f2(void) {
    foo.a = 1;
    foo.b = 2;
}

This resulted in

f1:
        mov     r0, #1
        mov     r2, #2
        ldr     r1, .L3
        ldr     r3, .L3+4
        str     r0, [r1]
        str     r2, [r3]
        bx      lr
.L3:
        .word   a
        .word   b
f2:
        mov     r1, #1
        mov     r2, #2
        ldr     r3, .L6
        stm     r3, {r1, r2}
        bx      lr
.L6:
        .word   foo

In the case of f1 we see that the compiler loads the addresses of a and b separately from literal pools (the addresses in the literal pools will be filled in later by the linker).

However in the case of f2 the compiler only has to load the address of the structure from the literal pool once, better still because it knows the two variables are next to each other in memory it can write them with a single "store multiple" instruction rather than two separate store instructions.

but from a style POV is this better or worse than putting them in files globals.c and/or globals.h?

IMO that depends if the variables are actually related or not.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.