[Solved] Adjusting shift register outputs to the signal of light barriers

Using Arduino Programming Questions
1

Hi,
I'm having problems with a project I'm working on right now.

First of all I will explain what the project is about and what I'm trying to do:

I have built a plate witch six holes arranged like a pyramid in it. Every hole is surrounded by 4 LEDs of which two are serially connected (so they are just behaving as if it was one LED). Next to every hole there is a reflection light barrier. For a picture see attachment: plate prototype

What is the Goal?
Goal is to light up the 4 LEDs around a hole as soon as a plastic cup was placed in the hole. If the cup is beeing removed the 4 LEDs switch of again. This of course working for all of the six holes.

(This plate is a prototype and in the final version i will be having two pyramids with 10 holes each.)

The hardware part:

I'm using an Arduino Duemilanove.

Since the plate has 6 holes with two LEDs each (2x 2 serially connected) I would need 12 digital outputs to controll them all. And since i will be needing 20x2 outputs in the final version I'm using two 74HC595 shiftregister to multiplex the ouputs (so now I have 16 outputs but just using 12 of them).
I'm also using two ULN2803 to supply the LEDs with electrical power.
The two ICs are connected like this http://www.instructables.com/files/orig/F5U/CQS8/GWF5UFZU/F5UCQS8GWF5UFZU.png
But just with two 74HC595 and two ULN2803.

For checking if there is a cup I'm using reflection light barriers. These give me an analog signal which is very low if there is a cup and very high if there is no cup. For multiplexing the analog inputs I'm using a 74HC4051.

I dont think we need to worry about the hardware part unless of course if you have any genius suggestions for improvement :wink: But the hardware part is working very well. Means I'm able to control the LEDs as i want and I'm able to read out every single light barrier separately.

Picture: Attachment Hardware

So whats my problem then? Where am I at?

I have written a program that does the exact thing I have described in "What is the Goal" BUT the LEDs are not as bright as they should be. This is because in the beginning of the loop all the LEDs are being switched off and during the loop while the program is checking where a cup is it switches on the LEDs around that hole. As the program gets to the next hole it switches on the LEDs around that hole if there is a cup but also switches of all the other leds. That means all LEDs are constantly being switched on and off again which reduces their brightness extremely.

So what to do?
One thing I could theoretically do is defining every possible situation that could possibly occur and tell the programm what to do in which case. So: "If cup1 high ; cup2 high ;.....;cup 6 low" switch on that that and that LED. That would certainly work but since there are six holes there are 2^6 possibilities (in the final version 2^10) that would be just stupid.
The other possibility would be to instead of switching on only the leds around the hole the program is checking at the time and switching of all the other LEDs get the shiftregister to adjust its output somehow. But thats exactly my problem.

So lets say the program notices that there is a cup in hole one. It will send the first shift register a "0" and the second shift register a "3" because these are the value that will light up the LEDs around hole one.
Now the program gets to the second hole and notices that there is a cup.
The combonation to light up the LEDs around hole two is ShiftReg1 = 0 and ShiftReg2 = 12. But if I send this to the shiftRegisters it would switch of the LEDs around hole one again and again the LEDs would appear less bright.
So if there would be a way to to just "add" the second LEDs to the first ones without defining 2^6 possibilities that would be the solution. Something like: "oh there is a cup in hole two so we somehow take the ShiftReg1 = 0 and ShiftReg2 = 12 and add that to whatever the shiftRegister where before in this case ShiftReg1 = 0 and ShiftReg2 = 3 and magically we and up with ShiftReg1 = 0 and ShiftReg2 = 15 OK BAD EXAMPLE because in this case that wouldnt be too hard to do but i think that is because the two holes are the first two and they are next to each other. I dont think that would work for: "Light up Cup3 and Cup6"...

For your understanding here are the ShiftRegister values for every hole:

Hole 1: ShiftReg1 = 0 and ShiftReg2 = 3
Hole 2: ShiftReg1 = 0 and ShiftReg2 = 12
Hole 3: ShiftReg1 = 0 and ShiftReg2 = 48
Hole 4: ShiftReg1 = 3 and ShiftReg2 = 0
Hole 5: ShiftReg1 = 12 and ShiftReg2 = 0
Hole 6: ShiftReg1 = 0 and ShiftReg2 = 192

These will light up the 4 LEDs around the hole. (see the order attachment)

So here is the code that I'm using right now (the one that is working but not very bright :smiley: )
As you can tell from the code I'm very new to programming and arduinos. Keep that in mind while trying to explain something to me :wink:

//defining pins

int S0 = 8;
int S1 = 9;
int S2 = 10;

int SensorPin = A0;

int dataPin = 2;        
int latchPin = 3;
int clockPin = 4;


//defining variables

int sCup1 ;
int sCup2 ;
int sCup3 ;
int sCup4 ;
int sCup5 ;
int sCup6 ;


void setup()
{
    //Configure each IO Pin
    pinMode(S0, OUTPUT);      
    pinMode(S1, OUTPUT);
    pinMode(S2, OUTPUT);
    pinMode(SensorPin, INPUT);
    
    pinMode(dataPin, OUTPUT);       
    pinMode(latchPin, OUTPUT);
    pinMode(clockPin, OUTPUT);
         
}

void loop() {
  
  //read out sensor cup 1
  digitalWrite (S0, LOW);
  digitalWrite (S1, LOW);
  digitalWrite (S2, LOW);
  int sCup1 = analogRead (SensorPin);
  
    //read out sensor cup 2
  digitalWrite (S0, HIGH);
  digitalWrite (S1, LOW);
  digitalWrite (S2, LOW);
  int sCup2 = analogRead (SensorPin);
  
    //read out sensor cup 3
  digitalWrite (S0, LOW);
  digitalWrite (S1, HIGH);
  digitalWrite (S2, LOW);
  int sCup3 = analogRead (SensorPin);
  
    //read out sensor cup 4
  digitalWrite (S0, HIGH);
  digitalWrite (S1, HIGH);
  digitalWrite (S2, LOW);
  int sCup4 = analogRead (SensorPin);
  
    //read out sensor cup 5
  digitalWrite (S0, LOW);
  digitalWrite (S1, LOW);
  digitalWrite (S2, HIGH);
  int sCup5 = analogRead (SensorPin);
  
    //read out sensor cup 6
  digitalWrite (S0, HIGH);
  digitalWrite (S1, LOW);
  digitalWrite (S2, HIGH);
  int sCup6 = analogRead (SensorPin);
  

   //switch off all LEDs
   
        digitalWrite(latchPin, LOW);            
        shiftOut(dataPin, clockPin, MSBFIRST, 0);         
        shiftOut(dataPin, clockPin, MSBFIRST, 0);
        digitalWrite(latchPin, HIGH);   
   
   //LEDs Cup1
   
     if (sCup1 < 500)
  
   { 
        digitalWrite(latchPin, LOW);            
        shiftOut(dataPin, clockPin, MSBFIRST, 0);         
        shiftOut(dataPin, clockPin, MSBFIRST, 3);
        digitalWrite(latchPin, HIGH);          
   }
   
   

   //LEDs Cup2
   
        if (sCup2 < 500)
        
   { 
        digitalWrite(latchPin, LOW);            
        shiftOut(dataPin, clockPin, MSBFIRST, 0);         
        shiftOut(dataPin, clockPin, MSBFIRST, 12);
        digitalWrite(latchPin, HIGH);          
   }
   

  
   //LEDs Cup3
   
        if (sCup3 < 500)
  
   { 
        digitalWrite(latchPin, LOW);            
        shiftOut(dataPin, clockPin, MSBFIRST, 0);         
        shiftOut(dataPin, clockPin, MSBFIRST, 48);
        digitalWrite(latchPin, HIGH);          
   }
   
 
  
   //LEDs Cup4
   
        if (sCup4 < 500)
  
   { 
        digitalWrite(latchPin, LOW);            
        shiftOut(dataPin, clockPin, MSBFIRST, 0);         
        shiftOut(dataPin, clockPin, MSBFIRST, 192);
        digitalWrite(latchPin, HIGH);          
   }
   

  
   //LEDs Cup5
   
        if (sCup5 < 500)
  
   { 
        digitalWrite(latchPin, LOW);            
        shiftOut(dataPin, clockPin, MSBFIRST, 3);         
        shiftOut(dataPin, clockPin, MSBFIRST, 0);
        digitalWrite(latchPin, HIGH);          
   }
   

  
     //LEDs Cup6
   
        if (sCup6 < 500)
  
   { 
        digitalWrite(latchPin, LOW);            
        shiftOut(dataPin, clockPin, MSBFIRST, 12);         
        shiftOut(dataPin, clockPin, MSBFIRST, 0);
        digitalWrite(latchPin, HIGH);          
   }
   

  
}

Thank you in advance for your time and let me know if you need any more information :slight_smile:

hardware.jpg
hardware.jpg816×612 217 KB

plate prototype.jpg
plate prototype.jpg816×612 165 KB

order.jpg
order.jpg662×568 36.5 KB

2

I suggest you declare a variable to hold the state of all the LEDs. The most obvious representation IMO would be to hold that state as a bitmask using the same layout that you're using for the shift register.

Do your input reading stuff and update the required state of LEDs as a result. The Arduino runtime library provides convenience functions to let you set and clear individual bits in a number, ot you can use the conventional C++ bitwise operators.

Once you have updated the required state of the LEDs, write that state out to the shift register.

Impact on your code should be negligible - instead of writing each LED state to the shift register,replace that with code to write the LED state to your variable; at the end of the loop, add a statement to write that variable out to the shift registers.

3

Alright, thank you very much that already helped a lot. As I said I'm very new to all this and had never heard of bitmasks. But I read into the topic and wrote a new program that surprisingly worked after a few tries (pretty much :smiley: )

So thats how my program looks like right now:

//defining pins

int S0 = 8;
int S1 = 9;
int S2 = 10;

int SensorPin = A0;

int dataPin = 2;        
int latchPin = 3;
int clockPin = 4;

//defining variables


byte LEDpattern1 = B00000000;
byte LEDpattern2 = B00000000;

byte maskCup1 = B00000011;
byte maskCup2 = B00001100;
byte maskCup3 = B00110000;
byte maskCup4 = B11000000;
byte maskCup5 = B00000011;
byte maskCup6 = B00001100;


int sCup1 ;
int sCup2 ;
int sCup3 ;
int sCup4 ;
int sCup5 ;
int sCup6 ;


void setup()
{
    //Configure each IO Pin
    pinMode(S0, OUTPUT);      
    pinMode(S1, OUTPUT);
    pinMode(S2, OUTPUT);
    pinMode(SensorPin, INPUT);
    
    pinMode(dataPin, OUTPUT);       
    pinMode(latchPin, OUTPUT);
    pinMode(clockPin, OUTPUT);
    
     
}

void loop() {
  
  //read out sensor cup 1
  digitalWrite (S0, LOW);
  digitalWrite (S1, LOW);
  digitalWrite (S2, LOW);
  int sCup1 = analogRead (SensorPin);
  
    //read out sensor cup 2
  digitalWrite (S0, HIGH);
  digitalWrite (S1, LOW);
  digitalWrite (S2, LOW);
  int sCup2 = analogRead (SensorPin);
  
    //read out sensor cup 3
  digitalWrite (S0, LOW);
  digitalWrite (S1, HIGH);
  digitalWrite (S2, LOW);
  int sCup3 = analogRead (SensorPin);
  
    //read out sensor cup 4
  digitalWrite (S0, HIGH);
  digitalWrite (S1, HIGH);
  digitalWrite (S2, LOW);
  int sCup4 = analogRead (SensorPin);
  
    //read out sensor cup 5
  digitalWrite (S0, LOW);
  digitalWrite (S1, LOW);
  digitalWrite (S2, HIGH);
  int sCup5 = analogRead (SensorPin);
  
    //read out sensor cup 6
  digitalWrite (S0, HIGH);
  digitalWrite (S1, LOW);
  digitalWrite (S2, HIGH);
  int sCup6 = analogRead (SensorPin);
  

      
   
   //LEDs Cup1
   
if (sCup1 < 500)
  
   { 
       LEDpattern1 = (LEDpattern1 | maskCup1);
   }

   else {
   }
   

   //LEDs Cup2
   
 if (sCup2 < 500)
        
   { 
      LEDpattern1 = (LEDpattern1 | maskCup2);        
   }

 else  {
   }

  
   //LEDs Cup3
   
 if (sCup3 < 500)
  
   { 
      LEDpattern1 = (LEDpattern1 | maskCup3);          
   }
  
 else {
 }

  
   //LEDs Cup4
   
 if (sCup4 < 500)
  
   { 
      LEDpattern1 = (LEDpattern1 | maskCup4);         
   }
   
else {
 }
  

   //LEDs Cup5
   
        if (sCup5 < 500)
  
   { 
      LEDpattern2 = (LEDpattern2 | maskCup5);          
   }
   
else {
 }
  

     //LEDs Cup6
   
  if (sCup6 < 500)
  
    { 
      LEDpattern2 = (LEDpattern2 | maskCup6);        
    }
  
else {
}        
        
        digitalWrite(latchPin, LOW);            
        shiftOut(dataPin, clockPin, MSBFIRST, LEDpattern2);         
        shiftOut(dataPin, clockPin, MSBFIRST, LEDpattern1);
        digitalWrite(latchPin, HIGH); 
   
}

So what that program does is switch on the right LEDs that belog to the hole.
So thats good.
What it obviously doesn't do is switch of the LEDs off if the cup is being removed.

This is because I couldn't think of a way to tell the program "else: just turn those two bits of that certain cup into 0 and leave the rest as it is stored in the variable"

Is there a way to just manipulate certain bits of the byte and leave the rest as it is?

4

Is that not what you are doing already ?
What does

B01010101 | B10000000

give you ?

5

Well you are right

B01010101 | B10000000

gives me

B11010101

Let me try to explain what I think my problem is by simulating a run through the program:

We start of with the LEDpattern being: B00000000 // all LEDs switched off

First Cup is in the hole

LEDpattern = (LEDpattern | maskCup1); // maskCup1 is B00000011

now the LEDpattern is B00000011 // LEDs for the first cup are switched on

Second Cup is in the hole

LEDpattern = (LEDpattern | maskCup2); // maskCup2 is B00001100

now the LEDpattern is B00001111 // LEDs for the first and second cup are switched on

This goes on till the last cup:

LEDpattern = (LEDpattern | maskCup4); // maskCup4 is B11000000

now the LEDpattern is 11111111 // LEDs for all cups are switched on

So thats all working fine, even if the cups are inserted in a different order or not all of them.

Now we get to the problem.

The case is: We take out the first Cup and go through the program again.

If I would say something like:

LEDpattern = (LEDpattern & maskNoCup1); // maskNoCup1 is B00000000

I would end up with LEDpattern B00000000 which would switch off all the LEDs although I just wanted to switch off the LEDs for the first cup.
I think it isn't possible to just put a mask over the LEDpattern again because then I would say something about bits 1 to 6 as well. But I cannot do that because that would switch off other LEDs although the cups are still there or the other way round it would switch on LEDs although the cups they belong to aren't there anymore.

I just want bits 1 to 6 to say what they are but since they could be 1 or 0 it is not possible to have a mask that always works like during the "switching on process".

I hope I could express myself well enough to explain the problem :wink:

6

hlawan:

LEDpattern = (LEDpattern | maskCup4); // maskCup4 is B11000000

The code above does a bitwise OR to set bits in a variable, where the bits are defined by a bitmask.

The code to clear them is much less obvious to understand, but surprisingly simple:

LEDpattern = (LEDpattern & ~maskCup4); // maskCup4 is B11000000

The ~ (tilde) operator gives you the bitwise inverse of the mask ("ones' complement") i.e. it inverts each bit. In this example it would give you B00111111.

The & (bitwise AND) operator then clears the bits which are zero in ~maskCup4. The result in this case is that it clears the top two bits and leaves the other bits unchanged.

Other options open to you are to use the Arduino function bitWrite(), bitSet(), bitClear() which let you read and update individual bits in a value without having to think about bitmasks. In this case where you seem to be dealing with multiple bits at the same time, the bitmasking approach you're using would produce more compact code.

7

Thank you very much :slight_smile:

The bitClear() funktion was exactly what i needed. Now the program is running fine and everything works.

Heres the code:

//defining pins

int S0 = 8;
int S1 = 9;
int S2 = 10;

int SensorPin = A0;

int dataPin = 2;        
int latchPin = 3;
int clockPin = 4;


//defining variables


byte LEDpattern1 = B00000000;
byte LEDpattern2 = B00000000;

byte maskCup1 = B00000011;
byte maskCup2 = B00001100;
byte maskCup3 = B00110000;
byte maskCup4 = B11000000;
byte maskCup5 = B00000011;
byte maskCup6 = B00001100;



int sCup1 ;
int sCup2 ;
int sCup3 ;
int sCup4 ;
int sCup5 ;
int sCup6 ;


void setup()
{
  //Configure each IO Pin
  pinMode(S0, OUTPUT);      
  pinMode(S1, OUTPUT);
  pinMode(S2, OUTPUT);
  pinMode(SensorPin, INPUT);

  pinMode(dataPin, OUTPUT);       
  pinMode(latchPin, OUTPUT);
  pinMode(clockPin, OUTPUT);
     
}

void loop() {

  //read out sensor cup 1
  digitalWrite (S0, LOW);
  digitalWrite (S1, LOW);
  digitalWrite (S2, LOW);
  int sCup1 = analogRead (SensorPin);

  //read out sensor cup 2
  digitalWrite (S0, HIGH);
  digitalWrite (S1, LOW);
  digitalWrite (S2, LOW);
  int sCup2 = analogRead (SensorPin);

  //read out sensor cup 3
  digitalWrite (S0, LOW);
  digitalWrite (S1, HIGH);
  digitalWrite (S2, LOW);
  int sCup3 = analogRead (SensorPin);

  //read out sensor cup 4
  digitalWrite (S0, HIGH);
  digitalWrite (S1, HIGH);
  digitalWrite (S2, LOW);
  int sCup4 = analogRead (SensorPin);

  //read out sensor cup 5
  digitalWrite (S0, LOW);
  digitalWrite (S1, LOW);
  digitalWrite (S2, HIGH);
  int sCup5 = analogRead (SensorPin);

  //read out sensor cup 6
  digitalWrite (S0, HIGH);
  digitalWrite (S1, LOW);
  digitalWrite (S2, HIGH);
  int sCup6 = analogRead (SensorPin);



  //LEDs Cup1

  if (sCup1 < 500)
  { 
    LEDpattern1 = (LEDpattern1 | maskCup1);
  }

  else 
  {
    bitClear(LEDpattern1 , 0);
    bitClear(LEDpattern1 , 1);
  }


  //LEDs Cup2

  if (sCup2 < 500)
  { 
    LEDpattern1 = (LEDpattern1 | maskCup2);        
  }

  else 
  {
    bitClear(LEDpattern1 , 2);
    bitClear(LEDpattern1 , 3);
  }


  //LEDs Cup3

    if (sCup3 < 500)
  { 
    LEDpattern1 = (LEDpattern1 | maskCup3);          
  }

  else 
  {
    bitClear(LEDpattern1 , 4);
    bitClear(LEDpattern1 , 5);
  }

  //LEDs Cup4

  if (sCup4 < 500)
  { 
    LEDpattern1 = (LEDpattern1 | maskCup4);  
  }    

  else 
  {
    bitClear(LEDpattern1 , 6);
    bitClear(LEDpattern1 , 7);
  }

  //LEDs Cup5

  if (sCup5 < 500)
  { 
    LEDpattern2 = (LEDpattern2 | maskCup5);          
  }

  else 
  {
    bitClear(LEDpattern2 , 0);
    bitClear(LEDpattern2 , 1);
  }

  //LEDs Cup6

  if (sCup6 < 500)
  { 
    LEDpattern2 = (LEDpattern2 | maskCup6);        
  }

  else 
  {
    bitClear(LEDpattern2 , 2);
    bitClear(LEDpattern2 , 3);
  }       


  ´//shifting out the data to the shift register

  digitalWrite(latchPin, LOW);            
  shiftOut(dataPin, clockPin, MSBFIRST, LEDpattern2);         
  shiftOut(dataPin, clockPin, MSBFIRST, LEDpattern1);
  digitalWrite(latchPin, HIGH); 

}
8

Well done on solving your problem.

Looking at your code I see variables like this

byte maskCup1 = B00000011;
byte maskCup2 = B00001100;
byte maskCup3 = B00110000;
byte maskCup4 = B11000000;
byte maskCup5 = B00000011;
byte maskCup6 = B00001100;

and

int sCup1 ;
int sCup2 ;
int sCup3 ;
int sCup4 ;
int sCup5 ;
int sCup6 ;

To me that looks like it is a prime candidate for the use of arrays which would mean that you could shorten the code that reads the values into sCup1, sCup2, sCup3 etc and creates the LEDpattern values. Changing your code may seem like a bit of a chore but it will pay off when you expand the size of your from 6 holes to 20 holes.

9

Alright, I didn't have too much time today to read about arrays but I think I understood what they are about.

Here is the new code:

//defining pins

int S0 = 8;
int S1 = 9;
int S2 = 10;

int SensorPin = A0;

int dataPin = 2;        
int latchPin = 3;
int clockPin = 4;


//defining variables

int sCup[6] ;

byte LEDpattern[] = {B00000000, B00000000};

byte maskCup[] = {B00000011, B00001100, B00110000, B11000000, B00000011, B00001100};




void setup()
{
  //Configure each IO Pin
  pinMode(S0, OUTPUT);      
  pinMode(S1, OUTPUT);
  pinMode(S2, OUTPUT);
  pinMode(SensorPin, INPUT);

  pinMode(dataPin, OUTPUT);       
  pinMode(latchPin, OUTPUT);
  pinMode(clockPin, OUTPUT);

  //initialize serial communication
  Serial.begin(9600);       
}

void loop() {

  //read out sensor cup 1
  digitalWrite (S0, LOW);
  digitalWrite (S1, LOW);
  digitalWrite (S2, LOW);
  sCup[0] = analogRead (SensorPin);

  //read out sensor cup 2
  digitalWrite (S0, HIGH);
  digitalWrite (S1, LOW);
  digitalWrite (S2, LOW);
  sCup[1] = analogRead (SensorPin);

  //read out sensor cup 3
  digitalWrite (S0, LOW);
  digitalWrite (S1, HIGH);
  digitalWrite (S2, LOW);
  sCup[2] = analogRead (SensorPin);

  //read out sensor cup 4
  digitalWrite (S0, HIGH);
  digitalWrite (S1, HIGH);
  digitalWrite (S2, LOW);
  sCup[3] = analogRead (SensorPin);

  //read out sensor cup 5
  digitalWrite (S0, LOW);
  digitalWrite (S1, LOW);
  digitalWrite (S2, HIGH);
  sCup[4] = analogRead (SensorPin);

  //read out sensor cup 6
  digitalWrite (S0, HIGH);
  digitalWrite (S1, LOW);
  digitalWrite (S2, HIGH);
  sCup[5] = analogRead (SensorPin);

 
 
  //LEDs Cup1
  
  if (sCup[0] < 500)
  { 
    LEDpattern[0] = (LEDpattern[0] | maskCup[0]);
  }

  else 
  {
    bitClear(LEDpattern[0] , 0);
    bitClear(LEDpattern[0] , 1);
  }


  //LEDs Cup2

  if (sCup[1] < 500)
  { 
    LEDpattern[0] = (LEDpattern[0] | maskCup[1]);        
  }

  else 
  {
    bitClear(LEDpattern[0] , 2);
    bitClear(LEDpattern[0] , 3);
  }


  //LEDs Cup3

    if (sCup[2] < 500)
  { 
    LEDpattern[0] = (LEDpattern[0] | maskCup[2]);          
  }

  else 
  {
    bitClear(LEDpattern[0] , 4);
    bitClear(LEDpattern[0] , 5);
  }

  //LEDs Cup4

  if (sCup[3] < 500)
  { 
    LEDpattern[0] = (LEDpattern[0] | maskCup[3]);  
  }    

  else 
  {
    bitClear(LEDpattern[0] , 6);
    bitClear(LEDpattern[0] , 7);
  }


  //LEDs Cup5

  if (sCup[4] < 500)
  { 
    LEDpattern[1] = (LEDpattern[1] | maskCup[4]);          
  }

  else 
  {
    bitClear(LEDpattern[1] , 0);
    bitClear(LEDpattern[1] , 1);
  }


  //LEDs Cup6

  if (sCup[5] < 500)
  { 
    LEDpattern[1] = (LEDpattern[1] | maskCup[5]);        
  }

  else 
  {
    bitClear(LEDpattern[1] , 2);
    bitClear(LEDpattern[1] , 3);
  }       


  //shifting out the data to the shift registers

  digitalWrite(latchPin, LOW);            
  shiftOut(dataPin, clockPin, MSBFIRST, LEDpattern[1]);         
  shiftOut(dataPin, clockPin, MSBFIRST, LEDpattern[0]);
  digitalWrite(latchPin, HIGH); 


}

Is this what you meant?
Are there more things about the code that work but are not really nice or could be better?

10

Well, you now have some data in arrays but are not really making use of them. Arrays are particularly useful when you can put code in a for loop and use the variable of the for loop as an index to the data in the array. It is a bit complicated in your case because the actions you take are different depending on the value of the array index.

I would consider trying a for loop to go through the cup numbers so that the sCup[x] < 500 comparison is in the for loop and the actions to be taken are dealt with using switch/case based on the array index.

To avoid more duplication of code how about creating a function that does the actions and passing a parameter to it for the LEDpattern to be used and the array index. The index needed for maskCup[] is already the same as the array index.

Using arrays and a function will considerably reduce the amount of code you have to enter and will make the program easier to extend.

11

I tried to use a for loop and switch/case functions and it kind of works. If I place a cup in a hole the leds are switched on but if there is no cup in the hole the leds blink randomly. There seems to be a logical mistake in my thinking or in my understanding of the switch/case.
Is it possible to put a new switch/case into a case of another switch/case?

//defining pins

int S0 = 8;
int S1 = 9;
int S2 = 10;

int SensorPin = A0;

int dataPin = 2;        
int latchPin = 3;
int clockPin = 4;


//defining variables

int sCup[6] ;

byte LEDpattern[] = {
  B00000000, B00000000};

byte maskCup[] = {
  B00000011, B00001100, B00110000, B11000000, B00000011, B00001100};



//Circle sequence

int seq1[6] = {
  3,12,48,0,0 ,192};
int seq2[6] = {
  0,0 ,0 ,3,12,0};

void setup()
{
  //Configure each IO Pin
  pinMode(S0, OUTPUT);      
  pinMode(S1, OUTPUT);
  pinMode(S2, OUTPUT);
  pinMode(SensorPin, INPUT);

  pinMode(dataPin, OUTPUT);       
  pinMode(latchPin, OUTPUT);
  pinMode(clockPin, OUTPUT);

    
}

void loop() {





  //read out sensor cup 1
  digitalWrite (S0, LOW);
  digitalWrite (S1, LOW);
  digitalWrite (S2, LOW);
  sCup[0] = analogRead (SensorPin);

  //read out sensor cup 2
  digitalWrite (S0, HIGH);
  digitalWrite (S1, LOW);
  digitalWrite (S2, LOW);
  sCup[1] = analogRead (SensorPin);

  //read out sensor cup 3
  digitalWrite (S0, LOW);
  digitalWrite (S1, HIGH);
  digitalWrite (S2, LOW);
  sCup[2] = analogRead (SensorPin);

  //read out sensor cup 4
  digitalWrite (S0, HIGH);
  digitalWrite (S1, HIGH);
  digitalWrite (S2, LOW);
  sCup[3] = analogRead (SensorPin);

  //read out sensor cup 5
  digitalWrite (S0, LOW);
  digitalWrite (S1, LOW);
  digitalWrite (S2, HIGH);
  sCup[4] = analogRead (SensorPin);

  //read out sensor cup 6
  digitalWrite (S0, HIGH);
  digitalWrite (S1, LOW);
  digitalWrite (S2, HIGH);
  sCup[5] = analogRead (SensorPin);


  for (int i = 0 ; i < 4 ; i++)
  {

    int range = map(sCup[i],0,1023,0,1);


    switch (range) 
    {

    case  0:
      LEDpattern[0] = (LEDpattern[0] | maskCup[i]);
      break;

    case  1:

      switch (i) 
      {
      case 0:
        bitClear(LEDpattern[0] , 0);
        bitClear(LEDpattern[0] , 1);
        break;

      case 1:
        bitClear(LEDpattern[0] , 2);
        bitClear(LEDpattern[0] , 3);
        break;

      case 2:
        bitClear(LEDpattern[0] , 4);
        bitClear(LEDpattern[0] , 5);
        break;

      case 3:
        bitClear(LEDpattern[0] , 6);
        bitClear(LEDpattern[0] , 7);
        break;
      } 

    }
  }

  for (int i = 4 ; i < 6 ; i++)
  {
    int range = map(sCup[i],0,1023,0,1);

    switch (range) 
    {

    case 0:
      LEDpattern[1] = (LEDpattern[1] | maskCup[i]);
      break;

    case 1:

      switch (i) 
      {
      case 4:
        bitClear(LEDpattern[1] , 0);
        bitClear(LEDpattern[1] , 1);
        break;

      case 5:
        bitClear(LEDpattern[1] , 2);
        bitClear(LEDpattern[1] , 3);
        break;
      }
    }
  }



  //shifting out the data to the shift registers

  digitalWrite(latchPin, LOW);            
  shiftOut(dataPin, clockPin, MSBFIRST, LEDpattern[1]);         
  shiftOut(dataPin, clockPin, MSBFIRST, LEDpattern[0]);
  digitalWrite(latchPin, HIGH); 


}
12

I don't have time to look at your code at the moment, but it is certainly possible to nest switch/case instances.

13

Alright I thought I found the mistake because I had forgotten to shift out the data while still being in the for loop.
I didn't change too much :smiley: I'll keep trying...

Here's the code just in case somebody is extremely bored and wants to read it :wink:

//defining pins

int S0 = 8;
int S1 = 9;
int S2 = 10;

int SensorPin = A0;

int dataPin = 2;        
int latchPin = 3;
int clockPin = 4;


//defining variables

int sCup[6] ;

byte LEDpattern[] = {
  B00000000, B00000000};

byte maskCup[] = {
  B00000011, B00001100, B00110000, B11000000, B00000011, B00001100};



//Circle sequence

int seq1[6] = {
  3,12,48,0,0 ,192};
int seq2[6] = {
  0,0 ,0 ,3,12,0};

void setup()
{
  //Configure each IO Pin
  pinMode(S0, OUTPUT);      
  pinMode(S1, OUTPUT);
  pinMode(S2, OUTPUT);
  pinMode(SensorPin, INPUT);

  pinMode(dataPin, OUTPUT);       
  pinMode(latchPin, OUTPUT);
  pinMode(clockPin, OUTPUT);

  //initialize serial communication
  Serial.begin(9600);       
}

void loop() {





  //read out sensor cup 1
  digitalWrite (S0, LOW);
  digitalWrite (S1, LOW);
  digitalWrite (S2, LOW);
  sCup[0] = analogRead (SensorPin);

  //read out sensor cup 2
  digitalWrite (S0, HIGH);
  digitalWrite (S1, LOW);
  digitalWrite (S2, LOW);
  sCup[1] = analogRead (SensorPin);

  //read out sensor cup 3
  digitalWrite (S0, LOW);
  digitalWrite (S1, HIGH);
  digitalWrite (S2, LOW);
  sCup[2] = analogRead (SensorPin);

  //read out sensor cup 4
  digitalWrite (S0, HIGH);
  digitalWrite (S1, HIGH);
  digitalWrite (S2, LOW);
  sCup[3] = analogRead (SensorPin);

  //read out sensor cup 5
  digitalWrite (S0, LOW);
  digitalWrite (S1, LOW);
  digitalWrite (S2, HIGH);
  sCup[4] = analogRead (SensorPin);

  //read out sensor cup 6
  digitalWrite (S0, HIGH);
  digitalWrite (S1, LOW);
  digitalWrite (S2, HIGH);
  sCup[5] = analogRead (SensorPin);


  for (int i = 0 ; i < 4 ; i++)
  {

    int range = map(sCup[i],0,1023,0,1);


    switch (range) 
    {

    case  0:
      LEDpattern[0] = (LEDpattern[0] | maskCup[i]);
      break;

    case  1:

      switch (i) 
      {
      case 0:
        bitClear(LEDpattern[0] , 0);
        bitClear(LEDpattern[0] , 1);
        break;

      case 1:
        bitClear(LEDpattern[0] , 2);
        bitClear(LEDpattern[0] , 3);
        break;

      case 2:
        bitClear(LEDpattern[0] , 4);
        bitClear(LEDpattern[0] , 5);
        break;

      case 3:
        bitClear(LEDpattern[0] , 6);
        bitClear(LEDpattern[0] , 7);
        break;
      } 

    }

    digitalWrite(latchPin, LOW);            
    shiftOut(dataPin, clockPin, MSBFIRST, LEDpattern[1]);         
    shiftOut(dataPin, clockPin, MSBFIRST, LEDpattern[0]);
    digitalWrite(latchPin, HIGH); 
  }


  for (int i = 4 ; i < 6 ; i++)
  {
    int range = map(sCup[i],0,1023,0,1);

    switch (range) 
    {

    case 0:
      LEDpattern[1] = (LEDpattern[1] | maskCup[i]);
      break;

    case 1:

      switch (i) 
      {
      case 4:
        bitClear(LEDpattern[1] , 0);
        bitClear(LEDpattern[1] , 1);
        break;

      case 5:
        bitClear(LEDpattern[1] , 2);
        bitClear(LEDpattern[1] , 3);
        break;
      }
    }
    digitalWrite(latchPin, LOW);            
    shiftOut(dataPin, clockPin, MSBFIRST, LEDpattern[1]);         
    shiftOut(dataPin, clockPin, MSBFIRST, LEDpattern[0]);
    digitalWrite(latchPin, HIGH); 
  }


}
14

What sort of values do you get from reading the sensors

  sCup[5] = analogRead (SensorPin);

I wonder if using map() to convert the values to 0 or 1 is working. I think that I would write a function to set range explicitly to 1 or 0 depending whether the value was above or below a threshold. It feels more controllable than letting map() do its stuff because you are depending on the value being above or below 512 at the moment and I wonder if that is the case.

You can do the comparison and setting of range without a function using a ternary operator like this

value = (sCup[index] < threshold) ? LOW : HIGH;

If I have got that right (I will soon be told if I haven't) then value will be set to LOW if sCup[index] is less than threshold else it will be set to HIGH.

This may or may not be the cause of your problem but using map() like you are feels wrong somehow

15

That indeed was the problem. I don't really understand why but now it is working again.
Thank you very much.

Next thing I will try is to somehow shorten the code for the

  //read out sensor cup 1
  digitalWrite (S0, LOW);
  digitalWrite (S1, LOW);
  digitalWrite (S2, LOW);
  sCup[0] = analogRead (SensorPin);
.
.
.
.
.

When you said:

I would consider trying a for loop to go through the cup numbers so that the sCup
< 500 comparison is in the for loop and the actions to be taken are dealt with using switch/case based on the array index.

To avoid more duplication of code how about creating a function that does the actions and passing a parameter to it for the LEDpattern to be used and the array index. The index needed for maskCup[] is already the same as the array index.

Was this what you thought of or does the code still seem too long?

 for (int i = 0 ; i < 4 ; i++)
  {

    int value = (sCup[i] < 500) ? LOW : HIGH;  
    
    
    switch (value) 
    {

    case  0:
      LEDpattern[0] = (LEDpattern[0] | maskCup[i]);
      break;

    case  1:

      switch (i) 
      {
      case 0:
        bitClear(LEDpattern[0] , 0);
        bitClear(LEDpattern[0] , 1);
        break;

      case 1:
        bitClear(LEDpattern[0] , 2);
        bitClear(LEDpattern[0] , 3);
        break;

      case 2:
        bitClear(LEDpattern[0] , 4);
        bitClear(LEDpattern[0] , 5);
        break;

      case 3:
        bitClear(LEDpattern[0] , 6);
        bitClear(LEDpattern[0] , 7);
        break;
      } 

    }

    digitalWrite(latchPin, LOW);            
    shiftOut(dataPin, clockPin, MSBFIRST, LEDpattern[1]);         
    shiftOut(dataPin, clockPin, MSBFIRST, LEDpattern[0]);
    digitalWrite(latchPin, HIGH); 
  }
16

Can you please post your while code as it is now as a fresh place to start from ?

You can improve these sections

  //read out sensor cup 1
  digitalWrite (S0, LOW);
  digitalWrite (S1, LOW);
  digitalWrite (S2, LOW);
  sCup[0] = analogRead (SensorPin);

by putting them in a for loop too and holding the HIGH/LOW values to be used in an array but could I ask that you change the comments about cup numbers

//read out sensor cup 1

so that the numbers match the corresponding index to sCup[] ? It will make no difference to operation of the program but may mean that the operation of the for loop and array are more obvious.

17

Alright I changed the cup numbers, they started to confuse me as well.

You can improve these sections
Code:
//read out sensor cup 1
digitalWrite (S0, LOW);
digitalWrite (S1, LOW);
digitalWrite (S2, LOW);
sCup[0] = analogRead (SensorPin);
by putting them in a for loop too and holding the HIGH/LOW values to be used in an array

/done :slight_smile:

And I was able to fit the "LEDpattern adjustment and shiftout process" for the two different ShiftRegisters into one for loop using a threshold again.

So this is what the program looks like right now:

//defining pins

int S0 = 8;
int S1 = 9;
int S2 = 10;

int SensorPin = A0;

int dataPin = 2;        
int latchPin = 3;
int clockPin = 4;


//defining variables

int sCup[6] ;

byte LEDpattern[] = {B00000000, B00000000};

byte maskCup[] = {B00000011, B00001100, B00110000, B11000000, B00000011, B00001100};

int SOvalue[] = {0,1,0,1,0,1};
int S1value[] = {0,0,1,1,0,0};
int S2value[] = {0,0,0,0,1,1};

void setup()
{

  //Configure each IO Pin
  pinMode(S0, OUTPUT);      
  pinMode(S1, OUTPUT);
  pinMode(S2, OUTPUT);
  pinMode(SensorPin, INPUT);

  pinMode(dataPin, OUTPUT);       
  pinMode(latchPin, OUTPUT);
  pinMode(clockPin, OUTPUT);

}

void loop() {


  //read out sensor values and store them in sCup[]

  for (int i=0 ; i<6 ; i++)
  {
    digitalWrite (S0, SOvalue[i]);
    digitalWrite (S1, S1value[i]);
    digitalWrite (S2, S2value[i]);
    sCup[i] = analogRead (SensorPin);
  }


  //adjust LED pattern to SensorValues and shift out data
  for (int i = 0 ; i < 6 ; i++)
  {

    int value = (sCup[i] < 500) ? LOW : HIGH;  
    int patternValue = (i < 4) ? LOW : HIGH;

    switch (value) 
    {

    case  0:
      LEDpattern[patternValue] = (LEDpattern[patternValue] | maskCup[i]);
      break;

    case  1:

      switch (i) 
      {
      case 0:
        bitClear(LEDpattern[0] , 0);
        bitClear(LEDpattern[0] , 1);
        break;

      case 1:
        bitClear(LEDpattern[0] , 2);
        bitClear(LEDpattern[0] , 3);
        break;

      case 2:
        bitClear(LEDpattern[0] , 4);
        bitClear(LEDpattern[0] , 5);
        break;

      case 3:
        bitClear(LEDpattern[0] , 6);
        bitClear(LEDpattern[0] , 7);
        break;

      case 4:
        bitClear(LEDpattern[1] , 0);
        bitClear(LEDpattern[1] , 1);
        break;

      case 5:
        bitClear(LEDpattern[1] , 2);
        bitClear(LEDpattern[1] , 3);
        break;
      } 

    }

    digitalWrite(latchPin, LOW);            
    shiftOut(dataPin, clockPin, MSBFIRST, LEDpattern[1]);         
    shiftOut(dataPin, clockPin, MSBFIRST, LEDpattern[0]);
    digitalWrite(latchPin, HIGH); 
  }

}

what would you say is the next thing I should improve? I think it already looks amazing compared to the code I posted in the very beginning. Again thank you very much for your help. I'm learning quiet a bit step by step with your help.

18

As a first step, how about replacing these

      case 0:
        bitClear(LEDpattern[0] , 0);
        bitClear(LEDpattern[0] , 1);
        break;

with

case 0:
  clearTwoBits(0, 0, 1);
  break;

and a new function

  void clearTwoBits(int patternNumber, int firstBit, int secondBit)
  {
    bitClear(LEDpattern[patternNumber] , firstBit);
    bitClear(LEDpattern[0] , secondBit);  
  }

Not a huge change but another step towards making the program easier to extend.

19

These and the following ones

  //read out sensor cup 0
  digitalWrite (S0, LOW);
  digitalWrite (S1, LOW);
  digitalWrite (S2, LOW);
  sCup[0] = analogRead (SensorPin);

  //read out sensor cup 1
  digitalWrite (S0, HIGH);
  digitalWrite (S1, LOW);
  digitalWrite (S2, LOW);
  sCup[1] = analogRead (SensorPin);

look like another candidate for a for loop based on the cup number. Each of them writes a pattern to the same LEDs and reads the corresponding sensor value. If you put the HIGH/LOW values to be written in an array with the cup number as its row index and the 3 HIGH/LOW values as columns then you could get the values to be written to S0, S1 and S2 from the array row for the cup number and the sCup[] index number will be the for loop variable.

One reason for suggesting changing the cup numbers in the comments was that, as you know, the array index starts at zero not one. With the comments changed there is less chance of confusion I think.

20

These and the following ones
Code:
//read out sensor cup 0
digitalWrite (S0, LOW);
digitalWrite (S1, LOW);
digitalWrite (S2, LOW);
sCup[0] = analogRead (SensorPin);

//read out sensor cup 1
digitalWrite (S0, HIGH);
digitalWrite (S1, LOW);
digitalWrite (S2, LOW);
sCup[1] = analogRead (SensorPin);
look like another candidate for a for loop based on the cup number. Each of them writes a pattern to the same LEDs and reads the corresponding sensor value. If you put the HIGH/LOW values to be written in an array with the cup number as its row index and the 3 HIGH/LOW values as columns then you could get the values to be written to S0, S1 and S2 from the array row for the cup number and the sCup[] index number will be the for loop variable.

One reason for suggesting changing the cup numbers in the comments was that, as you know, the array index starts at zero not one. With the comments changed there is less chance of confusion I think.

(I think), I did that. I modified my post right after posting it, maybe you still saw the old version?