help speeding up code

I've wrote a speedo program using a hall effect that then drives 3 x 7 segment display via 3 x CD4511 decoders.

The code works well except the write to the displays is slow and they flash about every 1/2 second.

It is the speedo subroutine (that senses the hall) that is slowing it down because if I run the 7 segment code from an analogue voltage the display 'appear' to stay on and change quickly when the anslog volts changes.

I need to make this as fast as possible as it is part of a much larger program.

Any suggestions gratefully received.

// the start of the BCD pins used for ABCD must be sequential
int BCDStart = 43;

// the start of the common pins - must be sequential
int commonStart = 47;

// this code can use either polarity of display.
// true if common is high when ON, otherwise false. Using common cathode = false
boolean commonHigh = false;

// the analog input for the potentiometer or use value from hall OP code
int hall_pin = A4;
float hall_thresh = 10.0;
int wheelCirc = 1000;
int KMHour = 0;

// the number of digits your display has.
int noOfDigits = 3;

void setup() {
  // Enable all bcd outputs, 
  // set them to OFF 
  for(int i=0;i<4;++i) 
  {
    pinMode(BCDStart + i, OUTPUT);
    pinMode(hall_pin, INPUT);
    digitalWrite(BCDStart + i, commonHigh?1:0);// If commonHigh is true (its false) set all pins to 1 else set to 0. Common cathode needs 0 (1 fires segment).
  }
  
  for(int i=0;i<noOfDigits;++i) 
  {
    pinMode(commonStart + i, OUTPUT);
    digitalWrite(commonStart + i, commonHigh?0:1); 
  }
   Serial.begin(9600);
}

void loop() {
speedo();
  int amount = KMHour; 
  if (amount <99)
  {
    noOfDigits = 2;
  }
    else
    {
    noOfDigits = 3;//turns of funwanted digits
  }
   
  for(int i=(noOfDigits -1);i>=0;--i) {
    // find the current digit, which is in the low 4 bits. first pass is right display on, second pass swithches to left display.
    int digit = amount % 10;
    // and write out the representation of the voltage
    writeDigitToDisplay(digit);// first send remainder to function then 2nd pass send 10's to function
    // enable the correct digit for display
    digitalWrite(commonStart + i, commonHigh?1:0);//writes each returned bit of remainder to right display. 
    // short delay between changes.
    delay(1);
    // Turn off the correct digit for display
    digitalWrite(commonStart + i, commonHigh?0:1);
    // get the next digit.
    amount = amount / 10;// finds 10's value and rerun For to initiate middle display
   // Turn off the correct digit for display
    digitalWrite(commonStart + i, commonHigh?0:1);
   }
}

// Now we define the bits that are on and off
// for ABCD output, These are used in the
// function below to generate the BCD for the 4511.
int dig[10] = {

          0b0000,//0
          0b0001,//1
          0b0010,//2
          0b0011,//3
          0b0100,//4
          0b0101,//5
          0b0110,//6
          0b0111,//7
          0b1000,//8
          0b1001,//9
 };

void writeDigitToDisplay(int digit) {
  
  for(int i=0;i<4;++i) 
  {
    // isolate the current bit in the loop.
    int currentBit = (1<<(i)); //shifts binary 0001 by 3-i. CurrentBit is 4 new combinations (as i changes)or reverse LSB read as (1<<(i))
    // check if the bit is on, and invert for
    // commonhigh if needed.
    int bitOn = (currentBit&digit)!=0;//if currentBit & digit then 3-1 is 1
    if(commonHigh) {//is false
      bitOn = !bitOn;//so bitOn stays at 1
  }
   // and lastly set the bit
    digitalWrite(BCDStart+i, bitOn); //writes bitOn to each BCD pin
  
  }
}

void speedo()
{
 float hall_count = 1.0;
  float start = micros();
  bool on_state = false;
  // counting number of times the hall sensor is tripped
  // but without double counting during the same trip
  while(true)
  {
    if (digitalRead(hall_pin)==0){//every time hall is switched to 0 run if
      if (on_state==false){//already set to false so make true
        on_state = true;
        hall_count+=1.0;//count = (hall_count + 1.0) + new value of hall_count
      }
    } else{
      on_state = false;
    }
   if (hall_count>=hall_thresh)
   {
      break;
   }
 }
  float end_time = micros();
  float time_passed = ((end_time - start)/1000000.0); 
  float rpm_val = (hall_count/time_passed)*60.0;
  KMHour = (((wheelCirc * rpm_val)/1000000)*60);
  Serial.println(KMHour);
}

well just a few notes that won't fix your issue but stand out.

// Now we define the bits that are on and off
// for ABCD output, These are used in the
// function below to generate the BCD for the 4511.
int dig[10] = {

          0b0000,//0
          0b0001,//1
          0b0010,//2
          0b0011,//3
          0b0100,//4
          0b0101,//5
          0b0110,//6
          0b0111,//7
          0b1000,//8
          0b1001,//9
 };

this is eh, how to put this politely.. totally unnecessary (you know i google words like that :wink: ) here you actually define dig[0] = 1, dig[1] = 1, dig[2] = 2 etc. the array is redundant if you use a BCD chip. and as far as i can tell you never even use the array.

float start = micros(); this should be an unsigned long

Anyway you'll have to change the speedo() function into a non blocking one and best is to attach an interrupt to the pin of the hall sensor i have not much experience with this but i think you'll need to change the pin to 2 or 3 (you anyway don't need it to be an analog pin since you do a digitalRead() on it) Check out this part of the documentation about it.

If you seriously need to speed up the program, unroll all the "for" instructions. In other words, duplicate the lines of code in the "for" for the number in the count. This will eliminate all the code to run the "for" loop.

Then look at the assembly code generated as part of the program compile. Understand each set of assembly codes generated for each line of "C" code and try to change the "C" to generate the fewest assembly instructions.

Not an easy and automatic to make faster code, but can be done.

Paul

How many Hall ticks per wheel revolution? Your wheel circumference is given as "1000": Is that mm?

It looks like you are taking the time required for a complete (single) revolution by counting the signals from the hall sensor.. If that is the case.

Then rather count the time taken per individual pulse to determine the speed, should allow for the speedo() function to finish faster and then loop() can get back to updating the display.

If i'm right, then alternatively you could just drive faster and get the speedo() function to count up to hall_thresh sooner hehe :wink:

Paul_KD7HB:
Then look at the assembly code generated as part of the program compile. Understand each set of assembly codes generated for each line of "C" code and try to change the "C" to generate the fewest assembly instructions.

Paul, how does one do that?

Consider/try the attached code.

It's based on your code but has some significant differences:

  1. Instead of blocking waiting on at least 10 pulses to be seen on the Hall sensor, this code uses a window of known time (500mS in this example) to count pulses. It's non-blocking, allowing the microcontroller to do other things such as updating the display.

Each measurement window is 500mS long; at the end of each window the speed is updated and a new measurement window started.

500mS may seem like a long time between updates but this is twice per second. For a human-readable speedo there's no need to update the speed any faster than that (IMHO.)

  1. The display (assumed to be 3 digits) is updated at roughly 30Hz, with a period of 10mS per digit. Leading zeros in the 100s and 10s position are blanked.

  2. I don't know how many pulses per revolution your Hall sensor produces; I assumed 60 in this example. There's a define allowing you to set it to the actual value. I assumed the wheel circumference value was given in mm. The speed calculation is aided by a define called SPEED_K (constant) that takes the pertinent constant and computes a float that, when multiplied by the number of ticks seen per 500mS, should give KPH. You should double check this :slight_smile:

  3. Due to the way the 7-seg digit timing is done, you may need to add series resistors to the common lines to limit current.

Anyway, look it over and see if you can use anything from it. I modified your display routine some so it'll probably require some debugging to work again.

(NOTE: Compiled for a Mega2560 successfully but not tested. May offend professional coders. YMMV...)

// the start of the BCD pins used for ABCD must be sequential
int BCDStart = 43;

// the start of the common pins - must be sequential
int commonStart = 47;

// this code can use either polarity of display.
// true if common is high when ON, otherwise false. Using common cathode = false
boolean commonHigh = false;

// the analog input for the potentiometer or use value from hall OP code
int hall_pin = 2;           //on 2560 can be used with falling-edge interrupt
int wheelCirc = 1000;
int KMHour = 0;
int hall_count = 0;

#define SPEED_MEAS_WINDOW   500     //mS update speed at 2Hz (500mS/update)
#define WHEEL_CIRC          1000    //mm
#define NUM_TICKS_PER_REV   60      //# guess

//speed constant
//based on wheel circumference, number of ticks per rev, mm to km conversion, measurement interval etc
#define SPEED_K             (float)((WHEEL_CIRC * 3600.0)/(1.0E+06 * (float)NUM_TICKS_PER_REV * ((float)SPEED_MEAS_WINDOW/1000.0) ))
//speed measurement states
#define SPEED_STATE_WAIT    0
#define SPEED_STATE_INIT    1
#define SPEED_STATE_MEASURE 2
#define SPEED_STATE_COMPUTE 3

#define BLANK               0xFF    //invalid BCD digit
#define DISP_UPDATE_TIME    10      //update rate of digits (30mS "frame rate")
//
#define DISP_STATE_SETUP        0
#define DISP_STATE_DIGIT_HUNDS  1
#define DISP_STATE_DIGIT_TENS   2
#define DISP_STATE_DIGIT_ONES   3


void setup() 
{
    int
        i;
        
    //set up Hall interrupt
    pinMode(hall_pin, INPUT_PULLUP);
    attachInterrupt(digitalPinToInterrupt(hall_pin), HallInterrupt, FALLING);
    
    // Enable all bcd outputs, 
    // set them to OFF 
    for( i=0; i<4; i++ ) 
    {
        pinMode(BCDStart + i, OUTPUT);
        digitalWrite(BCDStart + i, commonHigh?1:0);// If commonHigh is true (its false) set all pins to 1 else set to 0. Common cathode needs 0 (1 fires segment).
    }//for

    //num digits == 3
    for( i=0; i<3; i++ ) 
    {
        pinMode(commonStart + i, OUTPUT);
        digitalWrite(commonStart + i, commonHigh?0:1); 
    }//for

    KMHour = 0;
    
    Serial.begin(9600);
}

//////////////////////////////////////////////////////////////////////////////////
// HallInterrupt
//  on each falling edge of hall input, hall_count is incremented
//////////////////////////////////////////////////////////////////////////////////
void HallInterrupt( void )
{
    hall_count++;
    
}//HallInterrupt

void MeasureSpeed( void )
{
    int
        calc_hall_count;
    static byte
        speed_state = SPEED_STATE_INIT;
    static unsigned long
        timeWindow;
    unsigned long
        timeNow;
        
    switch( speed_state )
    {        
        case    SPEED_STATE_INIT:
            noInterrupts();
            timeNow = millis();
            hall_count = 0;
            //integration window is SPEED_MEAS_WINDOW mS
            timeWindow = timeNow + SPEED_MEAS_WINDOW;
            interrupts();
            speed_state = SPEED_STATE_MEASURE;
            
        break;

        case    SPEED_STATE_MEASURE:
            timeNow = millis();
            if( timeNow >= timeWindow )
            {
                noInterrupts();
                //get a copy of the hall_count and move to compute speed
                calc_hall_count = hall_count;
                interrupts();
                KMHour = (int)((float)calc_hall_count * SPEED_K);
                //limit displayed value to 200kph
                if( KMHour > 200 )
                    KMHour = 200;
                Serial.println(KMHour);   
                
                speed_state = SPEED_STATE_INIT;
            
            }//if
            
        break;

        default:
            speed_state = SPEED_STATE_INIT;
        break;
        
    }//switch
    
}//MeasureSpeed

void UpdateDisplay(  void )
{
    //update the display at 30Hz (~330mS) overall; requires us to have a 10mS step through this
    //state machine
    static int
        last_digit_num = 0;
    static int
        ones, tens, hunds;
    static byte
        displayState = DISP_STATE_SETUP;
    static unsigned long
        displayUpdate = millis() + DISP_UPDATE_TIME;

    if( millis() < displayUpdate )
        return;

    //turn off last digit
    digitalWrite(commonStart + 0, commonHigh?0:1);
    
    switch( displayState )
    {
        case    DISP_STATE_SETUP:
            //get digits for latest speed
            hunds = (int)(KMHour/100);
            KMHour = KMHour - 100*hunds;
            tens = (int)(KMHour/10);
            KMHour = KMHour - 10*tens;
            ones = KMHour;
            
            digitalWrite(commonStart + last_digit_num, commonHigh?0:1);

            displayState = DISP_STATE_DIGIT_HUNDS;
                        
        break;
        
        case    DISP_STATE_DIGIT_HUNDS:
            //if hundreds value is 0 skip it (leading zero)
            if( hunds == 0 )
                writeDigitToDisplay( BLANK );// first send remainder to function then 2nd pass send 10's to function
            else
                writeDigitToDisplay( hunds );
            
            digitalWrite(commonStart + 2, commonHigh?1:0);
            last_digit_num = 2;
            
            displayUpdate = millis() + DISP_UPDATE_TIME;
            displayState = DISP_STATE_DIGIT_TENS;
        
        break;

        case    DISP_STATE_DIGIT_TENS:
            //if hundreds value is 0 and tens is zero, skip it (another leading zero)
            //if hundreds value is 0 skip it (leading zero)
            if( hunds == 0 && tens == 0 )
                writeDigitToDisplay( BLANK );
            else
                writeDigitToDisplay( tens );
            
            digitalWrite(commonStart + 1, commonHigh?1:0);
            last_digit_num = 1;
            
            displayUpdate = millis() + DISP_UPDATE_TIME;
            displayState = DISP_STATE_DIGIT_ONES;
        
        break;

        case    DISP_STATE_DIGIT_ONES:
            //always display ones
            writeDigitToDisplay( ones );
            
            digitalWrite(commonStart + 0, commonHigh?1:0);
            last_digit_num = 0;
            
            displayUpdate = millis() + DISP_UPDATE_TIME;
            displayState = DISP_STATE_SETUP;
        
        break;
        
    }//switch
    
}//UpdateDisplay

void loop() 
{
    MeasureSpeed();
    UpdateDisplay();
       
}//loop

void writeDigitToDisplay(int value) 
{
    int
        i,
        bitOn;
        
    if( value == BLANK )
    {
        //for blanked digits (e.g. leading zeros) just set bits "inactive"
        //inactive level depends on commonHigh
        for( i=0; i<4; i++ )
            digitalWrite(BCDStart+i, (commonHigh)?1:0);
    }
    else
    { 
        for( i=0; i<4; i++ ) 
        {
            bitOn = (value & (1<<i))?1:0;
            digitalWrite(BCDStart+i, (commonHigh)?bitOn:!bitOn); //writes bitOn to each BCD pin
  
        }//for
        
    }//else
    
}//writeDigitToDisplay

Brilliant... Thanks.

It is one pulse per revolution so I will change that. I've got the resistors in the 7 seg and that works OK.

Will let you know how it goes....

Well it loaded OK on the Mega and I changed the rev counter to one per cycle.

The KM/Hr serial.print reports the right figure.

The 7 seg displays aren't reading the km/hr correctly though (digits missing/jumping).....shame I cant add a video to this.

I know the wiring is good I tested it with a simple analog input.

Will keep looking...

Thanks for the help so far.....

Kev

Sorry to hear. I may have made an error translating your code wrt the commons and order of display etc. I'll see if I can find any glaring errors here.

In the UpdateDisplay() routine my intent was to shut off the last common before applying the next digit. I think I may have erred there. So, two things:

  1. Update the last line here:
void UpdateDisplay(  void )
{
    //update the display at 30Hz (~330mS) overall; requires us to have a 10mS step through this
    //state machine
    static int
        last_digit_num = 0;
    static int
        ones, tens, hunds;
    static byte
        displayState = DISP_STATE_SETUP;
    static unsigned long
        displayUpdate = millis() + DISP_UPDATE_TIME;

    if( millis() < displayUpdate )
        return;

    //turn off last digit
    digitalWrite(commonStart + last_digit_num, commonHigh?0:1);

(was digitalWrite(commonStart + 0, commonHigh?0:1);)

and

  1. in state DISP_STATE_SETUP, remove the similar line:
        case    DISP_STATE_SETUP:
            //get digits for latest speed
            hunds = (int)(KMHour/100);
            KMHour = KMHour - 100*hunds;
            tens = (int)(KMHour/10);
            KMHour = KMHour - 10*tens;
            ones = KMHour;
            
            digitalWrite(commonStart + last_digit_num, commonHigh?0:1);  <<<<< REMOVE

            displayState = DISP_STATE_DIGIT_HUNDS;

See if that helps.

Here is what I have now with your corrections (I hope). I've listed observations at the end of this.

[code
// the start of the BCD pins used for ABCD must be sequential
int BCDStart = 43;

// the start of the common pins - must be sequential
int commonStart = 47;

// this code can use either polarity of display.
// true if common is high when ON, otherwise false. Using common cathode = false
boolean commonHigh = false;

// the analog input for the potentiometer or use value from hall OP code
int hall_pin = 2;           //on 2560 can be used with falling-edge interrupt
int wheelCirc = 1000;
int KMHour = 0;
int hall_count = 0;

#define SPEED_MEAS_WINDOW   500     //mS update speed at 2Hz (500mS/update)
#define WHEEL_CIRC          1000    //mm
#define NUM_TICKS_PER_REV   1      //# guess

//speed constant
//based on wheel circumference, number of ticks per rev, mm to km conversion, measurement interval etc
#define SPEED_K             (float)((WHEEL_CIRC * 3600.0)/(1.0E+06 * (float)NUM_TICKS_PER_REV * ((float)SPEED_MEAS_WINDOW/1000.0) ))
//speed measurement states
#define SPEED_STATE_WAIT    0
#define SPEED_STATE_INIT    1
#define SPEED_STATE_MEASURE 2
#define SPEED_STATE_COMPUTE 3

#define BLANK               0xFF    //invalid BCD digit
#define DISP_UPDATE_TIME    10      //update rate of digits (30mS "frame rate")
//
#define DISP_STATE_SETUP        0
#define DISP_STATE_DIGIT_HUNDS  1
#define DISP_STATE_DIGIT_TENS   2
#define DISP_STATE_DIGIT_ONES   3


void setup() 
{
    int
        i;
        
    //set up Hall interrupt
    pinMode(hall_pin, INPUT_PULLUP);
    attachInterrupt(digitalPinToInterrupt(hall_pin), HallInterrupt, FALLING);
    
    // Enable all bcd outputs, 
    // set them to OFF 
    for( i=0; i<4; i++ ) 
    {
        pinMode(BCDStart + i, OUTPUT);
        digitalWrite(BCDStart + i, commonHigh?1:0);// If commonHigh is true (its false) set all pins to 1 else set to 0. Common cathode needs 0 (1 fires segment).
    }//for

    //num digits == 3
    for( i=0; i<3; i++ ) 
    {
        pinMode(commonStart + i, OUTPUT);
        digitalWrite(commonStart + i, commonHigh?0:1); 
    }//for

    KMHour = 0;
    
    Serial.begin(9600);
}

//////////////////////////////////////////////////////////////////////////////////
// HallInterrupt
//  on each falling edge of hall input, hall_count is incremented
//////////////////////////////////////////////////////////////////////////////////
void HallInterrupt( void )
{
    hall_count++;
    
}//HallInterrupt

void MeasureSpeed( void )
{
    int
        calc_hall_count;
    static byte
        speed_state = SPEED_STATE_INIT;
    static unsigned long
        timeWindow;
    unsigned long
        timeNow;
        
    switch( speed_state )
    {        
        case    SPEED_STATE_INIT:
            noInterrupts();
            timeNow = millis();
            hall_count = 0;
            //integration window is SPEED_MEAS_WINDOW mS
            timeWindow = timeNow + SPEED_MEAS_WINDOW;
            interrupts();
            speed_state = SPEED_STATE_MEASURE;
            
        break;

        case    SPEED_STATE_MEASURE:
            timeNow = millis();
            if( timeNow >= timeWindow )
            {
                noInterrupts();
                //get a copy of the hall_count and move to compute speed
                calc_hall_count = hall_count;
                interrupts();
                KMHour = (int)((float)calc_hall_count * SPEED_K);
                //limit displayed value to 200kph
                if( KMHour > 200 )
                    KMHour = 200;
                Serial.println(KMHour);   
                
                speed_state = SPEED_STATE_INIT;
            
            }//if
            
        break;

        default:
            speed_state = SPEED_STATE_INIT;
        break;
        
    }//switch
    
}//MeasureSpeed

void UpdateDisplay(  void )
{
    //update the display at 30Hz (~330mS) overall; requires us to have a 10mS step through this
    //state machine
    static int
        last_digit_num = 0;
    static int
        ones, tens, hunds;
    static byte
        displayState = DISP_STATE_SETUP;
    static unsigned long
        displayUpdate = millis() + DISP_UPDATE_TIME;

    if( millis() < displayUpdate )
        return;

    //turn off last digit
     digitalWrite(commonStart + last_digit_num, commonHigh?0:1);
    
    switch( displayState )
    {
        case    DISP_STATE_SETUP:
            //get digits for latest speed
            hunds = (int)(KMHour/100);
            KMHour = KMHour - 100*hunds;
            tens = (int)(KMHour/10);
            KMHour = KMHour - 10*tens;
            ones = KMHour;
            
           
            displayState = DISP_STATE_DIGIT_HUNDS;
                        
        break;
        
        case    DISP_STATE_DIGIT_HUNDS:
            //if hundreds value is 0 skip it (leading zero)
            if( hunds == 0 )
                writeDigitToDisplay( BLANK );// first send remainder to function then 2nd pass send 10's to function
            else
                writeDigitToDisplay( hunds );
            
            digitalWrite(commonStart + 2, commonHigh?1:0);
            last_digit_num = 2;
            
            displayUpdate = millis() + DISP_UPDATE_TIME;
            displayState = DISP_STATE_DIGIT_TENS;
        
        break;

        case    DISP_STATE_DIGIT_TENS:
            //if hundreds value is 0 and tens is zero, skip it (another leading zero)
            //if hundreds value is 0 skip it (leading zero)
            if( hunds == 0 && tens == 0 )
                writeDigitToDisplay( BLANK );
            else
                writeDigitToDisplay( tens );
            
            digitalWrite(commonStart + 1, commonHigh?1:0);
            last_digit_num = 1;
            
            displayUpdate = millis() + DISP_UPDATE_TIME;
            displayState = DISP_STATE_DIGIT_ONES;
        
        break;

        case    DISP_STATE_DIGIT_ONES:
            //always display ones
            writeDigitToDisplay( ones );
            
            digitalWrite(commonStart + 0, commonHigh?1:0);
            last_digit_num = 0;
            
            displayUpdate = millis() + DISP_UPDATE_TIME;
            displayState = DISP_STATE_SETUP;
        
        break;
        
    }//switch
    
}//UpdateDisplay

void loop() 
{
    MeasureSpeed();
    UpdateDisplay();
       
}//loop

void writeDigitToDisplay(int value) 
{
    int
        i,
        bitOn;
        
    if( value == BLANK )
    {
        //for blanked digits (e.g. leading zeros) just set bits "inactive"
        //inactive level depends on commonHigh
        for( i=0; i<4; i++ )
            digitalWrite(BCDStart+i, (commonHigh)?1:0);
    }
    else
    { 
        for( i=0; i<4; i++ ) 
        {
            bitOn = (value & (1<<i))?1:0;
            digitalWrite(BCDStart+i, (commonHigh)?bitOn:!bitOn); //writes bitOn to each BCD pin
  
        }//for
        
    }//else
    
}//writeDigitToDisplay
]

on the 7 segment the LSB seems stick at a flashing zero and the MSB just jumps around

I am using a fan with a magnet attached to test this program and again I verified the display wiring using another program with just an analogue voltage so that should be Ok.

I'm sorry it's not working as expected. I'll try to replicate your setup here and see if I can figure out what's gone wrong.

Are you sourcing and sinking directly to the Arduino pins or do you have transistors switching the commons?

What display are you using?

Can you show your wiring?

Metallor:
Paul, how does one do that?

See reply #2 and #4 in How can you find the ELF file for your Arduion Project? - Programming Questions - Arduino Forum

I am switching the commons directly (no transistors) and using my basic test program which works with the analog input, it all switches finer and the correct vales are displayed.

I am confident the wiring is good as well because of the test program.

I really appreciate the help here. I am learning from your code but I'm fairly new to this so it is a steep curve but this is really helping.

I don't have an easy way to show the wiring but again I know it is good from test program.

Basically the 4 Arduino binary pins go to the 4511's A,B,C,D inputs (all lines shared). The 3 commons lines on the 7 segments come from the Arduino output pins.

The enable pins (LE) on the 4511's are all held low.

And of course the segments come from the 4511's via resistors

Okay, I wired up my 2560 with a seven-seg. However, I had to make some changes (see code below):

  • my display is common anode
  • I don't have a BCD IC so I used port pins 30-36 to drive segments a-g and used a decode table
  • I use a counter on KMHour for testing

It's otherwise very much like the code I posted and seems to work here. Have a look at this (it's what's running on my setup. I have a vid I'm uploading to a host now...maybe I can link it later.)

BTW, I changed the digit period from 10mS to 5mS to get rid of some perceivable flicker.

// the start of the BCD pins used for ABCD must be sequential
int BCDStart = 43;

//30 a
//31 b
//32 c
//33 d
//34 e
//35 f
//36 g
#define SEVSEGSTART     30  //pin 30 of 2560

// the start of the common pins - must be sequential
// 47 - ones
// 48 - tens
// 49 - hunds
// 50 - thous (not used)
int commonStart = 47;

// this code can use either polarity of display.
// true if common is high when ON, otherwise false. Using common cathode = false
boolean commonHigh = false;

// the analog input for the potentiometer or use value from hall OP code
int hall_pin = 2;           //on 2560 can be used with falling-edge interrupt
int wheelCirc = 1000;
int KMHour = 0;
int hall_count = 0;

#define SPEED_MEAS_WINDOW   500     //mS update speed at 2Hz (500mS/update)
#define WHEEL_CIRC          1000    //mm
#define NUM_TICKS_PER_REV   60      //# guess

//speed constant
//based on wheel circumference, number of ticks per rev, mm to km conversion, measurement interval etc
#define SPEED_K             (float)((WHEEL_CIRC * 3600.0)/(1.0E+06 * (float)NUM_TICKS_PER_REV * ((float)SPEED_MEAS_WINDOW/1000.0) ))
//speed measurement states
#define SPEED_STATE_WAIT    0
#define SPEED_STATE_INIT    1
#define SPEED_STATE_MEASURE 2
#define SPEED_STATE_COMPUTE 3

#define BLANK               10      //invalid BCD digit
#define DISP_UPDATE_TIME    5      //update rate of digits (30mS "frame rate")
//
#define DISP_STATE_SETUP        0
#define DISP_STATE_DIGIT_HUNDS  1
#define DISP_STATE_DIGIT_TENS   2
#define DISP_STATE_DIGIT_ONES   3

const byte DigitDecode[] =
    {
        //xgfedcba
        0b00111111,     //0
        0b00000110,     //1
        0b01011011,     //2
        0b01001111,     //3
        0b01100110,     //4
        0b01101101,     //5
        0b01111101,     //6
        0b00000111,     //7
        0b01111111,     //8
        0b01101111,     //9
        0b00000000      //BLANK
    };

void setup() 
{
    int
        i;
        
    //set up Hall interrupt
    pinMode(hall_pin, INPUT_PULLUP);
    attachInterrupt(digitalPinToInterrupt(hall_pin), HallInterrupt, FALLING);
    
    // Enable all bcd outputs, 
    // set them to OFF 
    for( i=0; i<7; i++ )
    {
        pinMode(SEVSEGSTART + i, OUTPUT);
        digitalWrite(SEVSEGSTART + i, HIGH);// If commonHigh is true (its false) set all pins to 1 else set to 0. Common cathode needs 0 (1 fires segment).
    }//for
    
    for( i=0; i<4; i++ ) 
    {
        pinMode(BCDStart + i, OUTPUT);
        digitalWrite(BCDStart + i, commonHigh?1:0);// If commonHigh is true (its false) set all pins to 1 else set to 0. Common cathode needs 0 (1 fires segment).
    }//for

    //num digits == 3
    for( i=0; i<3; i++ ) 
    {
        pinMode(commonStart + i, OUTPUT);
        digitalWrite(commonStart + i, commonHigh?0:1); 
    }//for

    KMHour = 0;
    
    Serial.begin(9600);
}

//////////////////////////////////////////////////////////////////////////////////
// HallInterrupt
//  on each falling edge of hall input, hall_count is incremented
//////////////////////////////////////////////////////////////////////////////////
void HallInterrupt( void )
{
    hall_count++;
    
}//HallInterrupt

void MeasureSpeed( void )
{
    int
        calc_hall_count;
    static byte
        speed_state = SPEED_STATE_INIT;
    static unsigned long
        timeWindow;
    unsigned long
        timeNow;
        
    switch( speed_state )
    {        
        case    SPEED_STATE_INIT:
            noInterrupts();
            timeNow = millis();
            hall_count = 0;
            //integration window is SPEED_MEAS_WINDOW mS
            timeWindow = timeNow + SPEED_MEAS_WINDOW;
            interrupts();
            speed_state = SPEED_STATE_MEASURE;
            
        break;

        case    SPEED_STATE_MEASURE:
            timeNow = millis();
            if( timeNow >= timeWindow )
            {
#if 0
                noInterrupts();
                //get a copy of the hall_count and move to compute speed
                calc_hall_count = hall_count;
                interrupts();
                KMHour = (int)((float)calc_hall_count * SPEED_K);
#endif
                KMHour++;   //test
                //limit displayed value to 200kph
                if( KMHour > 999 )
                    KMHour = 0;
                                    
                Serial.println(KMHour);   
                
                speed_state = SPEED_STATE_INIT;
            
            }//if
            
        break;

        default:
            speed_state = SPEED_STATE_INIT;
        break;
        
    }//switch
    
}//MeasureSpeed

void UpdateDisplay(  void )
{
    int
        dval;
    //update the display at 30Hz (~330mS) overall; requires us to have a 10mS step through this
    //state machine
    static int
        last_digit_num = 0;
    static int
        ones, tens, hunds;
    static byte
        displayState = DISP_STATE_SETUP;
    static unsigned long
        displayUpdate = millis() + DISP_UPDATE_TIME;

    if( millis() < displayUpdate )
        return;

    //turn off last digit
    digitalWrite(commonStart + last_digit_num, LOW);
    
    switch( displayState )
    {
        case    DISP_STATE_SETUP:
            //get digits for latest speed
            dval = KMHour;
            hunds = (int)(dval/100);
            dval = dval - 100*hunds;
            tens = (int)(dval/10);
            dval = dval - 10*tens;
            ones = dval;
            displayState = DISP_STATE_DIGIT_HUNDS;
                        
        break;
        
        case    DISP_STATE_DIGIT_HUNDS:
            //if hundreds value is 0 skip it (leading zero)
            if( hunds == 0 )
                writeDigitToDisplay( BLANK );// first send remainder to function then 2nd pass send 10's to function
            else
                writeDigitToDisplay( hunds );
            
            digitalWrite(commonStart + 2, HIGH);
            last_digit_num = 2;
            
            displayUpdate = millis() + DISP_UPDATE_TIME;
            displayState = DISP_STATE_DIGIT_TENS;
        
        break;

        case    DISP_STATE_DIGIT_TENS:
            if( hunds == 0 && tens == 0 )
                writeDigitToDisplay( BLANK );
            else
                writeDigitToDisplay( tens );
            
            digitalWrite(commonStart + 1, HIGH);
            last_digit_num = 1;
            
            displayUpdate = millis() + DISP_UPDATE_TIME;
            displayState = DISP_STATE_DIGIT_ONES;
        
        break;

        case    DISP_STATE_DIGIT_ONES:
            //always display ones
            writeDigitToDisplay( ones );
            
            digitalWrite(commonStart + 0, HIGH);
            last_digit_num = 0;
            
            displayUpdate = millis() + DISP_UPDATE_TIME;
            displayState = DISP_STATE_SETUP;
        
        break;
        
    }//switch
    
}//UpdateDisplay

void loop() 
{
    MeasureSpeed();
    UpdateDisplay();
       
}//loop

void writeDigitToDisplay(int value) 
{
#ifdef  BCD_DECODER
    int
        i,
        bitOn;
        
    if( value == BLANK )
    {
        //for blanked digits (e.g. leading zeros) just set bits "inactive"
        //inactive level depends on commonHigh
        for( i=0; i<4; i++ )
            digitalWrite(BCDStart+i, (commonHigh)?1:0);
    }
    else
    { 
        for( i=0; i<4; i++ ) 
        {
            bitOn = (value & (1<<i))?1:0;
            digitalWrite(BCDStart+i, (commonHigh)?bitOn:!bitOn); //writes bitOn to each BCD pin
  
        }//for
        
    }//else
#else
    byte
        i;
    for( i=0; i<7; i++ )
        digitalWrite( SEVSEGSTART+i, (DigitDecode[value]&(1<<i))?LOW:HIGH );

#endif    
}//writeDigitToDisplay

Forgot to mention on the wiring. Pins 16,3,4 are all high and 8 (Vss) is 0V.

Thanks again

Kev

I think the problem must be around how the commons are switched between the three displays in the program you helped with but I cant see why?

I am running common cathode on 3 displays.

Your test is only using one 7 seg right?

I'm running three digits.

Thanks.

I will try to get an image of the wiring up but I'm pretty sure its fine......

Not sure what to try next?