Polling for Nios 2

The program listens to the Altera FPGA DE2 board's keys that can start and stop and reset a counter:

#include 
#include "system.h"
#include "altera_avalon_pio_regs.h"
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 )

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

void pollkey() {

    int action = IORD_ALTERA_AVALON_PIO_DATA(DE2_PIO_KEYS4_BASE);
    putchar(action);
    if (action == 7) {  // 2^0+2^1+2^2
        timeloc = 0x0;
    } else if (action == 13) { // 2^0+2^2+2^3
        RUN = 0;
    } else if (action == 14) { // 2^1+2^2+2^3
        RUN = 1;
    } else if (action == 11) { // 2^0+2^1+2^3
        tick(&timeloc);
    }
}

int main() {
    while (TRUE) {
        pollkey();
        puttime(&timeloc);
        delay(1000);
        IOWR_ALTERA_AVALON_PIO_DATA(DE2_PIO_REDLED18_BASE, timeloc);
        if (RUN == 1) {
            tick(&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) > 8) > 12) << 21);
    IOWR_ALTERA_AVALON_PIO_DATA(DE2_PIO_HEX_LOW28_BASE, hexresult);
}

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

I also use Nios 2 assembly for the delay:

```
.equ delaycount, 16911 #set right delay value here!
.text # Instructions follow
.global delay #

You need to change the naming convention. Obviousy you can't do that for the API provided for the FPGA but for the function's name as well as variable names you need to change that a bit.

Like hex7seg should be changed hex7seg_lower and hex7seg2 should be changed to hex7seg_upper. Obviously you can use different conventions but making them descriptive would be helpful. Anyone familiar with what you are doing wouldn't need to read through the functions to find which one is for the upper hex digit and which is for the lower digit. Otherwise you can use comments which you are lacking.

The naming convention problem is apparent in trantab. It should have been tran_tab. Much more readable and easier to interperet as translation_table. hexresult, puthex are all confusing. You know what you are doing but if you want anyone else to be able to guess what the program is doing you need to change the names.

The transtab is a constant. So why not define it as a global constant. I know it is bad to define global variables but if you define it as

const int tran_tab[] = //The values

it will be easier to access it everywhere. Also you should be putting such constants in a header file for easy modification.

Don't use the register keyword unless you are very much sure. If you are using any modern compiler it would be better at optimizing the use of register variables than you the programmer. I had confusion about that also and used them. See this for clarification.

I am not sure whether you need the values of both hexadecimal digits separately or not. If you want both then it would be easier to use this. Assuming Global constant variable for the translation table I removed the variable.

void hex7seg(int digit, int *lower, int *upper) {
    *lower = tran_tab[digit & 0x0f];
    *upper = tran_tab[digit & 0xf0];
}

You can use switch statement in pollkey instead of if-else ladder . Might make the thing more readable.

Other than that I don't think I can tell more. I'll have a look again but that's nearly all I can say.

StackExchange Code Review Q#30870, answer score: 5