next topic »

[on:]

I am doing this just an as exercise on learning for my own use.

Any help or wisdom is welcomed to make it better or easier to read or understand.

Code:

/*
TCS230 COLOR sensor from TAOS.
Written By Michael King
Copiertalk.com
Creative commons share alike.
 
 */

// include the library code:
#include <LiquidCrystal.h>

// initialize the library with the numbers of the interface pins
LiquidCrystal lcd(53, 49, 41, 43, 45, 47);

int whatcolor = 0; // what color are we testing
unsigned long   currentmillis =  millis();
unsigned long   oldmillis = millis();
;

int s0 = 30; // Pins for our color sensor filters
int s1 = 31;
int s2 = 32;
int s3 = 33;
int oe = 35;
int ledpin = 13;

unsigned long red = 0;
unsigned long blue = 0;
unsigned long green = 0;
unsigned long nocolor = 0;
unsigned long sensorpulse = 0;


// ****************************************************
// Setup function
// This is executed once at the start of our program
// It sets up our pins for our sensor.
// ****************************************************
void setup()
{

  sensorpulse = 0; // I want to make sure this value starts at zero
  pinMode (s0, OUTPUT); // set up our sensor
  pinMode (s1, OUTPUT); // pins to control
  pinMode (s2, OUTPUT); // Color Filter.
  pinMode (oe,OUTPUT);
  pinMode (s3, OUTPUT);
  digitalWrite(oe,LOW);
  digitalWrite(s0,LOW); // Set scaling here.
  digitalWrite(s1,HIGH);
  digitalWrite(ledpin,HIGH);

  attachInterrupt(0, sensorisr, RISING);
  // set up the LCD's number of columns and rows:
  lcd.begin(16, 2);
  scrnsetup();

}

// ****************************************************
//         This is our interupt service routine.
// ****************************************************

void sensorisr ()
{
  sensorpulse ++; // Add 1 to our pulse count.
}

// ****************************************************
//          This function returns a value
// Depending on the color filter defined below.
// 1 = red filter
// 2 = blue filter
// 3 = green filter
// 4 = no color filter
// ****************************************************

unsigned long  getcolor (int color,unsigned long colormillis)
{
  unsigned long currentpulse;
  digitalWrite(ledpin,LOW); // turn leds on
  if (color == 1)
  {
    digitalWrite(oe,LOW); // turn sensor on
    digitalWrite ( s2, LOW); // red
    digitalWrite ( s3, LOW);
    do 
    {
      currentpulse = sensorpulse;
    }
    while ((millis() - colormillis)  < 250); // check for 1/4 sencond;
  }
  if (color == 2)
  {
    digitalWrite(oe,LOW); // Turn Sensor on
    digitalWrite ( s2, LOW); // BLue filter
    digitalWrite ( s3, HIGH);
    do 
    {
      currentpulse = sensorpulse;

    }
    while ((millis() - colormillis)  < 250); // check for 1/4 sencond;
  }

  if (color == 3)
  {
    digitalWrite(oe,LOW); // turn sensor on
    digitalWrite ( s2, HIGH); // greeen
    digitalWrite ( s3, HIGH);
    do 
    {
      currentpulse = sensorpulse;

    }
    while ((millis() - colormillis)  < 250); // check for 1/4 sencond;
  }

  if (color == 4)
  {
    digitalWrite(oe,LOW); // turn sensor on
    digitalWrite ( s2, HIGH); // clear filter or no color
    digitalWrite ( s3, LOW);
    do 
    {
      currentpulse = sensorpulse;

    }
    while ((millis() - colormillis)  < 250); // check for 1/4 sencond;
  }
  sensorpulse=0; // reset our pulse count prior to leaving
  digitalWrite (oe,HIGH); // Turn sensor off
  // digitalWrite(ledpin,HIGH); // turn leds off if needed
  return currentpulse;
}
// ****************************************************
// set the static portion of our display up
// This does not change so there is no reason to display it over and over
// ****************************************************
// +++++++++++++++ set the lcd screen up ++++++++++++
void scrnsetup()
{
  lcd.setCursor(0,0);
  lcd.print("R:");
  lcd.setCursor(7,0);
  lcd.print("B:");
  lcd.setCursor(0,1);
  lcd.print("G:");
  lcd.setCursor(7,1);
  lcd.print("N:");
}


// ****************************************************
// Display our color values to the lcd display
// ****************************************************
// Position the cursor on the lcd and display the color values
void displaysensor()
{
  lcd.setCursor(3,0);
  lcd.print(red);
  lcd.setCursor ( 10,0);
  lcd.print(blue);
  lcd.setCursor(3,1);
  lcd.print(green);
  lcd.setCursor ( 10,1);
  lcd.print(nocolor);
}

// ****************************************************
//          Get the color values from our sensor
// 1 = red filter
// 2 = blue filter
// 3 = green filter
// 4 = no color filter
// ****************************************************
void getsensor() // Get the sensor values;
{
  red = getcolor (1,millis());
  blue = getcolor (2,millis());
  green = getcolor (3,millis());
  nocolor = getcolor (4,millis());
}


// ****************************************************
//         Our Main loop over and over again
// ****************************************************
void loop()
{
  displaysensor();
  getsensor();

}

I am working towards a device to test colors on a printed page but am just starting the project and want to get the sensor output so far. Let me know what you think.

[Reply #1 on:]

Code:

  pinMode (s0, OUTPUT); // set up our sensor
  pinMode (s1, OUTPUT); // pins to control
  pinMode (s2, OUTPUT); // Color Filter.
  pinMode (oe,OUTPUT);
  pinMode (s3, OUTPUT);

I'd rather see all 4 sN pins set together.

Some blank lines to separate logical bits of code would be helpful, too.

Not using blank lines where not needed would be helpful, too. (The one right after setup()'s open brace is annoying.)

Code:

  digitalWrite(s0,LOW); // Set scaling here.
  digitalWrite(s1,HIGH);

It might be obvious to you how that sets scaling, but it isn't obvious to anyone else.

Code:

// ****************************************************
//         This is our interupt service routine.
// ****************************************************

void sensorisr ()
{
  sensorpulse ++; // Add 1 to our pulse count.
}

If you are going to use comments, and I strongly recommend that you do, stating the obvious is a waste of time.

sensorpulse (I hate names all in lower case) is used by the ISR and other functions, but is not declared volatile.

Code:

unsigned long  getcolor (int color,unsigned long colormillis)

Spacesbetweenargumentsarenice.

Code:

    do 
    {
      currentpulse = sensorpulse;
    }
    while ((millis() - colormillis)  < 250); // check for 1/4 sencond;

During this code, the compiler can see that sensorpulse never changes (since it is not declared volatile). As a result, this loop will never set a different value for currentpulse (camelCase was invented for a reason!), so, you might as well have used delay(250); here.

colormillis is a lousy name. Yes, the variable has something to do with time, but what, exactly? startTime would make more sense.

Code:

  if (color == 1)
  {
  }
  if (color == 2)
  {
  }
  if (color == 3)
  {
  }
  if (color == 4)
  {
  }

Is there any situation where more than one of these blocks will be executed? The else if statement should be deployed, here.

You have very similar actions in each block. A function, instead of cut and paste, would have been better.

Code:

void loop()
{
  displaysensor();
  getsensor();

}

Display some values, then get some values to display. Hmmm, something's wrong with this picture.

[Reply #2 on:]

Thanks paul.

I will clean up documenting a little more and work on the timer first. I will want it to check other sensor and perform other tasks while it is checking the sensor pulse values. As it is a full second elapses before the controller can do any other tasks.

I need to look up what the volitile description is for a variable. I have never used that before so its a new term to me.

Thanks again!

[Reply #3 on:]

I'd rather see all 4 sN pins set together.

I agree

Some blank lines to separate logical bits of code would be helpful, too.

Not using blank lines where not needed would be helpful, too. (The one right after setup()'s open brace is annoying.)

I agree. thats why I asked. see if this is easier to read and understand. I may have over documented it this time?

It might be obvious to you how that sets scaling, but it isn't obvious to anyone else.

thats where I may have overdone the over documentation. In the data sheet and the sensor sheet there are
some inconsistancies I am still trying to get a handle on my self. bare with me a little here for the short term.

If you are going to use comments, and I strongly recommend that you do, stating the obvious is a waste of time.

sensorpulse (I hate names all in lower case) is used by the ISR and other functions, but is not declared volatile.

I looked into volatile and it makes sense. I have implemented it. Thanks.

Code:

unsigned long  getcolor (int color,unsigned long colormillis)

Spacesbetweenargumentsarenice.

easy fix although not in this version.

During this code, the compiler can see that sensorpulse never changes (since it is not declared volatile). As a result, this loop will never set a different value for currentpulse (camelCase was invented for a reason!), so, you might as well have used delay(250); here.

I would like for there to be no delay at all. I am still thinking this one over. I can increase the sensitivity of the sensor and reduce the time but in the end I would like for there to be no delay at all. That would mean handling the millis timing from the calling function rather than controling it from this function. If I increase the sensitivity it may effect millis as the program uses an interupt.

I am still thinking this over. I am probably thinking this over too much.

colormillis is a lousy name. Yes, the variable has something to do with time, but what, exactly? startTime would make more sense.

Better than naming it fred.

Is there any situation where more than one of these blocks will be executed? The else if statement should be deployed, here.

No, But that does not mean someone will not try. I will see if I can come up with a way to limit an operator error or programming error.

You have very similar actions in each block. A function, instead of cut and paste, would have been better.


Display some values, then get some values to display. Hmmm, something's wrong with this picture.
That is just what I was taught. Let main loop handle basic control and let your functions do the heavy lifting.

[Reply #4 on:]

Code:

/*
TCS3200 / 3210  COLOR sensor from TAOS.
 Written By Michael King
 Copiertalk.com
 Creative commons share alike.
 
 The sensor provides a pulse  depending on the value of the color filter
 , frequency scaling desired as well as the output enable pin being low.
 We are going to use an interupt on pin 2 to count the number of pulses in
 a given amount of time. We will use the values returned to try and
 and tell the color the sensor is "seeing" at the moment.
 
 We will use these pin settings in our program to fine tume the sensor to
 our needs.
 
 s0 and s1 set the Frequency scaling
 s0 = low / s1 = low is power down mode for the sensor
 s0 = low / s1 = hihg 2% scale
 s0 = high / s1 = low 20% scale
 s0 = high / s1 = high 100% scale
 
 s2 and s3 set the color filter of the sensor
 s2 = low /  s3 = low red filter
 s2 = low / s3 = high blue filter
 s2 = high / s3 = low Clear or no filter
 s2 = high / s3 = high green filter
 
 Some breakout boards use an LED to illuminate the object and
 we will use pin 13 to control this led on the breakout board that I
 am using.
 
 */

// include the library code:
#include <LiquidCrystal.h>

// initialize the library with the numbers of the interface pins
LiquidCrystal lcd(53, 49, 41, 43, 45, 47);

int whatcolor = 0; // what color are we testing
unsigned long   currentmillis =  millis();
unsigned long   oldmillis = millis();


// ****************************************************
// s0 and s1 set the Frequency scaling
// s0 = low / s1 = low is power down mode for the sensor
// s0 = low / s1 = hihg 2% scale
// s0 = high / s1 = low 20% scale
// s0 = high / s1 = high 100% scale
// s2 and s3 set the color filter of the sensor
// s2 = low /  s3 = low red filter
// s2 = low / s3 = high blue filter
// s2 = high / s3 = low Clear or no filter
// s2 = high / s3 = high green filter
// ****************************************************
int s0 = 30; // Pins for our color sensor filters
int s1 = 31; // and our scaling. You can use any digital pins.
int s2 = 32;
int s3 = 33;
int oe = 35;
int ledpin = 13; // pin to control sensor led on /off
// These variables are to store the pulse count returned by
// our interupt service routine. we assign them with the function call
//
unsigned long red = 0;
unsigned long blue = 0;
unsigned long green = 0;
unsigned long nocolor = 0;
volatile unsigned long sensorpulse = 0; // This value will be changed
// by the service routine and
// then taken and asigned to another
// variable in our program depending on the
// color filter in use   


// ****************************************************
// Setup function
// This is executed once at the start of our program
// It sets up our pins for our sensor, our lcd and defines our
// initial values of pin modes and outut levels.
// ****************************************************
void setup()
{
  sensorpulse = 0; // I want to make sure this value starts at zero

  pinMode (s0, OUTPUT); // set up our sensor 
  pinMode (s1, OUTPUT); // pins to control scaling

  pinMode (s2, OUTPUT); // set up our sensor pins
  pinMode (s3, OUTPUT); // to control color filter

  pinMode (oe,OUTPUT); // output enable pin for our color  sensor
  digitalWrite(oe,LOW); // low is enable / high is disabled

  digitalWrite(s0,LOW); // Set scaling here.
  digitalWrite(s1,HIGH); // s0 high and s1 low is 2% scale

  digitalWrite(ledpin,HIGH); // Pin to control LED on sensor if needed

  attachInterrupt(0, sensorisr, RISING); // Asign our interupt to the function and set to rising

  // set up the LCD's number of columns and rows:
  lcd.begin(16, 2);
  scrnsetup(); // write static portion of our lcd.
}


// ****************************************************
//         This is our interupt service routine.
// ****************************************************
void sensorisr ()
{
  sensorpulse ++; // Add 1 to our pulse count.
}

// ****************************************************
//          This function returns a value
// Depending on the color filter defined below.
// 1 = red filter
// 2 = blue filter
// 3 = green filter
// 4 = no color filter
// ****************************************************
unsigned long  getcolor (int color , unsigned long colormillis)
{
  unsigned long currentpulse;
  digitalWrite(ledpin,LOW); // turn leds on
  if (color == 1)
  {
    digitalWrite(oe,LOW); // turn sensor on
    digitalWrite ( s2, LOW); // red
    digitalWrite ( s3, LOW);
    do 
    {
      currentpulse = sensorpulse;
    }
    while ((millis() - colormillis)  < 250); // check for 1/4 sencond;
  }
  if (color == 2)
  {
    digitalWrite(oe,LOW); // Turn Sensor on
    digitalWrite ( s2, LOW); // BLue filter
    digitalWrite ( s3, HIGH);
    do 
    {
      currentpulse = sensorpulse;
    }
    while ((millis() - colormillis)  < 250); // check for 1/4 sencond;
  }

  if (color == 3)
  {
    digitalWrite(oe,LOW); // turn sensor on
    digitalWrite ( s2, HIGH); // greeen
    digitalWrite ( s3, HIGH);
    do 
    {
      currentpulse = sensorpulse;
    }
    while ((millis() - colormillis)  < 250); // check for 1/4 sencond;
  }

  if (color == 4)
  {
    digitalWrite(oe,LOW); // turn sensor on
    digitalWrite ( s2, HIGH); // clear filter or no color
    digitalWrite ( s3, LOW);
    do 
    {
      currentpulse = sensorpulse;
    }
    while ((millis() - colormillis)  < 250); // check for 1/4 sencond;
  }
  sensorpulse=0; // reset our pulse count prior to leaving
  digitalWrite (oe,HIGH); // Turn sensor off
  // digitalWrite(ledpin,HIGH); // turn leds off if needed
  return currentpulse;
}


// ****************************************************
// set the static portion of our display up
// This does not change so there is no reason to display it over and over
// ****************************************************
void scrnsetup()
{
  lcd.setCursor(0,0);
  lcd.print("R:");
  lcd.setCursor(7,0);
  lcd.print("B:");
  lcd.setCursor(0,1);
  lcd.print("G:");
  lcd.setCursor(7,1);
  lcd.print("N:");
}


// ****************************************************
// Display our color values to the lcd display
// ****************************************************
void displaysensor()
{
  lcd.setCursor(3,0);
  lcd.print(red);
  lcd.setCursor ( 10,0);
  lcd.print(blue);
  lcd.setCursor(3,1);
  lcd.print(green);
  lcd.setCursor ( 10,1);
  lcd.print(nocolor);
}


// ****************************************************
//          Get the color values from our sensor
// 1 = red filter
// 2 = blue filter
// 3 = green filter
// 4 = no color filter
// ****************************************************
void getsensor() // Get the sensor values;
{
  red = getcolor (1,millis());
  blue = getcolor (2,millis());
  green = getcolor (3,millis());
  nocolor = getcolor (4,millis());
}


// ****************************************************
//         Our Main loop over and over again
// ****************************************************
void loop()
{
  displaysensor();
  getsensor();

}

Currently what I have.

[Reply #5 on:]

Are you thinking of moving the interface pins whilst the program is running?
No, I thought not, so why not make them constants, and while you're at it, make them "byte"s too?

Your get colour function has a lot of replicated code that could easily be factored up. Maybe even a switch/case instead of the "if"s.
Why pass the function the current value of millis?- it could just as well fetch it itself.

Some means of tying the constants 1,2,3,4 to the red, blue, green, clear actions would be a nice touch - maybe an enum? You could probably directly use the binary value to select the appropriate "s" pins, and eliminate the switch/case I mentioned earlier.

[Reply #6 on:]

That is just what I was taught. Let main loop handle basic control and let your functions do the heavy lifting.

Great. But, it's important to call the functions in the proper order.

Code:

void loop()
{
  displaysensor();
  getsensor();
}

It just seems to me like it would make more sense to get some data then display that data. Could just be me, though.