1
\$\begingroup\$

The program is behaving like expected, but I think something better can be done since I'm a novice C programmer. I suspect that the interrupt handler can use some other way of calling the hardware instead of direct memory. Can you look and tell me what can be improved?

#include <stdio.h>
#include "system.h"
#include "altera_avalon_pio_regs.h"

/* Include header file for alt_irq_register() */
#include "alt_irq.h"

extern int initfix_int(void);
extern void puttime(int* timeloc);
extern void puthex(int time);
extern void tick(int* timeloc);
extern void delay(int millisec);
extern int hexasc(int invalue);

#define TRUE 1
#define KEYS4 ( (unsigned int *) 0x840 )

#define NULL_POINTER ( (void *) 0)

int timeloc = 0x5957; /* startvalue given in hexadecimal/BCD-code */
int RUN = 1;

#define TIMER1 ( (unsigned int *) 0x920 )
#define PERIOD (49999)

/* Define addresses etc for de2_pio_keys4 */
volatile int * const de2_pio_keys4_base = (volatile int *) 0x840;
volatile int * const de2_pio_keys4_intmask = (volatile int *) 0x848;
volatile int * const de2_pio_keys4_edgecap = (volatile int *) 0x84c;
const int de2_pio_keys4_intindex = 2;
const int de2_pio_keys4_irqbit = 1 << 2;

/* Define address for de2_pio_hex_low28 */
volatile int * de2_pio_hex_low28 = (volatile int *) 0x9f0;

/*
 * The n2_fatal_error function is called for unexpected
 * conditions which most likely indicate a programming error
 * somewhere in this file. The function prints "FATAL ERROR"
 * using out_char_uart_0, lights an "Err" pattern on the
 * seven-segment display, and then enters an infinite loop.
 */
void n2_fatal_error() {
    /* Define the pattern to be sent to the seven-segment display. */
#define N2_FATAL_ERROR_HEX_PATTERN ( 0xcbd7ff )
    /* Define error message text to be printed. */
    static const char n2_fatal_error_text[] = "FATAL ERROR";
    /* Define pointer for pointing into the error message text. */
    register const char * cp = n2_fatal_error_text;

    /* Send pattern to seven-segment display. */
    *de2_pio_hex_low28 = N2_FATAL_ERROR_HEX_PATTERN;
    /* Print the error message. */
    while (*cp) {
        //out_char_uart_0( *cp );
        cp = cp + 1;
    }

    /* Stop and wait forever. */
    while (1)
        ;
}

/*
 * Interrupt handler for de2_pio_keys4.
 * The parameters are ignored here, but are
 * required for correct compilation.
 * The type alt_u32 is an Altera-defined
 * unsigned integer type.
 *
 * To help debugging interruptible interrupt-handlers,
 * this handler delays a long while when a key is pressed.
 * However, there is no delay when the key is released.
 *
 * We keep a software copy of the LED value, since
 * the parallel output ports are not program-readable.
 *
 * Example: we send out the value 1 on de2_pio_keys4,
 * by executing *DE2_PIO_KEYS4_BASE = 1;
 * Then we try to read the port by executing
 * int test_val = *DE2_PIO_KEYS4_BASE; // WRONG
 * The value of test_val is now undefined.
 * The port returns some bits which are not related
 * to the value we have written.
 *
 * The software copy of the LED value
 * for this interrupt handler
 * is the global variable myleds, defined above.
 */
void irq_handler_keys(void * context, alt_u32 irqnum) {
    alt_u32 save_value;
    save_value = alt_irq_interruptible(de2_pio_keys4_intindex);
    /* Read edge capture register of the de2_pio_keys4 device. */
    int edges = *de2_pio_keys4_edgecap;
    *de2_pio_keys4_edgecap = 0;

    /* If action on KEY0 */
    if (edges & 1) {
        /* If KEY0 is pressed now */
        if ((*de2_pio_keys4_base & 1) == 0) {

            if (RUN == 0) {
                RUN = 1;
            } else {
                RUN = 0;
            }

        }
        /* If KEY0 is released now */
        else if ((*de2_pio_keys4_base & 1) != 0) {

        }
        alt_irq_non_interruptible(save_value);
    } else if (edges & 2) {
        /* If KEY1 is pressed now */
        if ((*de2_pio_keys4_base & 2) == 0) {

            tick(&timeloc);
            puttime(&timeloc);
            puthex(timeloc);

        }
        /* If KEY1 is released now */
        else if ((*de2_pio_keys4_base & 2) != 0) {

        }
        alt_irq_non_interruptible(save_value);
    }

    else if (edges & 4) {
        /* If KEY2 is pressed now */
        if ((*de2_pio_keys4_base & 4) == 0) {

            timeloc = 0x0;
            puttime(&timeloc);
            puthex(timeloc);

        }
        /* If KEY2 is released now */
        else if ((*de2_pio_keys4_base & 4) != 0) {

        }
        alt_irq_non_interruptible(save_value);
    }

    else if (edges & 8) {
        /* If KEY3 is pressed now */
        if ((*de2_pio_keys4_base & 8) == 0) {

            timeloc = 0x5957;
            puttime(&timeloc);
            puthex(timeloc);

        }
        /* If KEY3 is released now */
        else if ((*de2_pio_keys4_base & 8) != 0) {

        }
        alt_irq_non_interruptible(save_value);
    }

}

/*
 * Initialize de2_pio_keys4 for interrupts.
 */
void keysinit_int(void) {
    /* Declare a temporary for checking return values
     * from system-calls and library functions. */
    register int ret_val_check;

    /* Allow interrupts */
    *de2_pio_keys4_intmask = 15;

    /* Set up Altera's interrupt wrapper for
     * interrupts from the de2_pio_keys4 device.
     * The function alt_irq_register will enable
     * interrupts from de2_pio_keys4.
     * Return value is zero for success,
     * nonzero for failure. */
    ret_val_check = alt_irq_register(de2_pio_keys4_intindex, NULL_POINTER,
            irq_handler_keys);
    /* If there was an error, terminate the program. */
    if (ret_val_check != 0)
        n2_fatal_error();
}

int main() {

    /* Remove unwanted interrupts.
     * initfix_int is supplied by KTH.
     * A nonzero return value indicates failure. */
    if (initfix_int() != 0)
        n2_fatal_error();

    keysinit_int();
    int counter = 0;
    while (1) {
        delay(1);
        ++counter;
        if (counter % 1000 == 0) {
            IOWR_ALTERA_AVALON_PIO_DATA(DE2_PIO_REDLED18_BASE, timeloc);
            if (RUN == 1) {
                tick(&timeloc);
                puttime(&timeloc);
                puthex(timeloc);
            }
        }
    }

    return 0;
}

int hex7seg(int digit) {
    int trantab[] = { 0x40, 0x79, 0x24, 0x30, 0x19, 0x12, 0x02, 0x78, 0x00,
            0x10, 0x08, 0x03, 0x46, 0x21, 0x06, 0x0e };
    register int tmp = digit & 0xf;
    return (trantab[tmp]);
}

void puthex(int inval) {
    unsigned int hexresult;
    hexresult = hex7seg(inval);
    hexresult = hexresult | (hex7seg(inval >> 4) << 7);
    hexresult = hexresult | (hex7seg(inval >> 8) << 14);
    hexresult = hexresult | (hex7seg(inval >> 12) << 21);
    IOWR_ALTERA_AVALON_PIO_DATA(DE2_PIO_HEX_LOW28_BASE, hexresult);
}
\$\endgroup\$
1
\$\begingroup\$

The interrupt handler looks a little verbose. There are too many comments, most of which are 'noise' (they contribute little, also true elsewhere) and there are many empty sections (key-up detection). Here is a simplified version if the ISR:

void irq_handler_keys(void *context, alt_u32 irqnum) 
{
    const alt_u32 save_value = alt_irq_interruptible(irqnum);
    const unsigned edges = *de2_pio_keys4_edgecap;
    const unsigned keys = ~*de2_pio_keys4_base; // inverted

    *de2_pio_keys4_edgecap = 0;

    if (edges & PIO_KEYS4_TICK & keys) {
        tick(&timeloc);
        puttime(&timeloc);
        puthex(timeloc);
    } else if (edges & PIO_KEYS4_RUN & keys) {
        RUN ^= 1;
    } else if (edges & PIO_KEYS4_ZERO & keys) {
        timeloc = ZERO_TIME;
        puttime(&timeloc);
        puthex(timeloc);
    } else if (edges & PIO_KEYS4_RESET & keys) {
        timeloc = INITIAL_TIME;
        puttime(&timeloc);
        puthex(timeloc);
    }
    alt_irq_non_interruptible(save_value);
}

The changes are clear, but I'll run through them.

  • Firstly the call to alt_irq_interruptible needs to be balanced by just one call to alt_irq_non_interruptible, not four. And passing irqnum seems more logical to me than passing your assumed interrupt number.
  • The registers seem more like unsigned values than int values (eg you can't add them) and the keys register need only be read at the start of the function.
  • I reordered the tests to check the most likely one first. While this makes no significant difference if the ticking occurs once a second (although why is it coming through a key press if it is a clock tick? - maybe I am reading too much into the function-call names), this is an ISR so efficiency is important to bear in mind.
  • I have removed the redundant conditions to make the body rather simpler (the compiler would do this too so it is not significant at run time). This also changes what the function does (ie. it descends the if-else sequence until it finds a falling edge rather than just any edge) which might not be what you wanted.

  • And I used constants instead of 1,2,4,8 etc.

Note that RUN and timeloc should be volatile.

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for the answer. It is part of the project to increment a tick when pressing a key as well as counting up once per second. So therefore the call to tick is included in the interrupt for one of the keys. \$\endgroup\$ – Niklas Rosencrantz Sep 11 '13 at 14:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.