0

I have heard many times that global variables can be "dangerous" if not used cautiously. And that one way of protecting globals (or indicate reader to which section of code those relate to) is to use struct and put all globals inside as members.

With term "globals protection" I refer to not being able to accidentally modify value of global from some xxx-source file. With struct, you clarify to reader (or yourself) where members of this global struct might be used and therefore there is less uncertainty about variable functionality.

I tried to implement this on my project where I thought this was a good idea since I plan to use more ISRs in future. In order to prevent unclear intentions or buggy code, used upper method of global variables protection. But came to conclusion that new organization of variables looks even worse if one has to access struct inside struct, as code readability is decreased for great portion (my opinion).

Here is snippet of code I use for button debouncing. XORing two members looks like a nasty long "snake" but could be written as change = new ^ old if certain variable organization (inside structures) wouldn't be needed.

/*** HEADER FILE ***/

/** Button state structure **/
typedef struct {
    uint16_t old;
    uint16_t new;
    uint16_t change;
} state;

/** Protect ISR globals within structure **/
typedef struct {
    state buttonState;
    uint16_t isPortStable;
    uint16_t countStableStates[BTN_COUNT];
    uint32_t tick;
} debounceISR;

debounceISR DB;   // !! Keep struct name short to affect readability as little as possible !!

/*** SOURCE FILE ***/

/* Read keyboard and check for transitions */
DB.buttonState.new = BTN_PORT;
DB.buttonState.change = DB.buttonState.new ^ DB.buttonState.old;
DB.buttonState.old = DB.buttonState.new;

On the long run I want to prevent global variables to interfere without user (coder) being aware and cause complicated problems as number of ISRs (and therefore globals) increase in one project.

Are there any alternative options to such needs? Should I tend to use pointer and static variables in order to avoid globals when using ISRs?

Keno
  • 126
  • 8
  • What is your understanding of what "protecting globals" means? It isn't obvious how using a struct would protect anything about the globals. Not saying there aren't good reasons for using structs but to be able to answer your question we need to understand what the exact goals of this "protect globals" are. – kaylum Nov 15 '21 at 21:10
  • It does not protect anything. It does not protect anything. BTW your DB **has** to be `volatile` as changes are not seen by the compiler (ISRs are not called) – 0___________ Nov 15 '21 at 21:10
  • 5
    I've never heard of making globals members of a global struct as a mitigation for the problems related to globals. I guess doing so would address most of the namespace issues related to globals, but I have never considered those to be the primary problem. – John Bollinger Nov 15 '21 at 21:10
  • 1
    There is nothing wrong with using globals when appropriate. Of course, if you can limit their scope (by marking them as `static`) then do it, it will prevent "interference" with them in other translation units. – Eugene Sh. Nov 15 '21 at 21:11
  • On the other hand, I have heard of and have sometimes practiced creating a struct type encompassing data that might otherwise have been placed in global variables, and avoiding globals by passing around an instance of that struct or (more often) a pointer to one to the functions that need it. – John Bollinger Nov 15 '21 at 21:14
  • @kaylum Yeah sorry about that. I edited the question, however I don't know whether that clarifies term "globals protection" to you. – Keno Nov 15 '21 at 21:25
  • 3
    I wouldn't say globals are "dangerous". They are *potentially confusing*, especially if you have a lot of them, especially if they're both read from and written to from multiple, disparate parts of your code. There's no rule (in most people's style guides) against globals; there's merely a suggestion that they be used carefully, and only when needed. – Steve Summit Nov 15 '21 at 21:33
  • Arranging globals inside a special struct isn't going to make them safer; at best it's going to give you a sort of poor-man's namespace management. This might lead to less confusion, although in the long run it might be nothing more than a nuisance. – Steve Summit Nov 15 '21 at 21:33
  • @SteveSummit What about using a pointer to access (read from/write to) a `static` variable externally, used inside one specific ISR? – Keno Nov 15 '21 at 21:38
  • If you really want to make globals "safer", you should (1) make a rule to have as few of them as necessary and (2) make a rule to make sure that they're constant or relatively so, set to a value either once on program initialization, or at well-defined times, and from one or a very few well-defined spots in the code. If you have enough global variables that you can put them into their own struct, that suggests you may have too many of them already. – Steve Summit Nov 15 '21 at 21:39
  • 2
    @Keno *What about using a pointer...* You can do that. I've done that. It's not a bad idea, but I wouldn't say it's a super-great idea, either. Moving on from "poor man's namespace", it's sort of a "poor man's encapsulation class", although without the benefit of getters and setters. It might buy you a little, it might cost you a little. – Steve Summit Nov 15 '21 at 21:42
  • 1
    @Keno Finally, my comments are all from the perspective of a "high-level" programmer. My code never has ISR's, so I don't have to worry about how to communicate with them, or whether globals are a good/bad way to do so. Many of the "rules" are subtly or significantly different for embedded programming. There are plenty of embedded C programmers here; you might (might?) pick up their advice more effectively by adding an [embedded] tag. – Steve Summit Nov 15 '21 at 21:45
  • @JohnBollinger I believe you, but check [this question](https://electronics.stackexchange.com/questions/468017) and first answer to [this one](https://stackoverflow.com/questions/2868651). – Keno Nov 15 '21 at 21:45
  • 1
    In the context of globals and especially for async contexts such as ISR the term "protect" usually means preventing inconsistent/unexpected read and writes. For example if an ISR and the non-ISR context both modify multiple global values then the total state may be inconsistent (half written) as ISR can interrupt non-ISR in the middle of updating the state. Having structs does not help with this. You are more talking about good coding style to make the code easier to read/understand/maintain and hence less buggy. All good things but wouldn't classify that as "protection" in the common sense. – kaylum Nov 15 '21 at 21:48
  • @kaylum True, I used term "protection" in a bad way. I was (probably) referring to what you said in penultimate sentence. – Keno Nov 15 '21 at 21:52
  • @Keno, that's a suggestion for mitigating the code duplication involved in declaring the globals everywhere they are accessed, but defining them only once. I don't consider that to be an inherent problem of globals. – John Bollinger Nov 15 '21 at 21:52
  • 1
    My best guess is that they meant putting them in structures will reduce global namespace pollution, and thus less confusion about which instance of `counter` you're interacting with, but global `struct`s provide no protection from race conditions that otherwise "bare" global variables have. – Russ Schultz Nov 16 '21 at 01:54
  • 1
    Does the `DB` structure need to be accessible outside this source file? If not, simply make it `static` — unless there's a rule I'm unaware of about not doing that. Then the structure is only accessible to the code inside the source file. Make as few things visible outside the source file as possible — everything that can be `static` should be `static`. That's a good rule for user-land code; I don't know of a reason why it becomes worse for kernel-land code. – Jonathan Leffler Nov 16 '21 at 06:54

1 Answers1

2

Read https://www.embedded.com/a-pox-on-globals.

Your starting premise is a fallacy, you still have a global, it just happens to be a structure. It is insufficient to suspect globals are "bad", you have to understand what the problems are and how to solve them. It is not one single problem, and they cannot be solved by one single technique, and in this case, the technique you propose is not a solution at all.

The ISR shared and state data objects should be declared static in the same source file as the ISR itself (in a structure if you wish and that makes sense - i.e. the variables are related). If the data objects are shared, then the same source should include accessor functions, and it is the accessors that you declare in the header, not the data objects.

The accessors can then include code to deal with data consistency and atomic access, as well as imposing read/write restrictions and range checking etc.

Moreover when you need to debug access to any shared data object, the accessor gives you a single place to put a breakpoint to catch any and all access.

As well as the A Pox in Globals article above, you might also read https://www.embedded.com/introduction-to-the-volatile-keyword/. Your ISR shared data must be declared volatile at the very least.

Clifford
  • 88,407
  • 13
  • 85
  • 165