C for Nios-2 IRQ handling

Question

Related:

Nios 2 interrupt handler

/*
 * Main C program for nios2int 2012-10-31
 * Assignment 6: Interrupts from
 * timer_1 and de2_pio_keys4
 */

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

/* Define the null pointer */
#define NULL_POINTER ( (void *) 0)

/* Define address for de2_pio_redled18 */
volatile int * const de2_pio_redled18_base = (volatile int *) 0x810;
/* The above means:
 *
 * de2_pio_redled18_base is a pointer variable.
 * The pointer is allowed to point to a volatile int.
 *
 * The keyword "volatile" means that this int must be read (with a load
 * instruction) every time the C program reads it, and written
 * (with a store instruction) every time the C program writes to it.
 * So the keyword "volatile" forbids the compiler to re-use old values
 * of the int. Re-using old values, instead of reading them again from memory,
 * is a very common optimization that the compiler can perform.
 *
 * We initialize the pointer to 0x810.
 * 0x810 is an integer value, and not a pointer, so we must use a
 * type-cast to tell the compiler that we really know what we are doing.
 * The type-cast is the type-specification in parentheses.
 * The type in the cast must be the same as the type of de2_pio_redled18_base.
 *
 * The keyword "const" means that we are not allowed to change this variable.
 * The C compiler will give an error message if we write C code that tries
 * to change a const. This can help you catch some common typing-mistakes.
 *
 * The "const" keyword has been placed very carefully. We can not change
 * the pointer-variable itself, but we can change whatever value that
 * the pointer-variable points to. So we can write to address 0x810
 * without any error-messages from the compiler, but we cannot change
 * de2_pio_redled18_base to point to something else.
 *
 * Note: the "volatile" keyword has also been placed very carefully, so that
 * it is the int that becomes volatile and not something else.
 */

/* 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;
/* de2_pio_keys4_irqbit
 * is a bit-mask with a 1 in the bit with index 2 */

/* Define addresses etc for de2_pio_toggles18 */
volatile int * const de2_pio_toggles18_base    = (volatile int *) 0x850;
volatile int * const de2_pio_toggles18_intmask = (volatile int *) 0x858;
volatile int * const de2_pio_toggles18_edgecap = (volatile int *) 0x85c;
const int de2_pio_toggles18_intindex = 3;
const int de2_pio_toggles18_irqbit = 1 << 3;
/* de2_pio_toggles18_irqbit
 * is a bit-mask with a 1 in the bit with index 3 */

/* Define addresses etc for timer_1 */
volatile int * const timer_1_base        = (volatile int *) 0x920;
volatile int * const timer_1_status      = (volatile int *) 0x920; /* same as base */
volatile int * const timer_1_control     = (volatile int *) 0x924;
volatile int * const timer_1_period_low  = (volatile int *) 0x928;
volatile int * const timer_1_period_high = (volatile int *) 0x92c;
volatile int * const timer_1_snaplow     = (volatile int *) 0x930;
volatile int * const timer_1_snaphigh    = (volatile int *) 0x934;
const int timer_1_intindex = 10;
const int timer_1_irqbit = 1 << 10;
/* timer_1_irqbit
 * is a bit-mask with a 1 in the bit with index 10 */

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

/* Define address for de2_pio_greenled9 */
volatile int *  de2_pio_greenled9 = (volatile int *) 0xa10;

/* Define address for de2_uart_0 */
#define UART_0   ( (volatile int *) 0x860 )

/* Delay parameter for somedelay() */
#define DELAYPARAM (65535)

/* Delay parameter for bigdelay() */
#define BIGDELAYPARAM (33)

/*
 * Define timeout count for timer_1
 * Use 4999999 for the simulator (six 9's - 0.1 seconds),
 * but 49999999 for the hardware (seven 9's - 1.0 seconds ).
 */
#define TIMER_1_TIMEOUT (4999999)

/* Define global variables. They are declared volatile,
 * since they are modified by interrupt handlers. */
volatile int mytime = 0x5957;   /* Time display */
volatile int myleds = 0;        /* Green LEDs (local copy) */



/* Declare those functions that are defined in other files. */
int initfix_int( void );        /* in initfix_int.c */
void puttime( volatile int * ); /* in puttime_uart_0.c */
void tick ( volatile int * );   /* in your file tick.s */

//volatile int * irq_handler_toggles;

void out_char_uart_0( int c )
{
  /* Wait until transmitter is ready */
  while( (UART_0[2] & 0x40) == 0 );
  /* Now send character */
  UART_0[1] = c & 0xff;
}

/* This simple subroutine stalls
 * execution for a short while. */
void somedelay( void )
{
  int i = DELAYPARAM;
  while( (i = i - 1) > 0);
}

/* This simple subroutine stalls
 * execution for a long while. */
void bigdelay( void )
{
  int j = BIGDELAYPARAM;
  while( (j = j - 1) > 0) somedelay();
}

/*
 * 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;
  /* Clear edge capture register - writing
   * any value clears all bits. */
  *de2_pio_keys4_edgecap = 0;
  /* If action on KEY0 */
  if( edges & 1 )
  {
    /* If KEY0 is pressed now */
    if( (*de2_pio_keys4_base & 1) == 0 )
    {
      /* Turn on green LED LEDG0
       * in software copy of LED bits. */
      myleds = myleds | 1;
      /* Copy software LED bits to actual LEDs. */
      *de2_pio_greenled9 = myleds;

      /* Print an upper-case 'D' using out_char_uart_0. */
      out_char_uart_0( 'D' );

      /* Wait a long while */
      bigdelay();
      /* Print a lower-case 'd' using out_char_uart_0. */
      out_char_uart_0( 'd' );
    }
    /* If KEY0 is released now */
    else if( (*de2_pio_keys4_base & 1) != 0 )
    {
      /* Turn off green LED LEDG0
       * in software copy of LED bits. */
      myleds = myleds & 0xffffe;

      /* Print an 'U' using out_char_uart_0. */
      out_char_uart_0( 'U' );
      /* Copy software LED bits to actual LEDs. */
      *de2_pio_greenled9 = myleds;
    }
    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 from KEY0 only. */
  *de2_pio_keys4_intmask = 1;

  /* 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();
}

/*
 * Interrupt handler for timer_1.
 * The parameters are ignored here, but are
 * required for correct compilation.
 * The type alt_u32 is an Altera-defined
 * unsigned integer type.
 */
void irq_handler_timer_1( void * context, alt_u32 irqnum )
{
    alt_u32 save_value;
    save_value = alt_irq_interruptible( timer_1_intindex );
  *timer_1_status = 0; /* Acknowledge interrupt */
  tick( &mytime );
  puttime( &mytime );
  out_char_uart_0( '\n' );
  alt_irq_non_interruptible( save_value );
}

/*
 * Initialize timer_1 for regular interrupts,
 * once every timeout period.
 * The timeout period is defined above,
 * see definition of TIMER_1_TIMEOUT
 */
void timerinit_int( void )
{
  /* Declare a local temporary variable
   * for checking return values
   * from system-calls and library functions. */
  register int ret_val_check;

  *timer_1_period_low = TIMER_1_TIMEOUT & 0xffff;
  *timer_1_period_high = TIMER_1_TIMEOUT >> 16;
  *timer_1_control = 7;
  /* START bit (must always be a 1)
   * CONT bit (timer restarts on timeout)
   * ITO bit (interrupt on timeout) */

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

void irq_handler_toggles( void * context, alt_u32 irqnum )
{
    alt_u32 save_value;
    save_value = alt_irq_interruptible( de2_pio_toggles18_intindex );
    out_char_uart_0((int) 'S'); //castar char code för S till funktionen.
    *de2_pio_redled18_base = 0x0001;
    bigdelay();
    *de2_pio_redled18_base = 0x0000;
    out_char_uart_0((int) 's');
    *de2_pio_toggles18_edgecap = 0x0001; /* Acknowledge interrupt */
    alt_irq_non_interruptible( save_value );
}

//void (* irq_handler_toggles) (void);

void toggles_init()
{
    int check_value;
    *de2_pio_toggles18_intmask = 0x0001; //enable toggle0 only

    check_value = alt_irq_register( de2_pio_toggles18_intindex, NULL_POINTER, irq_handler_toggles );

    if( check_value != 0 ) n2_fatal_error(); /* Change this to code for handling the 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();

  /* Initialize de2_pio_keys4 for
   * interrupts. */
  keysinit_int();

  /* Initialize timer_1 for
   * continuous timeout interrupts. */
  timerinit_int();

  toggles_init();

  /* Loop forever. */
  while( 1 )
  {
    out_char_uart_0('_'); /* print an underscore */

    /* Programmed delay between underscores.
     * Defined earlier in this file. */
    somedelay();
  }
}

Answer 1

Nick, the code looks very clean and well organised. Nice on the eye.

General comments:

• make local functions static

• Comment noise. Many comments make the code worse by imparting no useful information. Your assignment doubtless requires that you comment your code, but you also need to learn what not to write. Check with your supervisor before wholesale deletion of comments - which could loose you points. Examples of noise (there are many, many more):

  • describing the meaning of 'volatile', 'const' etc should not be necessary.

  • de2_pio_keys4_irqbit is a bit-mask with a 1 in the bit with index 2 etc

  • Define address for ... etc

• also, many comments are wrapped at colum 50 or less. Let them extend to 80 (but no further)

• delete all commented-out code

• functions defined in other files should have prototypes in a header shared by both files (unless defined in assembler).

• using register is generally pointless. The compiler has a better idea of what needs to go in a register than you.

• embedded constants are bad practice. You should normally define them together (or with the registers they apply to) using #define

Detailed points:

• NULL_POINTER seems unnecessary. Just use 0

• all of your machine registers are defined as volatile int but for many, an unsigned type looks more applicable (those which do not hold a numeric value, such as bitmasks).

• why is UART_0 defined with a #define but all other registers use a const int pointer?

• consider an alternative method of register access. In your method, all access is through a pointer: *de2_pio_keys4_edgecap = 0; and if( (*de2_pio_keys4_base & 1) == 0 )

  In a small program like this it might be overkill, but it can be worth wrapping register access to avoid direct use of pointers:

  static inline void clear_edgecap(void) 
  {
     *de2_pio_keys4_edgecap = 0;   
  }
  
  static inline int key0_is_pressed(void)
  {
     return (*de2_pio_keys4_base & 1) == 0;
  }
  

  then the function calls are self-explanatory

  clear_edgecap();
  if (key0_is_pressed()) {...}
  

  Note that the functions are inline and so there is likely to be zero overhead.

• why not pass char to out_char_uart_0 instead of int ?

• busy-looping in out_char_uart_0 (and delay functions) would normally be a bad idea, but in the context of your project is perhaps necessary (I assume you have no OS). I'd prefer to see the loop made clear with braces:

   while ((UART_0[2] & UART_XMIT_READY) == 0) {
       /* Wait until transmitter is ready */
   }
  

  note the #defined constant instead of 0x40

• delay functions:

  int i = ...;
  while (--i > 0) {
      /* busy loop */
  }
  

• in n2_fatal_error, N2_FATAL_ERROR_HEX_PATTERN might be better defined with other #defines (if there are others relating to the 7-seg display).

• printing of error message uses a loop calling out_char_uart_0. This might be better as a simple function, out_string_uart_0

• cp = cp + 1 is normally written ++cp

In irq_handler_keys

• function is heavily over-commented. Nearly all the comments are just noise.

• assign a value immediately:

  alt_u32 save_value = alt_irq_interruptible(...);
  

• de2_pio_keys4_base is interrogated twice

• the saved value is restored only if the edges & 1 branch was taken - is this correct?

• for unused parameters, you can use a void cast to 'use' them and keep the compiler from complaining.

  (void) context;
  (void) irqnum;
  

In keysinit_int:

• I would use 'irq' rather than 'int' as an abbreviation for an interrupt (request)

• over-commented again. Declare a temporary for checking ... what use is that to anybody?

• The comment on the alt_irq_register call should be on the function declaration, not the call. The function name is poorly chosen as it gives the reader no idea of its purpose. Maybe enable_altera_irq

• ret_val_check is verbose: ret would suffice, but here you don't even need a variable:

   if (alt_irq_register(de2_pio_keys4_intindex, 0, irq_handler_keys) != 0) {
        n2_fatal_error();
   }
  

In irq_handler_timer_1

• the function header would be better used to say the purpose of the function (briefly!)

In toggles_init

• here you use the name check_value instead of ret_val_check. Better to be consistent. But note that the variable is not necessary (see above).

• need (void) parameter list

In main

• need (void) parameter list.