How to organize a complex software: core logic and external interface

[pozz]

I often write some C lines for embedded software (firmware) that will
run into a microprocessor.

I always try to organize the source code into multiple C files in order
to separate different tasks, creating well defined interfaces between
those tasks.
I think splitting tasks/modules is a Good Thing to do during software
developing: the code is more structured and would be simpler to maintain
and understand.

I tend to create one (or more) file that implements the core module of
the application: the internal state of the machine that could
automatically change depending on time, input signals (from ADC or GPIO)
and so on.
Other modules are human-machine-interfaces that take commands/requests
from the user (LCD, PC, buttons, ...) and convert them into commands for
the core module.

IMHO, the separation between core and HMI is another Good Thing. In the
future, if I will have to change the external interface (from LCD
control to PC, for example) I need to rewrite just one well localized
part of the entire code, without touching the core module (that is most
probably a complex code).

This approach works well most of the time, but there are some critical
situations that I'll try to explain just to see if you have some good
suggestions. I will consider a simple example.

Suppose the core module could be in three state G, Y, R and that it
changes automatically from G to Y, from Y to R and from R to G depending
on time. The user interface is composed by three LEDs (one red, one
green and one yellow). Only the LED corresponding to the state is on.

In the core module I can define a system_status() function that returns
the current state. The HMI will have a simple function that is called
continuously:

void
leds_refresh(void) {
if (system_status == STATUS_G) {
led_on(LED_G); led_off(LED_Y); led_off(LED_R);
} else if (system_status == STATUS_Y) {
led_off(LED_G); led_on(LED_Y); led_off(LED_R);
} else if (system_status == STATUS_R) {
led_off(LED_G); led_off(LED_Y); led_on(LED_R);
}
}

What if I want to blink the LED of the previous state for 5 seconds,
before switching to the LED corresponding to the next state?

One solution is to use the same interface between core and HMI module
(system_status() function) and modify leds_refresh():

void leds_refresh(void) {
static int old_status;
int status = system_status();

if (old_status != status) {
if (old_status == STATUS_G) {
led_blink(LED_G);
} else if (status == STATUS_Y) {
led_blink(LED_Y);
} else if (status == STATUS_R) {
led_blink(LED_R);
}
status = old_status;
timer_start();
} else if (timer_expired()) {
if (status == STATUS_G) {
led_off(LED_R); led_on(LED_G);
} else if (status == STATUS_Y) {
led_off(LED_G); led_on(LED_Y);
} else if (status == STATUS_R) {
led_off(LED_Y); led_on(LED_R);
}
}
}

Another possibility is to modifiy the core module and call a
core_event() callback function when the status change. core_event()
will be in the HMI module:

void core_event(int old_status, int new_status) {
if (old_status == STATUS_G) {
led_blink(LED_G);
} else if (status == STATUS_Y) {
led_blink(LED_Y);
} else if (status == STATUS_R) {
led_blink(LED_R);
}
timer_start();
}

void leds_refresh(void) {
if (timer_expired()) {
int status = system_status();
if (status == STATUS_G) {
led_off(LED_R); led_on(LED_G);
} else if (status == STATUS_Y) {
led_off(LED_G); led_on(LED_Y);
} else if (status == STATUS_R) {
led_off(LED_Y); led_on(LED_R);
}
}
}

Another solution is to create other intermediate states GY, YR and RG
when the "main status" changes: if the status changes from G to Y, it
changes temporarily into GY and only after 5 seconds definetely to Y.

What do you prefer? Do you have other more elegant/efficient solutions?

In some cases I prefer the first solution, because I don't have to touch
the core module. In other cases I prefer the second.

I tend to avoid the third solution, because my answer is "NO" to the
question "Should the core module manage more status only for HMI purposes?".

 

[James Kuyper]

On 02/13/2014 06:18 PM, pozz wrote:
....

Suppose the core module could be in three state G, Y, R and that it
changes automatically from G to Y, from Y to R and from R to G depending
on time. The user interface is composed by three LEDs (one red, one
green and one yellow). Only the LED corresponding to the state is on.

In the core module I can define a system_status() function that returns
the current state. The HMI will have a simple function that is called
continuously:

void
leds_refresh(void) {
if (system_status == STATUS_G) {
led_on(LED_G); led_off(LED_Y); led_off(LED_R);
} else if (system_status == STATUS_Y) {
led_off(LED_G); led_on(LED_Y); led_off(LED_R);
} else if (system_status == STATUS_R) {
led_off(LED_G); led_off(LED_Y); led_on(LED_R);
}
}

What if I want to blink the LED of the previous state for 5 seconds,
before switching to the LED corresponding to the next state?

One solution is to use the same interface between core and HMI module
(system_status() function) and modify leds_refresh():

void leds_refresh(void) {
static int old_status;
int status = system_status();

if (old_status != status) {
if (old_status == STATUS_G) {
led_blink(LED_G);
} else if (status == STATUS_Y) {
led_blink(LED_Y);
} else if (status == STATUS_R) {
led_blink(LED_R);
}
status = old_status;
timer_start();
} else if (timer_expired()) {
if (status == STATUS_G) {
led_off(LED_R); led_on(LED_G);
} else if (status == STATUS_Y) {
led_off(LED_G); led_on(LED_Y);
} else if (status == STATUS_R) {
led_off(LED_Y); led_on(LED_R);
}
}
}

Another possibility is to modifiy the core module and call a
core_event() callback function when the status change. core_event()
will be in the HMI module:

void core_event(int old_status, int new_status) {
if (old_status == STATUS_G) {
led_blink(LED_G);
} else if (status == STATUS_Y) {
led_blink(LED_Y);
} else if (status == STATUS_R) {
led_blink(LED_R);
}

That's precisely the kind of situation that a switch() statement is for.

timer_start();
}

void leds_refresh(void) {
if (timer_expired()) {
int status = system_status();
if (status == STATUS_G) {
led_off(LED_R); led_on(LED_G);
} else if (status == STATUS_Y) {
led_off(LED_G); led_on(LED_Y);
} else if (status == STATUS_R) {
led_off(LED_Y); led_on(LED_R);
}

That should also be a switch() statement.

}
}

I prefer your second solution. It doesn't waste time turning on LEDs
that are already on, or turning off LEDs that are already off, or
putting LEDs in blink mode that are already in blink mode. The time
wasted might be negligible, and it might be that the time cannot be put
to any alternative use, but it still bothers me.

 

[buja]

You are right to avoid "intermediate" statuses, the variable old_status is much easier to use and understand.

However, the first blinking solution has a problem: old_status is not initialised when led_refresh() is called the first time, so its value can be anything and that is always bad.

I do not like the second solution. You exit a function with a timer, which is picked up by another function. I think this coupling is not clear and not necessary.

I do agree with using switch statements, makes things much more readable.

But things could be much simpler if you are in full control of the LEDs:

status = system_status();
led_blink(old_status); /* put the blink duration inside led_blink() */
led_off(old_status); /* not even necessary if you control this inside led_blink() */
led_on(status);
old_status = status;

 

[James Kuyper]

You are right to avoid "intermediate" statuses, the variable old_status is much easier to use and understand.

However, the first blinking solution has a problem: old_status is not initialised when led_refresh() is called the first time, so its value can be anything and that is always bad.

It's declared with static storage duration, which means that if it's not
explicitly initialized, it's implicitly zero-initialized. If one of the
status codes is 0, then it's hopefully acceptable to assume the status
starts out with that value - if not, it will need to be explicitly
initialized with whatever status is appropriate. If 0 is not one of the
valid status codes, the first call to led_refresh() will always identify
the current status as a change, and act accordingly, which stands a good
chance of being the right way to do it.

 

[dk]

void leds_refresh(void) {

static int old_status;

int status = system_status();



if (old_status != status) {

if (old_status == STATUS_G) {

led_blink(LED_G);

} else if (status == STATUS_Y) {

led_blink(LED_Y);

} else if (status == STATUS_R) {

led_blink(LED_R);

}

status = old_status;

This will never work unless you correct the above statement to : old_status = status;

 

[buja]

Op zondag 16 februari 2014 15:06:27 UTC+1 schreef James Kuyper:

It's declared with static storage duration, which means that if it's not

explicitly initialized, it's implicitly zero-initialized.

You are right.