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.
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);
}