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

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.

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.

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.

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 aresome 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.

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.

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// ****************************************************int s0 = 30; // Pins for our color sensor filtersint 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

// ****************************************************// 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:"); }

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.

"Pete, it's a fool looks for logic in the chambers of the human heart." Ulysses Everett McGill.Do not send technical questions via personal messaging - they will be ignored.