19
\$\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\$
  • 3
    \$\begingroup\$ Dupe? stackoverflow.com/questions/2868651/… "Only declarations in .h files and the definitions in .c files." \$\endgroup\$ – Tony Stewart Sunnyskyguy EE75 Nov 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 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\$ – FMashiro Nov 19 at 14:54
  • \$\begingroup\$ @FMashiro: That is C++, not C. \$\endgroup\$ – Laurent LA RIZZA Nov 21 at 12:27
  • 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 at 14:13
23
\$\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
    \$\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 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 at 7:25
  • 1
    \$\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 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 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 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\$
  • 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 at 16:04
10
\$\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\$
  • 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 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 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 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 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 at 14:32
9
\$\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\$
  • 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 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 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 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 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 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\$
3
\$\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\$

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.