Hi: I have some code that will ultimately control a halloween prop. At the moment I am using 9 leds to check the code but these outputs will eventually control relays for hydraulic cylinders, smoke machines, sound and a water squirter.
The code involves a 70 second sequence where various outputs are triggered for a set period of time.
There are 26 "events" within this period and each is name ID1-26, along with constants for start time (after PIR activation) and duration..along with associated PIN numbers.
The code works perfectly with regards to LEDs lighting at the right time and for the right period of time but the problem I have is that some of the LEDs come on brightly some of the time and full brightness at other times. It only seems to effect certain "event ID's" and it's always the same ones although the same LED will come on full brightness at a later "ID" ..hope that make sense.
All the LEDs seem ok and a simple "light all" test showed them all at full brightness. This happens when the board (UNO) is powered by either the USB of a 5v DC adaptor.
It seems to happen on pins 4 and 5 I think...although they light fully at other times.
The first instance of this happening is for ID2 which should start at the start of the sequence for 1000 milliseconds on pin 4.
I have included Serial.print comments for each ID and can see that they are all triggering at the correct time and there is no (as far as I can see) rapid on off loops going on that may may it look dim.
Not sure if its relevant but it does seem to look like the problematic LED's sometime come on 1/3 brightness and sometimes 2/3 brightness...and then at other time full brightness.
The code is below...this is my first proper attempt at coding so I am guessing this could be streamlined significantly and I really should get my head around arrays!!!
Hope someone can shed some light as I am concerned that a dim LED is perhaps not getting the current required to trigger the relays at the new part of the project...and anyway its bugging me why it is how it is 
thanks for your time
PS...had to attach text file as it exceeded posting limits
Halloween.ino (19.1 KB)
How many current do you try to stuff into the leds? If the answer is +-20mA you might get in trouble. Although a pin can provide 20mA the Arduino also has a total max current. Try reducing the led current (aka higher resistor). With modern leds 5mA already should give you a very bright indicator.
thanks..I'll try that. Didn't think that was a problem tho as I tested the setup when I built it and all LED (8 of them) all were bright at the same time...and at the point in the sequence where they are dim, there are only a couple lit..but I'll try thank you!
Why did you mangle the nice .ino extension when posting the file?
Uhm, to come back to that, what the heck happened to the whole file?!?!
Classic mistakes for dim outputs:
- pinMode() not set to OUTPUT
- In a single loop you try to digitalWrite(pin, HIGH) but also digitalWrite(pin, LOW)
thanks...have fixed the attachement....was working on plain text editor!!
Cant believe the pinmode is not set...was on an earlier version so must have been chopped out by mistake...can't beliieve it works at all
could you point out which loop?
back to the drawing board...at least there's still over a month to Halloween
Your program would benefit from the use of arrays. It would cut down the amount of code dramatically by using the value of the current state variable as an index to the arrays of values.
I assume that the program will not be required to run for 49 days continuously but one day you might write one that does. The reason for mentioning this is that the IDStart + IDDuration will not work when millis() resets to zero.
See Using millis() for timing. A beginners guide
yes I thought that might be the case...right now the learning curve is very steep and was just trying to get something that I know works, before buying the hard ware for this prop.
yep needs to run for about 3 hours I guess...just to scare the bejesus out of the trick or treaters!!
once this is working I can delve into the whole array thing!...kind of like this at the moment tho as when I need to tweak timings and such once the physical build is done, it's easy to know what needs changing.
...but I agree it is very bloated >:(
just checked the code and the pinmodes are correctly set...knew it was there somewhere...is it ok where it is...just before void setup()
?
pinMode() in setup is correct.
And indeed, code would be 1/4th if you switched to arrays....
Also, don't try to get into the habbit of writing variable names as short as possible. It makes the code wayy harder to read and harder to remember what the name was. No only for us but also for future you. Long names take just a little longer to write (although auto complete of editor like Notepad++ reduce that) but make stuff a lot clearer.
But to the problem, which ID causes the problem?
yes will investigate arrays much further..
point noted re names.
to the problem...
when the PIR is triggered the variable showstart is stored
at this point ID's 1,2 and 3 are triggered a second later 4 is triggered. 2 and 4 are the issue...the LED is dim...these are on the same pin..i think the problem may be todo with this but the serial.print outputs show them on and off separately ok...
so what is the serial output if things go wrong?
I though it might be in a rapid on off loop that would explain the dimming effect. I had this early on and could see it in the serial output
Your problem:
const int ID4Pin = 4;
const int ID5Pin = 4;
ID2,ID4 and ID5 are all doing something on pin 4...but at different times...so I thought. So does it look like ID2 is setting it HIGH because its the right time but ID4 is trying to then set it LOW because it isn't time for ID4 yet?
I'll examine this closer later..its odd because pin 4 does go to full brightness later on in the sequence 
perhaps if I nest the IF's for each pin?
Correctly!
You could try and fix it all in place but I think everything will be a lot easier to do and maintain if you start version 2.0 now You learned a lot during this project so time to implement it
thank you..think a v2.0 is called for.
Would it be better to focus on one function per pin rather than once function per even do you think?
Your requirement seems to be to turn on outputs for given periods of time. An array of pin numbers and a corresponding array of durations would give you a way of defining which pin(s) should turn on and how long for. You could then iterate through the array to determine whether it is time to turn an output on or off based on the start time saved in an array and the elapsed time since the output was turned on.
Mm, hard to say. Both have pro's and con's.
I think I would do it per event but keep the state per pin. But I think I would make it in a way like this (pseudo code):
void loop(){
outputState = LOW;
//only set states if an event is happening
if(Event1IsHappening){
outputState = HIGH;
}
if(Event1IsHappening){
outputState = HIGH;
}
digitalWrite(pin, outputState);
}
This way only one state per cycle can be written to a pin. And an event wanting to drive a pin HIGH goes above LOW.
To give you a start:
//We distinguage between outputs and pins
//Outputs are just numbered 0, 1, 2...x and are mapped to a specific pin (which does not need to be in order or continues
// 0 1 2 3 4 5 6
const byte OutputPins[] = { 3, 4, 9, 5, 6, 7, 8};
const byte NrOutputs = sizeof(OutputPins);
bool outputStates[NrOutputs];
//I use seconds as time because that seems easier to use
struct event_t{
unsigned int startTime;
unsigned int duration;
byte output;
};
const event_t Events[] = {
{0, 60, 0}, //[start time], [duration], [output nr]
{0, 1, 1}, //example: start time 0, duration 1sec, output 1 = pin 4
{0, 60, 2},
{1, 1, 1},
{4, 1, 1};
const byte NrEvents = sizeof(Events)/sizeof(event_t);
void setup(){
for(byte i = 0; i < NrOutputs; i++){
pinMode(OutputPins[i], OUTPUT);
}
}
void loop(){
//set all states lower
for(byte i = 0; i < NrOutputs; i++){
outputStates[i] = LOW;
}
//check all events
for(byte i = 0; i < NrEvents; i++){
//check if an event is happening
//note that this way there is no start time and my checking is NOT roll over proof. Easy to change.
const unsigned long SecondsNow = millis() / 1000;
if(SecondsNow > Events[i].startTime && SecondsNow < (Events[i].startTime + Events[i].duration)){
outputStates[Events[i].output] = HIGH;
}
}
//set outputs
for(byte i = 0; i < NrOutputs; i++){
digitalWrite(OutputPins[i], outputStates[i]);
}
}
What I did not implement was a trigger to start it (/save start time), it start from the beginning. And the time checking goes wrong on a roll over. But that should be easy to fix
I also just created 5 out of the 26 events. And I also notice something weird in that. You trigger pin 4 at second 0 for 1 second in event 2. But in event 4 you trigger pin 4 as well at second 1 for again 1 second. Those two events follow each other without gap
thank you so much for the suggestions and pointers...time to bite the bullet and get to grips with arrays I think...
yes I did try altering the adjacent events to leave a gap (made them 75millis just in case that was causing a problem but to no avail. it does need to be 2 events within 2 secs not 1 event for 2 seconds as I want a pneumatic cylinder to make 2 knocking noises on the inside of a wooden crate
thanks again for your help guys!