HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Ad hoc implementation of cruise control

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
implementationcruisecontrolhoc

Problem

I've written a small C program that has ad hoc implementation of cruise control for my Altera DE2 FPGA. It works alright but there are too many global variables. Preferably there should be no global variables. How can I reduce the amount of global variables?

```
#include
#include "system.h"
#include "includes.h"
#include "altera_avalon_pio_regs.h"
#include "sys/alt_irq.h"
#include "sys/alt_alarm.h"
#include "altera_avalon_pio_regs.h"
#include "alt_types.h"
#include
#include
#include

#define N 8192

#define M 32

#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_3 0x0004 // Cruise Control Button
#define LED_GREEN_4 0x0010 // Brake Pedal
#define LED_GREEN_6 0x0040 // Gas Pedal

#define TASK_STACKSIZE 2048

OS_STK StartTask_Stack[TASK_STACKSIZE];
OS_STK ControlTask_Stack[TASK_STACKSIZE];
OS_STK VehicleTask_Stack[TASK_STACKSIZE];
OS_STK VehicleTask_Stack[TASK_STACKSIZE];
OS_STK WatchdogTask_Stack[TASK_STACKSIZE];
OS_STK DetectionTask_Stack[TASK_STACKSIZE];
// Task Priorities

#define STARTTASK_PRIO 5
#define VEHICLETASK_PRIO 10
#define CONTROLTASK_PRIO 12
#define DETECTIONTASK_PRIO 13
#define WATCHDOGTASK_PRIO 14

// Task Periods

#define CONTROL_PERIOD 300
#define VEHICLE_PERIOD 300

/*
* Definition of Kernel Objects
*/

// Mailboxes
OS_EVENT *Mbox_Throttle;
OS_EVENT *Mbox_Velocity;
OS_EVENT *Mbox_Writeok;

// Semaphores
OS_EVENT *aSemaphore;
OS_EVENT *aSemaphore2;

// SW-Timer
OS_TMR *SWTimer;
alt_u32 ticks;
alt_u32 time_1;
alt_u32 time_2;
alt_u32 timer_overhead;
/*
* Types
*/
enum active {on, off};

enum a

Solution

Wow, I don't know where to start. I think I'll just work on one function.

void SwitchIO(INT16S* current_velocity, int position)
{
    int w[700];
    int tmp = 600;
    int x[13];
    int a, b;
    int size = M;
    int i, j;
    timer_overhead = 0;

    if (switches_pressed()==1) {
        engine = on;
        top_gear=off;
        show_position(position);
        printf("The engine is turned on\n");
    } else  if (switches_pressed()==3) {
        engine = on;
        top_gear=on;
        show_position(position);
    }
    else if (switches_pressed()==0) {
        printf("switches_pressed()==0\n");
        top_gear = off;
        engine = off;
        show_position(position);
    }
    else if (switches_pressed()==2) {

    }
    else if ((switches_pressed()-19)/16 >= 0) {
        int number = (switches_pressed()-19)/16+1;
        if (number > 50) {
            number = 50;
        }
        printf("extra load %d\n", number);
        for (i = 0; i < 10; i++) {
            start_measurement();
            stop_measurement();
            timer_overhead = timer_overhead + time_2 - time_1;
        }
        initArray(w, 600);
        initArray(x, 13);
        start_measurement();

        j=tmp+number*4;
        for (i = 0; i < j; i++)
            w[i]++;
        stop_measurement();
        printf("%5.2f us", (float) microseconds(ticks - timer_overhead));
        printf("(%d ticks)\n", (int) (ticks - timer_overhead));

    }
    printf("switches_pressed: %d\n", switches_pressed());
    printf("switches_pressed: %d\n", (switches_pressed()-19)/16);

}


I'm not sure what the intent is here. Part of the problem is the completely obscure variable names and values. What is w and why does it need to be of size 700? And so forth.

Do you really mean to poll the status of switches_pressed() multiple times, and then only take one action? That doesn't seem logical to me. If I were writing something similar, I'd save the value of switches_pressed() into a temporary variable, and then use chained ifs or a switch in order to take the appropriate action.

CONTINUING...

#include "system.h"
#include "includes.h"
#include "altera_avalon_pio_regs.h"
#include "sys/alt_irq.h"
#include "sys/alt_alarm.h"
#include "altera_avalon_pio_regs.h"
#include "alt_types.h"


Can you post these files on pastebin, github, or some other textfile hosting site and include a link?

Why in the world do you #include "altera_avalon_pio_regs.h" twice?

Code Snippets

void SwitchIO(INT16S* current_velocity, int position)
{
    int w[700];
    int tmp = 600;
    int x[13];
    int a, b;
    int size = M;
    int i, j;
    timer_overhead = 0;

    if (switches_pressed()==1) {
        engine = on;
        top_gear=off;
        show_position(position);
        printf("The engine is turned on\n");
    } else  if (switches_pressed()==3) {
        engine = on;
        top_gear=on;
        show_position(position);
    }
    else if (switches_pressed()==0) {
        printf("switches_pressed()==0\n");
        top_gear = off;
        engine = off;
        show_position(position);
    }
    else if (switches_pressed()==2) {

    }
    else if ((switches_pressed()-19)/16 >= 0) {
        int number = (switches_pressed()-19)/16+1;
        if (number > 50) {
            number = 50;
        }
        printf("extra load %d\n", number);
        for (i = 0; i < 10; i++) {
            start_measurement();
            stop_measurement();
            timer_overhead = timer_overhead + time_2 - time_1;
        }
        initArray(w, 600);
        initArray(x, 13);
        start_measurement();

        j=tmp+number*4;
        for (i = 0; i < j; i++)
            w[i]++;
        stop_measurement();
        printf("%5.2f us", (float) microseconds(ticks - timer_overhead));
        printf("(%d ticks)\n", (int) (ticks - timer_overhead));

    }
    printf("switches_pressed: %d\n", switches_pressed());
    printf("switches_pressed: %d\n", (switches_pressed()-19)/16);

}
#include "system.h"
#include "includes.h"
#include "altera_avalon_pio_regs.h"
#include "sys/alt_irq.h"
#include "sys/alt_alarm.h"
#include "altera_avalon_pio_regs.h"
#include "alt_types.h"

Context

StackExchange Code Review Q#86468, answer score: 3

Revisions (0)

No revisions yet.