I had an interrupt functionality implementation in my embedded project with global variables. The interrupt checks PWM signal duty cycle and sets the value of a bool var to true if the value is correct. The main code then uses this variable to output HIGH or LOW on a specified pin.

I wanted to modularize this functionality and limit the scope of the variables. I saw an approach of making an "object" in a separate .c file. This way I defined static variables and methods to access and change values of the variables. I have a couple bool variables, methods to get their value and two universal methods to change the value of an input variable (to reduce code duplication).

The resulting implementation works but I would like some feedback if my approach is appropriate.

My module .c file looks like this (the number of variables could be higher than in this example):

static bool var1 = false;
static bool var2 = false;

bool *var1_get(void) {
  return &var1;
}

bool *var2_get(void){
  return &var2;
}

void var_set(bool *var_ok) {
  /* signal PWM is good */
  *var_ok = true;
}
void var_reset(bool *var_ok) {
  /* signal PWM is bad */
  *var_ok = false;
}

The interrupt uses the set and reset functions in the main.c for each variable at a time (different interrupts for different input signals):

// capture_val is the PWM signal duty cycle value
if ((capture_val > 88) && (capture_val < 92)) {
  var_set(var1_get());
} else {
  var_reset(var1_get());
}

and in the main while(1) loop the value of the variable to output on a specified pin is checked:

if (*var1_get() == false) {
  HAL_GPIO_WritePin(GPIOC, OUT_OK_Pin, GPIO_PIN_RESET);
} else {
  HAL_GPIO_WritePin(GPIOC, OUT_OK_Pin, GPIO_PIN_SET);
}

That means that with _get function I get the address of the variable to change its value in the interrupt and in the while loop I check its value with the same method. As I understand it I used a pointer function, but I saw it is usually defined differently to use functions dynamically (which is not what I want to do here). I don't really have a lot of experience with pointer functions and never saw an implementation like this which leads me to a couple of questions:

• Is my approach of implementation fine or should I just use static top-level variables in the main.c and ditch the module I made?

• Are the *var_get() functions fine to use as I defined them or is there a better standard of defining such functions? Is there a different approach that would have the same functionality of the module but with more appropriate implementation?

I'm mostly self-taught, so I appreciate any feedback on my approach.

I had an interrupt functionality implementation in my embedded project with global variables. The interrupt checks PWM signal duty cycle and sets the value of a bool var to true if the value is correct. The main code then uses this variable to output HIGH or LOW on a specified pin.

I wanted to modularize this functionality and limit the scope of the variables. I saw an approach of making an "object" in a separate .c file. This way I defined static variables and methods to access and change values of the variables. I have a couple bool variables, methods to get their value and two universal methods to change the value of an input variable (to reduce code duplication).

The resulting implementation works but I would like some feedback if my approach is appropriate.

My module .c file looks like this (the number of variables could be higher than in this example):

static bool var1 = false;
static bool var2 = false;

bool *var1_get(void) {
  return &var1;
}

bool *var2_get(void){
  return &var2;
}

void var_set(bool *var_ok) {
  /* signal PWM is good */
  *var_ok = true;
}
void var_reset(bool *var_ok) {
  /* signal PWM is bad */
  *var_ok = false;
}

The interrupt uses the set and reset functions in the main.c for each variable at a time (different interrupts for different input signals):

// capture_val is the PWM signal duty cycle value
if ((capture_val > 88) && (capture_val < 92)) {
  var_set(var1_get());
} else {
  var_reset(var1_get());
}

and in the main while(1) loop the value of the variable to output on a specified pin is checked:

if (*var1_get() == false) {
  HAL_GPIO_WritePin(GPIOC, OUT_OK_Pin, GPIO_PIN_RESET);
} else {
  HAL_GPIO_WritePin(GPIOC, OUT_OK_Pin, GPIO_PIN_SET);
}

That means that with _get function I get the address of the variable to change its value in the interrupt and in the while loop I check its value with the same method. As I understand it I used a pointer function, but I saw it is usually defined differently to use functions dynamically (which is not what I want to do here). I don't really have a lot of experience with pointer functions and never saw an implementation like this which leads me to a couple of questions:

• Is my approach of implementation fine or should I just use static top-level variables in the main.c and ditch the module I made?

• Are the *var_get() functions fine to use as I defined them or is there a better standard of defining such functions? Is there a different approach that would have the same functionality of the module but with more appropriate implementation?

I'm mostly self-taught, so I appreciate any feedback on my approach.

• c
• module
• embedded
• function-pointers

Share Improve this question Follow asked Mar 3 at 14:09 marusmarus 2344 bronze badges 5

• 1 Returning the address of the variable is not what you usually expect from a getter because this allows any read and write access. The main point of getter and setter functions is to encapsulate the access to the variable. If necessary you can do more than simply passing the value. – Bodo Commented Mar 3 at 16:30
• 1 Variables shared with an ISR need to be declared volatile. You also have to ensure that they get updated with an atomic access by disassembling the code. – Lundin Commented Mar 4 at 7:40
• @Lundin Thank you for reminding me about volatile keyword. As for the atomic access, I'm not too familiar about it (I understand the concept but not how to ensure it) so it's something I'll be looking into it. – marus Commented Mar 4 at 9:01
• 1 Basically if an interrupt writes to a shared resource while the main program is reading that resource - if either the read or the write takes place in multiple instructions, then you have a "race condition" bug which will result in a corrupted value. Atomic access is one way to avoid that bug. Temporarily disabling the interrupt while reading the variable is another. – Lundin Commented Mar 4 at 9:48
• For a bit more context, I am using STM32F4 family MCU and implemented timer interrupts with HAL. So after a bit more reading about it I saw that interrupt priority is set by the NVIC for different timers in my case, which avoids race condition. I didn't really think much about it when I was configuring project .ioc and your explanation helped me understand what the generated code is doing. – marus Commented Mar 4 at 14:56

Add a comment  | 

1 Answer 1

Sorted by: Reset to default 3

Returning pointers isn't good because it breaks encapsulation of allowing only the module to read/write the actual variable values. As is, your code allows things like this:

bool *var1 = var1_get();
print("var1=%d\n", *var1);
*var1 = false;
print("var1=%d\n", *var1);

So only return the values and have a separate setter function for each variable that takes the value to set:

bool var1_get(void) {
  return var1;
}

bool var2_get(void){
  return var2;
}

void var1_set(bool val) {
  var1 = val;
}
void var2_set(bool val) {
  var2 = val;
}

And to use:

if ((capture_val > 88) && (capture_val < 92)) {
  var1_set(true);
} else {
  var1_set(false);
}

...

if (!var1_get()) {
  HAL_GPIO_WritePin(GPIOC, OUT_OK_Pin, GPIO_PIN_RESET);
} else {
  HAL_GPIO_WritePin(GPIOC, OUT_OK_Pin, GPIO_PIN_SET);
}