Compare pulse count to value - Multiple Inputs

Hi all,

I have been tasked with getting a bulb (the ones that go in the ground, not lightbulbs) counting machine to work after its old electronics were given an unexpected 240V.

The machine itself is not vastly complicated. It has an angled vibrating bed with two speeds. Bulbs vibrate down the bed and as they fall from the end hit a lever which briefly uncovers one of 7 optical interrupters. There are 7 runs hence 7 optical interrupters.

Before the machine is started you enter a value on two bcd thumbwheels (anywhere from 0 to 99).
It should then count bulbs until it hits 75 percent of the bcd thumbwheel value at which point it turns off one of the vibration speed relays and slows the vibration, the last 25 percent of the bulbs are then counted until the predefined value and then the vibration stops. Once it stops a 20ms pulse is sent to a relay which in turn sends 24v to a mechanical counter which adds 1 to the mechanical counter total.
The process completes when a door is opened, the bulbs fall into a bag, when the door is closed it hits a microswitch, which resets the arduino and the process starts again.
The microswitch for reset is connected between GND and RESET on the arduino so it does truly reset the device everytime the door is closed.

This has become my job because I am the "IT Guy", this is by no means my forte and I am struggling.

I have for the most part got it working.

I have read into the many discussions about interrupts versus polling and INT versus PCINT, and have struggled to get my head around it. I am using an Arduino mega 2560 and couldn't really get my head around which pins I could and couldn't use. The machine doesn't go particularly fast (maybe 3 bulbs a second across all 7 pins) and as such I am hoping polling will be quick enough to not miss any counts. Using an example I found online I have modified the example StateChangeDetection sketch to work with an array of pins (I am not certain I have done this correctly).

I have cobbled together the following code which does work in terms of getting the values from the bcd thumbwheels, calculaing 75% of bcd value, setting both relays to high (which I chose to do before in setup rather than in loop as they are always going to be needed as soon as the machine starts) and polling the 7 pins (I think).

In order to calculate 75% of the bcd total I did what looks to be some horrible code. It leaves me in a position that this code will compile but with warnings that say.

/home/sam/Arduino/Bulb-Counter/Bulb-Counter.ino: In function 'void loop()':
/home/sam/Arduino/Bulb-Counter/Bulb-Counter.ino:205:20: warning: ISO C++ forbids comparison between pointer and integer [-fpermissive]
 if (bulbcounter >= bcdtot75) digitalWrite(vibrelay2, LOW);
                    ^~~~~~~~
/home/sam/Arduino/Bulb-Counter/Bulb-Counter.ino:206:20: warning: ISO C++ forbids comparison between pointer and integer [-fpermissive]
 if (bulbcounter >= bcdtot)
                    ^~~~~~
/home/sam/Arduino/Bulb-Counter/Bulb-Counter.ino:207:20: warning: ISO C++ forbids comparison between pointer and integer [-fpermissive]
 if (bulbcounter >= bcdtot) {
                    ^~~~~~

I am in a situation where I understand the words being used but cannot make head not tail of what to do about it. Which one is a pointer? Why is it a pointer? Why did I do this? You know that feeling where you've spent two days with a soldering iron in hand thinking you were doing hard bit until you sat down to write the code and realized just how little you know.

I would massively appreciate someone having a look at the below code and telling me if it makes any sense, what I have done wrong and what I should do about it. I am aware my code is not very pretty, it is commented please feel free to improve any or all of it.

Sorry for the long post, having spent my life in IT there is nothing more annoying that too little information.

Thank you

/* Bulb Counting Machine

/* Define Pins For BCD PushWheels */
#define tq1 2
#define tq2 3
#define tq4 4
#define tq8 5
#define uq1 6
#define uq2 7
#define uq4 8
#define uq8 9
/* End Define for BCD PushWheels */


/* Define optipins Array of optical interrupters */


int optipins[7] = {46, 47, 48, 49, 50, 51, 52}; // determine the amount of inputs and the input pins
int optival[7] = {0};                            // variable for reading the opti pin status
int currentState[7] = {0};                      // for comparison with previousstate
int previousState[7] = {0};                     // for comparison with currentstate
int bulbcounter = 0;                             // only one counter needed hence not an array of counters

/* Define Pins For Vibration Bed Relays */
#define vibrelay1 44
#define vibrelay2 45
/* End Define Pins for vibration Bed Relays */

/* Define Pin for 24v Mechanical Counter Relay */
#define mechcount 43
/* Define Pin for 24v Mechanical Counter Relay */

/* DEFINE LED PIN FOR COMPLETED COUNT - CURRENTLY SET FOR ON BOARD DIAG LED */
#define LED_PIN 13
/* SET INITIAL STATE ON LED */

int ledState = LOW;

/* Begin Setup */
void setup()
{
 Serial.begin(115200);
 delay(50);

/* Set BCD Pins to be INPUTS */
/* TENS PushWheel */
 pinMode(tq1, INPUT); // tens-thumbwheel '1'
 pinMode(tq2, INPUT); // tens-thumbwheel '2'
 pinMode(tq4, INPUT); // tens-thumbwheel '4'
 pinMode(tq8, INPUT); // tens-thumbwheel '8'
/* UNITS PushWheel */ 
 pinMode(uq1, INPUT); // units-thumbwheel '1'
 pinMode(uq2, INPUT); // units-thumbwheel '2'
 pinMode(uq4, INPUT); // units-thumbwheel '4'
 pinMode(uq8, INPUT); // units-thumbwheel '8'
/* End Define BCD Pins */

/* Set Optical Interrupter Array to be INPUTS */

  for ( byte i = 0 ; i < 7 ; i ++ ) {
    pinMode(optipins[i], INPUT);        // declare optipins as input
  }

/* End Define Optical Interrupter Array as INPUTS */


/* Set Mechcount Pin to be Output */
pinMode(mechcount, OUTPUT);
/* End Mechchount Pin Definition */

/* Set Vibration Bed Relay Pin to be OUTPUT */
  pinMode(vibrelay1, OUTPUT);    // sets vibrelay1 pin as output
  pinMode(vibrelay2, OUTPUT);    // sets vibrelay2 pin as output
/* End Vibration Bed Relay */

/* Start Vibration at HIGH - So machine starts in high vibration mode*/

digitalWrite(vibrelay1, HIGH);
digitalWrite(vibrelay2, HIGH);
Serial.println("POWER ON RELAYS BOTH HIGH");

/* End Vibration at HIGH */
}

/* CREATE INT CALLED bcdtot and CALCULATE VALUE */
int bcdtot()
{
/* ADD UP TENS PUSHWHEEL */
 int ttot=0;
 if (digitalRead(tq1)==HIGH) { ttot+=1; }
 if (digitalRead(tq2)==HIGH) { ttot+=2; }
 if (digitalRead(tq4)==HIGH) { ttot+=4; }
 if (digitalRead(tq8)==HIGH) { ttot+=8; }
 
/* ADD UP UNITS PUSHWHEEL */
 int utot=0;
 if (digitalRead(uq1)==HIGH) { utot+=1; }
 if (digitalRead(uq2)==HIGH) { utot+=2; }
 if (digitalRead(uq4)==HIGH) { utot+=4; }
 if (digitalRead(uq8)==HIGH) { utot+=8; }
 
/* MULTIPLY TENS BY 10 AND ADD UNITS */
int adtot=ttot*10+utot;
 return adtot;
}
/* END bcdtot */

/* CREATE INT CALLED bcdtot75 and CALCULATE VALUE as 75% of bcdtot. An int is fine here as whole numbers are all I need 
I am certain this is an ugly way to get 75% of bcdtot. Unless, I have completely misunderstood the use on an int. which is completely possible*/

int bcdtot75()
{
int bcdtotper=bcdtot() * 0.75; 
return bcdtotper; 
}


/* END SETUP */

/* BEGIN LOOP */
void loop()
{
if (bulbcounter >= bcdtot75) digitalWrite(vibrelay2, LOW);
if (bulbcounter >= bcdtot) {
  digitalWrite(vibrelay1, LOW);
  digitalWrite(mechcount, HIGH);
  delay (20);
  digitalWrite(mechcount, LOW);
}

for ( byte i = 0 ; i < 7 ; i ++ ) {
    optival[i] = digitalRead(optipins[i]);            // read input value
    if (optival[i] == HIGH) {                         // check if the input is HIGH (optitriggered released)
      currentState[i] = 1;
    }
    else {
      currentState[i] = 0;
    }
    if (currentState[i] != previousState[i]) {
      if (currentState[i] == 1) {
        bulbcounter = bulbcounter + 1;
        //Serial print
        Serial.print(bulbcounter);
        Serial.print(" ; ");
      }
    }
    previousState[i] = currentState[i];
  }
}
/* END LOOP */

The name of a function is a pointer to the function, and that's why you're getting the warning about not being able to compare an int to a pointer.

Did you mean to call the function?

if (bulbcounter >= bcdtot75())

At this point I honestly don't know. I've been at work for so long my brain has begun to melt. I'll look at it again in the morning and I might be able to tell you if I did or not.

Thank you, sometimes the most obvious things have to be almost written in the sky to me to see them.

I now understand that I am effectively trying to call the name of the function (the pointer) rather then its value.

It compiles, I think it makes sense. I'll upload it the machine in the morning.

you've been told that function calls need the "()" following their names, in 2 places

when you configure an input pins in setup(), should you read the input to initialize the state of the pin (possibly avoiding a mis-count?

presumably the BCD inputs are allowed to change while the system is running, otherwise you could just read them once when in setup?

why have bcdtot75() instead of just

    if (bulbcounter >= bcdtot() * 0.75)

optical switches typically don't need debouncing. is this true?

since this will loop pretty quickly, seems that the prints will report the same number often. could track the lastCount and only print the count when it changes

consider how chkButtons() monitors multiple inputs. there's no need to maintain the current state since the previous (last) state is set to the same value.

// check multiple buttons and toggle LEDs

enum { Off = HIGH, On = LOW };

byte pinsLed [] = { 10, 11, 12 };
byte pinsBut [] = { A1, A2, A3 };
#define N_BUT   sizeof(pinsBut)

byte butState [N_BUT];

// -----------------------------------------------------------------------------
int
chkButtons ()
{
    for (unsigned n = 0; n < sizeof(pinsBut); n++)  {
        byte but = digitalRead (pinsBut [n]);

        if (butState [n] != but)  {
            butState [n] = but;

            delay (10);     // debounce

            if (On == but)
                return n;
        }
    }
    return -1;
}

// -----------------------------------------------------------------------------
void
loop ()
{
    switch (chkButtons ())  {
    case 2:
        digitalWrite (pinsLed [2], ! digitalRead (pinsLed [2]));
        break;

    case 1:
        digitalWrite (pinsLed [1], ! digitalRead (pinsLed [1]));
        break;

    case 0:
        digitalWrite (pinsLed [0], ! digitalRead (pinsLed [0]));
        break;
    }
}

// -----------------------------------------------------------------------------
void
setup ()
{
    Serial.begin (9600);

    for (unsigned n = 0; n < sizeof(pinsBut); n++)  {
        pinMode (pinsBut [n], INPUT_PULLUP);
        butState [n] = digitalRead (pinsBut [n]);
    }

    for (unsigned n = 0; n < sizeof(pinsLed); n++)  {
        digitalWrite (pinsLed [n], Off);
        pinMode      (pinsLed [n], OUTPUT);
    }
}

keeping the code simple and clean, is good practice and helps to avoid bugs

To begin, the above did compile with the correct use of brackets (thank you TheMemberFormerlyKnownAsAWOL) and does work pretty well. It could be much nicer and I completely agree with the sentiment of "keeping the code simple and clean". I am learning and things are getting easier.

In answer to your questions.

1.) when you configure an input pins in setup(), should you read the input to initialize the state of the pin (possibly avoiding a mis-count?

Should I? I'm guessing the answer is yes. I had assumed that since all of the optical sensors are closed at machine start that they would all be LOW and as such I wouldn't have to check.

2.) presumably the BCD inputs are allowed to change while the system is running, otherwise you could just read them once when in setup?

The BCD inputs have absolutely no reason to change when the machine is running. It is often set to the same value for many months.

I honestly thought I was reading them once in setup. And then I look back and see that the bcd inputs are read in the weird space I have created between the end of setup and the beginning of loop.

3.) why have bcdtot75() instead of just

 if (bulbcounter >= bcdtot() * 0.75)

Yes indeed, that would make a huge amount more sense. It's mind boggling how obvious this is and how I completely missed it.

4.) optical switches typically don't need debouncing. is this true?

Yes/No/Maybe

The optical interrupters in the machine are not what I am used to. I am used to a 4 pin optical interrupter. These are a 3 pin (5V - GND - Signal) mounted on a small pcb, the sort that I would normally associate with a build your own robot kit,

In practice I have found that these don't need debouncing at all. I have run the machine for hours today and it doesn't double count at all. That is not to say that there shouldn't be some accounting for debounce but in practice it doesn't look like it needs it, so I guess it would just be extra code for no reason.

  1. since this will loop pretty quickly, seems that the prints will report the same number often. could track the lastCount and only print the count when it changes.

This would be much nicer. The serial output is only there so I could initially keep an eye on it and will eventually be commented out as it really should be unnecessary but it be much nicer to read the serial output which is currently a high speed list.

  1. consider how chkButtons() monitors multiple inputs. there's no need to maintain the current state since the previous (last) state is set to the same value..

I have spent 10 minutes looking at chkButtons and am only just beginning to make any sense of it. I will go and look at this harder make a bit more sense of it and come back to you on this.

Even though the machine now works I'm going to rewrite the code anyway. I'm not happy knowing that what I have written is out there running in the clunkiest of fashions.

Thanks for your help, I really appreciate it.

Hi gcjr,

Could you perhaps give me some pointers in how to move the bcd reading code to setup?

I keep getting myself into a position where I either get

a function-definition is not allowed here before '{' token

I am assuming this is because I am trying to put a function into setup and it doesn't like it.

or I get

bcdtot is not declared in this scope.

Which I assume it because setup and loop are different scopes.

I just can't make sense of which bit needs to go where.

Thanks.

If you get an error message with some of your code, and you don't understand it, post the code and post the whole error message.

Do not attempt to paraphrase error messages.

I am going back to basics so have started with just the bcd code. I have moved the bcd code to setup and it all seems to work. The serial output at the bottom is just so i could test it.

I just have a few questions:

Does it make sense and does it conform to a more logical layout?
Should I be defining constants as byte or uint8_t or should I have left them as int?
Are byte and uint8_t actually the same type?

Should I be using byte at all? It seemed to me that byte could be used for storing number of 0 - 255 and that is all I need.

/* Bulb Counter
*  
*/

/* Define const pins For BCD PushWheels */

const byte tq1 = 2;
const byte tq2 = 3;
const byte tq4 = 4;
const byte tq8 = 5;
const byte uq1 = 6;
const byte uq2 = 7;
const byte uq4 = 8;
const byte uq8 = 9;

/* End Define for BCD PushWheels */

byte bcdtot;
byte ttot;
byte utot;


void setup() {
  // put your setup code here, to run once:
 Serial.begin(115200);
 delay(50);

/* Set BCD Pins to be INPUTS */
/* TENS PushWheel */
 pinMode(tq1, INPUT); // tens-thumbwheel '1'
 pinMode(tq2, INPUT); // tens-thumbwheel '2'
 pinMode(tq4, INPUT); // tens-thumbwheel '4'
 pinMode(tq8, INPUT); // tens-thumbwheel '8'
/* UNITS PushWheel */ 
 pinMode(uq1, INPUT); // units-thumbwheel '1'
 pinMode(uq2, INPUT); // units-thumbwheel '2'
 pinMode(uq4, INPUT); // units-thumbwheel '4'
 pinMode(uq8, INPUT); // units-thumbwheel '8'
/* End Define BCD Pins */

 if (digitalRead(tq1)==HIGH) { ttot+=1; }
 if (digitalRead(tq2)==HIGH) { ttot+=2; }
 if (digitalRead(tq4)==HIGH) { ttot+=4; }
 if (digitalRead(tq8)==HIGH) { ttot+=8; }

 if (digitalRead(uq1)==HIGH) { utot+=1; }
 if (digitalRead(uq2)==HIGH) { utot+=2; }
 if (digitalRead(uq4)==HIGH) { utot+=4; }
 if (digitalRead(uq8)==HIGH) { utot+=8; }

bcdtot=ttot*10+utot;

}

void loop() {
  // put your main code here, to run repeatedly:

  Serial.println("ttot");
  Serial.println(ttot);
  delay(1000);
  Serial.println(digitalRead(tq1));
  Serial.println(digitalRead(tq2));
  Serial.println(digitalRead(tq4));
  Serial.println(digitalRead(tq8));

  Serial.println("utot");
  Serial.println(utot);
  delay(1000);
  Serial.println(digitalRead(uq1));
  Serial.println(digitalRead(uq2));
  Serial.println(digitalRead(uq4));
  Serial.println(digitalRead(uq8));
  delay(1000);
  Serial.println("bcdtot");
  Serial.println(bcdtot);
  delay(1000);

}
/* Bulb Counter */

/* Define const pins For BCD PushWheels */
const byte Pins[8] = {2, 3, 4, 5, 6, 7, 8, 9};
const byte Values[8] = {10, 20, 40, 80, 1, 2, 4, 8};
/* End Define for BCD PushWheels */

byte bcdtot;

void setup()
{
  // put your setup code here, to run once:
  Serial.begin(115200);
  delay(50);

  /* Set BCD Pins to be INPUTS */
  for (int p = 0; p < 8; p++)
    pinMode(Pins[p], INPUT);

  // Read the BCD pins and calculate the value
  bcdtot = 0;  // You forgot this part
  for (int p = 0; p < 8; p++)
  {
    if (digitalRead(Pins[p]) == HIGH)
    {
      bcdtot += Values[p];
    }
  }

  Serial.print("bcdtot: ");
  Serial.println(bcdtot);
  delay(1000);
}

void loop() {}

Thanks John,

That is a really neat solution, I hadn't considered putting the values in an array and just adding them up.

I assume that the inputs from the seven 'runs' will be treated very similarly so that might be another place to use arrays.

const byte RunPins[7] = {10, 11, 12, 13, A0, A1, A2};
bool LastState[7];
unsigned long DebounceTimers[7];

void setup()
{
  for (int p=0; p<7; p++)
    pinMode(RunPins[p], INPUT_PULLUP);
}

void loop()
{
  for (int p=0; p<7; p++)
  {
    bool state = digitalRead(RunPins[p]) == HIGH;
    if (state != LastState[p] && 
      millis() - DebounceTimers[p] >= DEBOUNCE_TIME)
    {
      // State Change Detected (and debounced)
      LastState[p] = state;
      DebounceTimers[p] = millis();
      if (state)
        BulbCounter++;  // Another one went by
     }
  }
}
    bulbcounter = bcdtot();

sorry -- correction

int  bulbCount = 0;

void setup () {
    ...
    bulbcount = bcdtot();
    ...
}

void loop ()
{
    if ( (bulbCount * 0.75) < bulbcounter) {
        ...
    }
    ...
}