patterncMinor
MicroC task switching
Viewed 0 times
switchingmicroctask
Problem
My background for this review is a university course in embedded systems using MicroC and this question. Now my program appears to run ok, but I'd like to know what you think can be improved or if the solution I've done so far is not acceptable for some reason?
```
#include
#include "system.h"
#include "includes.h"
#include "altera_avalon_pio_regs.h"
#include "sys/alt_irq.h"
#include "sys/alt_alarm.h"
#define DEBUG 1
#define HW_TIMER_PERIOD 100 / 100ms /
/ Button Patterns /
#define GAS_PEDAL_FLAG 0x08
#define BRAKE_PEDAL_FLAG 0x04
#define CRUISE_CONTROL_FLAG 0x02
/ Switch Patterns /
#define TOP_GEAR_FLAG 0x00000002
#define ENGINE_FLAG 0x00000001
/ LED Patterns /
#define LED_RED_0 0x00000001 // Engine
#define LED_RED_1 0x00000002 // Top Gear
#define LED_GREEN_0 0x0001 // Cruise Control activated
#define LED_GREEN_2 0x0002 // Cruise Control Button
#define LED_GREEN_4 0x0010 // Brake Pedal
#define LED_GREEN_6 0x0040 // Gas Pedal
/*
* Definition of Tasks
*/
#define TASK_STACKSIZE 2048
OS_STK StartTask_Stack[TASK_STACKSIZE];
OS_STK ControlTask_Stack[TASK_STACKSIZE];
OS_STK VehicleTask_Stack[TASK_STACKSIZE];
// Task Priorities
#define STARTTASK_PRIO 5
#define VEHICLETASK_PRIO 10
#define CONTROLTASK_PRIO 12
// Task Periods
#define CONTROL_PERIOD 300
#define VEHICLE_PERIOD 300
/*
* Definition of Kernel Objects
*/
// Mailboxes
OS_EVENT *Mbox_Throttle;
OS_EVENT *Mbox_Velocity;
// Semaphores
OS_EVENT *aSemaphore;
OS_EVENT *aSemaphore2;
// SW-Timer
OS_TMR *SWTimer;
OS_TMR *SWTimer1;
BOOLEAN status;
/*
* Types
*/
enum active {on, off};
enum active gas_pedal = off;
enum active brake_pedal = off;
enum active top_gear = off;
enum active engine = off;
enum active cruise_control = off;
/*
* Global variables
*/
int delay; // Delay of HW-timer
INT16U led_green = 0; // Green LEDs
INT32U led_red = 0; // Red LEDs
int sharedMemory=1;
void TimerCallback(params)
{
// Post to the semaphore to signal that it's time
```
#include
#include "system.h"
#include "includes.h"
#include "altera_avalon_pio_regs.h"
#include "sys/alt_irq.h"
#include "sys/alt_alarm.h"
#define DEBUG 1
#define HW_TIMER_PERIOD 100 / 100ms /
/ Button Patterns /
#define GAS_PEDAL_FLAG 0x08
#define BRAKE_PEDAL_FLAG 0x04
#define CRUISE_CONTROL_FLAG 0x02
/ Switch Patterns /
#define TOP_GEAR_FLAG 0x00000002
#define ENGINE_FLAG 0x00000001
/ LED Patterns /
#define LED_RED_0 0x00000001 // Engine
#define LED_RED_1 0x00000002 // Top Gear
#define LED_GREEN_0 0x0001 // Cruise Control activated
#define LED_GREEN_2 0x0002 // Cruise Control Button
#define LED_GREEN_4 0x0010 // Brake Pedal
#define LED_GREEN_6 0x0040 // Gas Pedal
/*
* Definition of Tasks
*/
#define TASK_STACKSIZE 2048
OS_STK StartTask_Stack[TASK_STACKSIZE];
OS_STK ControlTask_Stack[TASK_STACKSIZE];
OS_STK VehicleTask_Stack[TASK_STACKSIZE];
// Task Priorities
#define STARTTASK_PRIO 5
#define VEHICLETASK_PRIO 10
#define CONTROLTASK_PRIO 12
// Task Periods
#define CONTROL_PERIOD 300
#define VEHICLE_PERIOD 300
/*
* Definition of Kernel Objects
*/
// Mailboxes
OS_EVENT *Mbox_Throttle;
OS_EVENT *Mbox_Velocity;
// Semaphores
OS_EVENT *aSemaphore;
OS_EVENT *aSemaphore2;
// SW-Timer
OS_TMR *SWTimer;
OS_TMR *SWTimer1;
BOOLEAN status;
/*
* Types
*/
enum active {on, off};
enum active gas_pedal = off;
enum active brake_pedal = off;
enum active top_gear = off;
enum active engine = off;
enum active cruise_control = off;
/*
* Global variables
*/
int delay; // Delay of HW-timer
INT16U led_green = 0; // Green LEDs
INT32U led_red = 0; // Red LEDs
int sharedMemory=1;
void TimerCallback(params)
{
// Post to the semaphore to signal that it's time
Solution
Niklas, I'll make a few comments.
Firstly the code is rather chaotic. It is difficult to make out what is going
on and what the requirement is. I'd expect to see some sort of comment at the
top that summarises the purpose of the overall system. I cannot compile it
because of all the headers, so I don't know how cleanly it builds. Do you
have plenty of compiler warnings turned on?
I don't follow exactly what is required of you. If your 2 tasks are just
alternating then one semaphore and one timer would seem to be all you need,
not two of each. The timer wakes up the first task via the semaphore; the
first task waits on the semaphore, sends a message to the 2nd then waits
on a reply from the 2nd; the 2nd task waits on a message queue from the 1st
task, does its stuff then sends a reply to the first.
Some specific comments:
-
Error handling is lacking. Often you don't check errors or you check for
success but continue on failure. Error handling in an embedded system is
difficult. As this is an exercise, I suggest that on error you should turn
on all LEDs or make them say ERR or something like that and then hang in an
infinite loop. A real system would want to be able to recover (eg restart
via a watchdog timer), but I think that is beyond the scope of your project.
-
Some of your variable and function names are not so good.
Ditto,
2 timers and
-
There are many flags and some functions and variables that are unused.
When submitting code for review your should omit such parts. This also
applies to commented-out lines of code which give the reader a poor
impression of the completeness/readiness of your code.
-
Global variable
value.
-
Your
interrogated by
-
You have numeric constants scattered throughout your code. This is
generally considered bad practice, (although there can be occasions where it
makes sense).
-
Your comment on
-
You should be very careful about your integer arithmetic to avoid overflow
or loss of precision. Integer dividing x by 1000 for example will give a
result of 0 for x == 999. It can be better to multiply first, then divide
in order not to lose precision.
-
Your small types (8 and 16 bit) types may be inappropriate. Or they may be
ok. It depends upon your target processor.
-
Your
of the same name. Try to avoid globals if at all possible, but if you must
use them, don't do this. Also note that if globals are being shared between
tasks they need to be declared
-
Your task startup code is messy. I don't see why you need
And I don't see why
has its timer created for it by
questionable necessity for two timers/semaphore).
-
Splitting the job into separate files for each persistent task and a
Firstly the code is rather chaotic. It is difficult to make out what is going
on and what the requirement is. I'd expect to see some sort of comment at the
top that summarises the purpose of the overall system. I cannot compile it
because of all the headers, so I don't know how cleanly it builds. Do you
have plenty of compiler warnings turned on?
I don't follow exactly what is required of you. If your 2 tasks are just
alternating then one semaphore and one timer would seem to be all you need,
not two of each. The timer wakes up the first task via the semaphore; the
first task waits on the semaphore, sends a message to the 2nd then waits
on a reply from the 2nd; the 2nd task waits on a message queue from the 1st
task, does its stuff then sends a reply to the first.
Some specific comments:
-
Error handling is lacking. Often you don't check errors or you check for
success but continue on failure. Error handling in an embedded system is
difficult. As this is an exercise, I suggest that on error you should turn
on all LEDs or make them say ERR or something like that and then hang in an
infinite loop. A real system would want to be able to recover (eg restart
via a watchdog timer), but I think that is beyond the scope of your project.
-
Some of your variable and function names are not so good.
aSemaphore andaSemaphore2 give no clue as to their function and are hardly different.Ditto,
SWTimer amd SWTimer2. TimerCallback is unhelpful when there are2 timers and
release is equally opaque.-
There are many flags and some functions and variables that are unused.
When submitting code for review your should omit such parts. This also
applies to commented-out lines of code which give the reader a poor
impression of the completeness/readiness of your code.
-
Global variable
status should be deleted. Make it local and check itsvalue.
-
Your
b2sLUT should be const and should be integrated into andinterrogated by
show_velocity_on_sevenseg. int2seven seems redundant.-
You have numeric constants scattered throughout your code. This is
generally considered bad practice, (although there can be occasions where it
makes sense).
-
Your comment on
show_position refers to 4 LEDs but lists 6.-
You should be very careful about your integer arithmetic to avoid overflow
or loss of precision. Integer dividing x by 1000 for example will give a
result of 0 for x == 999. It can be better to multiply first, then divide
in order not to lose precision.
-
Your small types (8 and 16 bit) types may be inappropriate. Or they may be
ok. It depends upon your target processor.
-
Your
adjust_velocity obscures global brake_pedal with its local variableof the same name. Try to avoid globals if at all possible, but if you must
use them, don't do this. Also note that if globals are being shared between
tasks they need to be declared
volatile.-
Your task startup code is messy. I don't see why you need
main to startStartTask and then the latter to start the two persistent tasks and die.And I don't see why
VehicleTask creates its own timer yet ControlTaskhas its timer created for it by
StartTask (but see above on thequestionable necessity for two timers/semaphore).
-
Splitting the job into separate files for each persistent task and a
main might be preferable.Context
StackExchange Code Review Q#64399, answer score: 4
Revisions (0)
No revisions yet.