3
\$\begingroup\$

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

#include <stdio.h>
#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) << 7);
    hexresult = hexresult | (hex7seg(inval >> 8) << 14);
    hexresult = hexresult | (hex7seg(inval >> 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           # Makes "main" globally known

delay:  beq     r4,r0,fin       # exit outer loop
        movi    r8,delaycount   # delay estimation for 1ms

inner:  beq     r8,r0,outer     # exit from inner loop
        subi    r8,r8,1         # decrement inner counter
        br      inner

outer:  subi    r4,r4,1         # decrement outer counter
        #call    pollkey
        br      delay


fin:    ret

Update

I have updated the program which behaves as expected. The changes is that I needed one action every millisecond and one action every second, so I used a modulo 1000 count.

#include <stdio.h>
#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);
    if (action == 7) {
        timeloc = 0x0;
        puttime(&timeloc);
        puthex(timeloc);
        delay(200);
    } else if (action == 13) {
        RUN = 0;
    } else if (action == 14) {
        RUN = 1;
    } else if (action == 11) {
        tick(&timeloc);
        puttime(&timeloc);
        puthex(timeloc);
        delay(200);
    }
}

int main() {
    int counter = 0;
    while (TRUE) {
        pollkey();
        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);
}

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]);
}



        .equ    delaycount,     5800 #set 16911 right delay value here!
        .text                   # Instructions follow
        .global delay           # Makes "delay" globally known

delay:  beq     r4,r0,fin       # exit outer loop
        movi    r8,delaycount   # delay estimation for 1ms

inner:  beq     r8,r0,outer     # exit from inner loop
        subi    r8,r8,1         # decrement inner counter
        br      inner

outer:  subi    r4,r4,1         # decrement outer counter
        br      delay


fin:    ret
\$\endgroup\$
  • 3
    \$\begingroup\$ Too many magic numbers, it's hard to understand the purpose of the program. \$\endgroup\$ – idoby Sep 6 '13 at 13:02
  • 1
    \$\begingroup\$ @busy_wait He pretty much summarized its working in the first sentence. It's related to electronics - FPGA. He could have used typedef or comments for the magic numbers but they couldn't have been avoided as they are according to the data sheets. \$\endgroup\$ – Aseem Bansal Sep 7 '13 at 15:11
  • 1
    \$\begingroup\$ @AseemBansal They're magic in the sense that simple named constants could have made the code much more readable, like timeloc, KEYS4, etc. \$\endgroup\$ – idoby Sep 7 '13 at 15:44
  • 1
    \$\begingroup\$ @busy_wait Agreed. He could have used typedefs for them. \$\endgroup\$ – Aseem Bansal Sep 7 '13 at 15:46
  • 1
    \$\begingroup\$ Also in the third line of the assembly code, the comment is wrong. \$\endgroup\$ – idoby Sep 7 '13 at 15:53
5
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

Your external prototypes would be better located in a header shared by the file that defines them. If they are all defined in assembler (like delay) then they still need to be put in a header if they are used by more than one C file. On the prototypes:

  • It seems odd for puttime to take a non-constant pointer. Does it modify the referenced value or could it be passed by value?

  • Similarly, tick might be more normal if it takes a timeloc value and returns a new one.

  • Some of the int values should probably be unsigned (eg if they are bitmaps or such like).

If your global variables (timeloc or RUN) are ever updated from assembler, or asynchronously, be sure to mark them volatile. And RUN would normally be run (upper-case normally being used for constants). Both should be static if only accessed here.

And TRUE seems redundant. It gains nothing (while (1) is as understandable as while (TRUE) to me) and you don't even use it for the RUN state.


In pollkey (which should have a void parameter list) you are interpreting a value that is an active-low bitmap. Your way of testing values is not immediately obvious - if you had inverted/masked the value and tested against 1, 2, 4 and 8 it would have been. By testing only for these values you are assuming that they cannot be active together. Maybe that is true. I'd rather see some constants, but not those you have (better 1,2,4,8).


In your hex7seg the ints should probably be unsigned. I'd put the size in trantab to make it more likely the compiler will warn you if there are too many/few initialisers (depends on compiler warning level) and line-up the values for the same reason and for readability. And the temporary variable is unnecessary.

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

Since you don't use hex7seg2, delete it. If you need it (which seems unlikely) then an Aseem says you should not duplicate trantab


In your delay code, each loop has two branches instead of one. As a rule, I'd expect to see only one branch in each loop (inner and outer)

outer:  movi    r8, delaycount 
inner:  subi    r8, r8, 1
        bneq    r8, r0, inner
        subi    r4, r4, 1 
delay:  bneq    r4, r0, outer  
        ret

(assuming bneq exists in Nios2). Note: is it right to test against r0 in the inner loop and not against 0? Also, if you prefer, put the entry point at the beginning with an explicit test for 0 call parameter before the loops

\$\endgroup\$

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.