Code up for review... any (under)takers?

This piece of code has grown quite a bit.
It started with sorting out the interrupts and I received a ton of help from the community.
The I added all the rest and now everything is working.
I still need to calibrate it on a constant flow, so the volume etc calculations are doing rubbish.

It is so big and I have spent so long working on it, I keep on picking up little inefficiencies.
You might be able to spot something I’m doing that I could be doing better.
So I’m putting my neck on the stake here, go ahead and chop!

Any and all help/suggestions are appreciated.
Bashing and breaking is not. I’m human and I make mistakes, still learning :slight_smile:

Thanks!

1010

//Libraries
#include "U8glib.h" //LCD library

//LCD
U8GLIB_PCD8544 u8g(13, 11, 10, 9, 12);  //SCK, MOSI, CS, A0, Reset - LCD pins

#define DISPLAY_RATE   5 // Number loops before the LCD updates the output
int intLCDdisplay = 0;  //Counter for above

//variables
volatile long DirT;	//time between A Falling and B Rising
volatile long HalfrevT;	//time of one half revolution
volatile unsigned long Atrigger;	//time when A is triggered (Falling)
volatile boolean flag = false;	//flag to keep B from being less than A
boolean Dirflag;	//true = clockwise rotation

//calculated values
long Volume;  //L/min
long VolumeKG; //Volume/KG/min
long Frate;  //Flowrate/min
long Weight = 60;  //initial weight

long RPM = 700; // so LED buzzer does not freak out for now - will be dependent on Frate

//storage variables
volatile long Time_store; //time storage
long DirT_store;  //time difference storage
long HalfrevT_store; //half-revolution time storage

//print storage  - used for conversion when printing to the LCD
char volume[10];
char frate[10];
char dirt_store[10];
char halfrevt_store[10];
char weight[10];

//Inputs/Outputs
//LEDs - All on PortC
int T_Red = 18; //A4
int T_Orange = 17;
int Green = 16;
int B_Orange = 15;
int B_Red = 14; //A0

//Buzzer
int Buzzer = 19; //A5

//Buttons
const int Left = 4;  //< button
const int OK = 5;  //* button
const int Right = 6;  //> button
int L_Button_state = 0;  //pointer for < button
int R_Button_state = 0;  //pointer for * button
int OK_Button_state = 0;  //pointer for > button
int change_flag = 1;  //flag to indicate weight has increased/decreased

//Function declaration
void Weight_input();    //Weight input function
boolean Direction();	//Calculates the ratio between DirT and halfrev
long calc_Volume();	//Calculates the volume of air passing through the turbine and outputs data to LCD
long calc_FRate();	//Calculates the rate of flow through the turbine
void LEDs_Buzzer();	//Determines which LED to light and if the Buzzer should sound

//Interrupts
void A_ISR()	//Interrupt service routine for A(Falling)
{
  Time_store = micros();	//Time is stored since two values need to be set to micros()
  if (!flag)
  {
    HalfrevT = Time_store - Atrigger;	//halfrevolution is current time - previous trigger time
    Atrigger = Time_store;	//sets trigger to the stored time
  }
}

void B_ISR()	//Interrupt service routine for B(rising)
{
  if (!flag)
  {
    DirT = micros() - Atrigger;	//DirT is current time - the time A was triggered
    flag = true;	//sets the flag so that A is always less than B
  }
}

void setup()
{

  DDRD = DDRD | B10000000;  //setting pins 0-6 as inputs - faster than (pinMode(Left, INPUT); & pinMode(OK, INPUT); & pinMode(Right, INPUT);

  u8g.firstPage();
  do
  {
    Innovus();
  }
  while(u8g.nextPage());
  delay(2000); //wait a bit...

  u8g.setFont(u8g_font_unifont);  //sets the Font away from italic for Innovus Logo

  //Ask to rotate the display?  --Library only permits one call to rotation

  Weight_input();  //Ask the user if he/she is happy with the weight - buttons do not always register... fix/debounce?

  //Set Interrupts
  Atrigger = micros();  //assign a value so it is != zero
  attachInterrupt(0, A_ISR, FALLING);    //A is pulled low by Pin 2
  attachInterrupt(1, B_ISR, RISING);     //B rises on Pin 3
}

void loop() //reset button needs to be added to circuit...
{
  if (flag)
  {
    if (DirT>0)
    {
      //Store variables for printing
      DirT_store = DirT; 
      HalfrevT_store = HalfrevT; 
      flag = false;
      //Calculations
      
      Direction();
      calc_Volume();
      calc_FRate();

      if ( (intLCDdisplay++ % DISPLAY_RATE) == 0 ) //LCD prints only every x loops to avoid slowing down entire sketch.
      {
        LEDs_Buzzer();  //Lights up LEDs and sounds buzzer
        u8g.firstPage();
        do
        {
          Data();
        }
        while( u8g.nextPage() );
      }
    }
  }
}

//Functions (rubbish calculations except for direction)
boolean Direction()   //calculates a ratio using the time difference and half revolution time
{
  if (HalfrevT_store/DirT_store<4)
  {
    Dirflag = true; //clockwise/IN
  }
  else
  {
    Dirflag = false;
  }
}

long calc_Volume()
{
  Volume = HalfrevT_store*1.035;
  VolumeKG = Volume/Weight;
  if (Dirflag) //clockwise/IN
  {
    //add Volume to Volume_IN
  }
  else
  {
    //add Volume to Volume_OUT
  }
  return Volume;
}

long calc_FRate()
{
  Frate = 1.02*1586*pow(HalfrevT_store/100,-1.013); //revolution time is in Micros instead of Millis
  //  if (Dirflag)
  //  {
  //    //total flowrate?
  //  }
  //  else
  //  {
  //  }
  return Frate;
}

void LEDs_Buzzer()
{
  if (RPM>2000)
  {
    PORTC = B00110000; //A4/T_Red high & A5/Buzzer high
  }
  else if (RPM>1500)
  {
    PORTC = B00001000; //A3/T_Orange high
  }
  else if (RPM>1000)
  {
    PORTC = B00000100; //A2/Green high
  }
  else if (RPM>500)
  {
    PORTC = B00000010; //A1/B_Orange high
  }
  else
  {
    PORTC = B00000001; //A0/B_Red high
  }
}

void Weight_input()
{
  do  //do while waiting for OK/* button to be pressed
  {
    if (change_flag == 1)
    {
      u8g.firstPage();
      do
      {
        W_input();
      }
      while(u8g.nextPage());
      change_flag = 0;
    }

    //save button states
    L_Button_state = digitalRead(Left);  
    R_Button_state = digitalRead(Right);
    OK_Button_state = digitalRead(OK);
    delay(100);

    //< Button
    if (L_Button_state == LOW)
    {
      if (Weight >=10)
      {
        Weight = Weight-5;
        change_flag = 1;
      }
      else if (Weight >=2)
      {
        Weight = Weight-1;
        change_flag = 1;
      }
      else
        change_flag = 0;    //Add 100 miligram set?
      L_Button_state = HIGH;
    }
    else
      L_Button_state = LOW;

    //> Button
    if (R_Button_state == LOW)
    {
      if (Weight >=5)
        Weight = Weight+5;
      else if (Weight >=1)
        Weight = Weight+1;
      change_flag = 1;
      R_Button_state = HIGH;
    }
    else
      R_Button_state = LOW;
  }
  while(OK_Button_state == HIGH); // Wait for OK button to go low

  u8g.firstPage();
  do
  {
    u8g.drawStr( 17, 22, "Start!");
  }
  while(u8g.nextPage());
  delay(3000);
}

void W_input(void)
{
  sprintf(weight, "%ld", Weight);
  u8g.drawStr( 0, 10, "Weight:");
  u8g.drawStr( 20, 28, weight);
  u8g.drawStr( 44, 28, "KG");
  u8g.drawStr( 2, 47, "-   Ok   +");
}

void Data(void)
{
  //http://www.cplusplus.com/reference/clibrary/cstdio/sprintf/
  //convert all values since LCD can't print

  sprintf(volume, "%ld", Volume);
  sprintf(dirt_store, "%ld", DirT_store);
  sprintf(halfrevt_store, "%ld", HalfrevT_store);
  sprintf(frate, "%ld", Frate); //convert Frate from Long float to string
  //Frate is totally bugger, can't get Decimals to show... float/double just blanks

  //Draw strings
  u8g.drawStr( 60, 11, "L/M");
  u8g.drawStr( 0, 11, volume);
  u8g.drawStr( 0, 23, frate);
  u8g.drawStr( 0, 35, dirt_store);
  u8g.drawStr( 0, 47, halfrevt_store);

  //  sprintf(rpm, "%ld", RPM);
  //u8g.drawStr( 60, 23, "RPM");
  //u8g.drawStr( 0, 23, rpm);

  if (Dirflag)
  {
    u8g.drawStr( 60, 47, "In");
  }
  else
  {
    u8g.drawStr( 60, 47, "Out");
  }
}
--cont on next post

Too much for one post:

//Bitmap for Innovus Logo
const uint8_t Innovus_bit[] PROGMEM =
{
  0x0, 0x3,    0xE0,   0x0,    0x0,    0x0,
  0x0, 0x7,    0xF0,   0x1F,   0x0,    0x0,
  0x0, 0x7,    0xF0,   0x3F,   0x80,   0x0,
  0x0, 0xF,    0xF8,   0x3F,   0x80,   0x0,
  0x0, 0xF,    0xF8,   0x3F,   0x80,   0x0,
  0x0, 0x7,    0xF0,   0x1F,   0x0,    0x0,
  0x0, 0x7,    0xF0,   0x1F,   0x0,    0x0,
  0x0, 0x3,    0xE0,   0x1F,   0x0,    0x0,
  0x0, 0x0,    0x0,    0x1F,   0x0,    0x0,
  0x0, 0x0,    0x0,    0x1F,   0x0,    0x0,
  0x0, 0x7,    0xF0,   0x1F,   0x0,    0x0,
  0x0, 0x7,    0xF0,   0x1F,   0x0,    0x0,
  0x0, 0x7,    0xFF,   0xFF,   0x0,    0x0,
  0x0, 0xF,    0xF0,   0xF,    0xF0,   0x0,
  0x0, 0x7F,   0xF0,   0xE,    0x1E,   0x0,
  0x1, 0xC7,   0xF0,   0xE,    0x3,    0x80,
  0x7, 0x7,    0xF0,   0xE,    0x0,    0xE0,
  0xC, 0x7,    0xF0,   0xE,    0x0,    0x30,
  0x18,    0x7,    0xFF,   0xFE,   0x0,    0x18,
  0x18,    0xF,    0xF0,   0x7,    0xF0,   0x18,
  0xC, 0x7F,   0xF0,   0x4,    0x1E,   0x30,
  0x7, 0xC7,   0xF0,   0x4,    0x3,    0xE0,
  0x7, 0xC7,   0xF0,   0x4,    0x3,    0xE0,
  0xC, 0x7F,   0xF0,   0x0,    0x1E,   0x30,
  0x18,    0xF,    0xF0,   0x3,    0xF0,   0x18,
  0x18,    0x7,    0xFF,   0xFC,   0x0,    0x18,
  0xC, 0x7,    0xF0,   0x0,    0x0,    0x30,
  0x7, 0x7,    0xF0,   0x1F,   0x0,    0xE0,
  0x1, 0xC7,   0xF0,   0x3F,   0x83,   0x80,
  0x0, 0x7F,   0xF0,   0x3F,   0x9E,   0x0,
  0x0, 0xF,    0xF0,   0x7F,   0xF0,   0x0,
  0x0, 0x7,    0xFF,   0xFF,   0xC0,   0x0,
  0x0, 0x7,    0xF0,   0x3F,   0x80,   0x0,
  0x0, 0x7,    0xF0,   0x3F,   0x80,   0x0,
  0x0, 0x0,    0x0,    0x1F,   0x0,    0x0,
};

//Innovus Printout
void Innovus(void)
{
  u8g.setFont(u8g_font_8x13Or);
  u8g.drawBitmapP( 18, 1, 6, 35, Innovus_bit);
  u8g.drawStr( 14, 44, "innovus");
}

I can't see anything obviously wrong, except a couple of style issues, and they tend to be matters of opinion. However looking at this:

void loop() //reset button needs to be added to circuit...
{
  if (flag)
  {
    if (DirT>0)
    {
  
...

      if ( (intLCDdisplay++ % DISPLAY_RATE) == 0 ) //LCD prints only every x loops to avoid slowing down entire sketch.
      {
...
        do
        {
          Data();
        }
        while( u8g.nextPage() );
      }
    }
  }
}

I think you'll appreciate it later on if you put comments in saying what the closing curly braces do, like this:

void loop() //reset button needs to be added to circuit...
{
  if (flag)
  {
    if (DirT>0)
    {
  
...

      if ( (intLCDdisplay++ % DISPLAY_RATE) == 0 ) //LCD prints only every x loops to avoid slowing down entire sketch.
      {
...
        do
        {
          Data();
        }  // end do
        while( u8g.nextPage() );
      }  // end if time to display
    }  // end DirT > 0
  }  // end if flag true
}  // end loop

This helps you later on if you ever decide to add something after one "if" but before another.


OK, one style thing ...

void LEDs_Buzzer()
{
  if (RPM>2000)
  {
    PORTC = B00110000; //A4/T_Red high & A5/Buzzer high
  }
  else if (RPM>1500)
  {
    PORTC = B00001000; //A3/T_Orange high
  }
  else if (RPM>1000)
  {
    PORTC = B00000100; //A2/Green high
  }
  else if (RPM>500)
  {
    PORTC = B00000010; //A1/B_Orange high
  }
  else
  {
    PORTC = B00000001; //A0/B_Red high
  }
}

I don't think the braces add much to the above except make it harder to read. Some people will disagree with me and say you should always use them (but I note that you don't, always, further on in the sketch). This is easier to read IMHO like this:

void LEDs_Buzzer()
{
  if (RPM > 2000)
    PORTC = B00110000; //A4/T_Red high & A5/Buzzer high
  else if (RPM > 1500)
    PORTC = B00001000; //A3/T_Orange high
  else if (RPM > 1000)
    PORTC = B00000100; //A2/Green high
  else if (RPM > 500)
    PORTC = B00000010; //A1/B_Orange high
  else
    PORTC = B00000001; //A0/B_Red high
}

Oh, and put spaces between comparison (and other) operators like I did above. Think how hard English would be to read if you didn't have spaces after commas, periods, and other punctuation.

Nothing much to criticise, just some more stylistic things to help make the code easier to understand:

Having variables with the same name, such as Weight and weight really isn’t a very good idea.

Some of your other names are a bit terse, would it hurt to type Flowrate rather than FRate?

Shouldn’t change_flag be a boolean?

Can we have a summary of what it does?