Hello. I am building a replica of Captain Kirk's command chair from the original Star Trek series for a friend. I am using two Arduino Unos to manage the light and sound effects. In the right hand console, I have successfully cobbled together the necessary code for the Arduino with a Wave Shield to play the appropriate wavs and light or flash the appropriate LEDs when a given button is pushed. The problem I'm having is on the left hand console, where the other Arduino is doing the, ostensibly simpler, task of blinking various LEDs when rocker switches are activated. As Bones McCoy would say, I'm a doctor (pediatrician), not a programmer. I've been going at this by pulling code from various example sketches and trying to stitch it all together.
My intent is to have each rocker switch activate 2 or 3 LEDs which will then blink at fixed intervals so that the overall effect is quasi random. I have it about 95% functional, but the problem I'm having is that, if an LED happens to be on when the switch is turned "off," then that LED remains lit. I've tried adding an "else" statement specifically setting the LED pin to LOW when the switch is "off," but that doesn't solve my problem. I know the intent of this forum is just to guide and not to provide finished code, so if someone looking at what I have could suggest a direction for me, I would really appreciate it:
/*
*/
int ledState = LOW; // ledState used to set the LED
long previousMillis = 0; // will store last time LED was updated
// variables for the LED blinking intervals
long interval_L6 = 1000; // interval at which to blink LED L6 (milliseconds)
long interval_L7 = 500; // interval at which to blink LED L7 (milliseconds)
long interval_L8 = 80; // interval at which to blink LED L8 (milliseconds)
long interval_L9 = 80; // interval at which to blink LED L9 (milliseconds)
long interval_L10 = 120; // interval at which to blink LED L10 (milliseconds)
long interval_L11 = 600; // interval at which to blink LED L11 (milliseconds)
long interval_L12 = 900; // interval at which to blink LED L12 (milliseconds)
long interval_L13 = 350; // interval at which to blink LED L13 (milliseconds)
long interval_L14 = 1200; // interval at which to blink LED L14 (milliseconds)
// set pin numbers (I will eventually have 8 input pins, one for each rocker switch, and 10 output pins, one for each LED. Right now, I'm just testing with 1 in/1 out)
const int buttonPin = 2; // the number of the pushbutton pin
const int ledPin = 9; // the number of the LED pin
// variable for reading the pushbutton status
int buttonState = 0;
void setup() {
// initialize the LED pin as an output:
pinMode(ledPin, OUTPUT);
// initialize the pushbutton pin as an input:
pinMode(buttonPin, INPUT);
digitalWrite(buttonPin, HIGH);
}
void loop(){
// read the state of the pushbutton value:
buttonState = digitalRead(buttonPin);
// Check if the rocker switch is "on." If it is, the buttonState is LOW (I will have the internal pull-ups active, so the switch will just connect the input pin to ground)
if (buttonState == LOW) {
// blink LED at one of the intervals defined above
unsigned long currentMillis = millis();
if(currentMillis - previousMillis > interval_L6) {
// save the last time you blinked the LED
previousMillis = currentMillis;
// if the LED is off turn it on, and vice-versa:
if (ledState == HIGH)
ledState = LOW;
else
ledState = HIGH;
// set the LED L6 with the ledState of the variable:
digitalWrite(ledPin, ledState);
}
else {
// turn LED off:
digitalWrite(ledPin, LOW);
}
}
// variables for the LED blinking intervals
long interval_L6 = 1000; // interval at which to blink LED L6 (milliseconds)
long interval_L7 = 500; // interval at which to blink LED L7 (milliseconds)
long interval_L8 = 80; // interval at which to blink LED L8 (milliseconds)
long interval_L9 = 80; // interval at which to blink LED L9 (milliseconds)
long interval_L10 = 120; // interval at which to blink LED L10 (milliseconds)
long interval_L11 = 600; // interval at which to blink LED L11 (milliseconds)
long interval_L12 = 900; // interval at which to blink LED L12 (milliseconds)
long interval_L13 = 350; // interval at which to blink LED L13 (milliseconds)
long interval_L14 = 1200; // interval at which to blink LED L14 (milliseconds)
Thanks, that is helpful. I'm sure the code could be optimized extensively. I'm brand new to the Arduino - essentially just for this project. Any advice on the problem I'm having with the LED remaining in whatever state it happens to be in (either on or off) when the switch is turned "off?"
The current definitions of HIGH and LOW are not likely to change any time in the near future. So, this code:
// if the LED is off turn it on, and vice-versa:
if (ledState == HIGH)
ledState = LOW;
else
ledState = HIGH;
could be simpler and shorter:
ledState = !ledState;
Simpler and shorter is almost always better.
I generally recommend that new programmers put the { on a new line. It is easier, in my opinion, to see a block of code when the { and } line up. See where a block starts and ends makes it much easier to see where to add an else block.
This is your code (minus the comments) done that way. As you can see, you are setting the pin LOW based on time, not based on the rocker switch state. By the way, it also makes it obvious that you are missing a closing brace.
Thank you for your reply, Paul, and for advice on simplification. I understand that the LED state is being set by time, not by the rocker switch. I was hoping to have the rocker switch just "turn on" and "turn off" the bit of code that causes the interval blinking. Basically, when I turn the switch "off," I would like the LED to turn off, if it happens to be on when the switch is toggled. As it stands now, if I happen to turn the switch off while the LED is also off, then no problem. But if the LED is on when the switch is toggled, it stops blinking and remains steadily illuminated.
I thought maybe I could force the LED off by setting the output pin LOW when the rocker switch is turned "off" (switched back so that it is no longer grounding the input pin) with this command:
else
{
digitalWrite(ledPin, LOW);
}
But that just has the effect of making it so the LED never even turns on. I'm clearly not thinking about this in the right way, as I'm sure it's quite simple.
I thought maybe I could force the LED off by setting the output pin LOW when the rocker switch is turned "off" (switched back so that it is no longer grounding the input pin) with this command:
You can, but the thing is you have two if statements and one else block. It is not clear to me that you have the else associated with the correct if. In fact, it appears to me as though you do not. That's why I suggest lining up the { and }. That, with proper indenting (that can be accomplished easily using the Tools + Auto format menu item) will make it clear whether the else is matched with the correct if.
I was hoping to have the rocker switch just "turn on" and "turn off" the bit of code that causes the interval blinking
Then you could use two state variables: one that continuously goes "on" and "off", which will be "copied" to the led only if the "blinkEnabled" variable (controlled by the rocker) is true.
Possibly useful example code shows one many ways you might track multiple blinking LED's.
It does include some conflicting advice from PaulS above concerning changing the sate of the LED's. Long time programmer so slightly different perspective.
My opinion, and that's all it is, is that the values of LOW and HIGH are not documented so their values should NOT be assumed thus my use of 'toggleState' in this example code.
On the other hand source code to the Wiring Library is of course included and upon looking at it one can determine that LOW = 0 and HIGH = 1. To me that still means undocumented. But the fact is you have access to the code and can see what they are. In my opunion looking at the code is NOT the same as documented as most people don't even realize the source is there to look at
To each their own!
/* =====================================================================================================================
* File - BlinkTrek.pde
* ---------------------------------------------------------------------------------------------------------------------
* A blink'en adventure
*/
#if defined(ARDUINO) && ARDUINO >= 100
#include "Arduino.h"
#else
#include "WProgram.h"
#endif
//#define DEBUG_PRINT
#ifndef DEBUG_PRINT
#define DebugPrint(X)
#define DebugPrintln(X)
#else
#define DebugPrint(X) Serial.print(X)
#define DebugPrintln(X) Serial.println(X)
#endif
#define SIZEOF_ARRAY(ARRAY) (sizeof(ARRAY) / sizeof(ARRAY[0]))
#if defined(_BOARD_CEREBOT_MX4CK_)
const uint8_t pinLED1 = 64; // ON-BOARD LED1
const uint8_t pinLED2 = 65; // ON-BOARD LED2
const uint8_t pinLED3 = 66; // ON-BOARD LED3
const uint8_t pinLED4 = 67; // ON-BOARD LED4
#elif defined(_BOARD_CEREBOT_MX7CK_)
const uint8_t pinLED1 = 51; // ON-BOARD LED1
const uint8_t pinLED2 = 52; // ON-BOARD LED2
const uint8_t pinLED3 = 53; // ON-BOARD LED3
const uint8_t pinLED4 = 54; // ON-BOARD LED4
#else
const uint8_t pinLED1 = ?; // YOUR LED PIN NUMBER HERE
const uint8_t pinLED2 = ?; // YOUR LED PIN NUMBER HERE
const uint8_t pinLED3 = ?; // YOUR LED PIN NUMBER HERE
#endif
const unsigned long tmsINTERVAL1 = 250UL;
const unsigned long tmsINTERVAL2 = 1000UL;
const unsigned long tmsINTERVAL3 = 500UL;
const unsigned long tmsINTERVAL4 = 750UL;
struct blinker_t
{
const uint8_t _pin; // pin number of associated LED
const unsigned long _dmsInterval; // delta time in millieseconds to advance trigger time
unsigned long _tmsTarget; // absolute time in millieseconds of next trigger (tms = time-in-millis)
uint8_t _state; // current value
};
blinker_t blinkers[] =
{
{ pinLED1, tmsINTERVAL1, 0UL, LOW }
, { pinLED2, tmsINTERVAL2, 0UL, LOW }
, { pinLED3, tmsINTERVAL3, 0UL, LOW }
#if defined(_BOARD_CEREBOT_MX4CK_) || defined(_BOARD_CEREBOT_MX7CK_)
, { pinLED4, tmsINTERVAL4, 0UL, LOW }
#endif
};
// toggle between two provided states: state and state2, or defaults of LOW/HIGH if unspecified
// <http://www.arduino.cc/playground/Code/BitMath>
template<typename T>
T toggleState(T& vstate, T const state1 = LOW, T const state2 = HIGH) {
return (vstate ^= (state1 ^ state2));
}
// <http://arduino.cc/playground/Code/TimingRollover>
bool isBlinkenTime(unsigned const long tmsRef, const unsigned long& tmsTarget)
{
return ((long)(tmsRef - tmsTarget) >= 0);
}
void loop()
{
unsigned long tmsRef = millis();
for ( size_t i = SIZEOF_ARRAY(blinkers); i--; )
{
if ( isBlinkenTime(tmsRef, blinkers[i]._tmsTarget) )
{
// ... it's blink'en time ...
// ... set next blink'en time ...
blinkers[i]._tmsTarget = (tmsRef + blinkers[i]._dmsInterval);
// ... toggle state and set LED to new state ...
digitalWrite(blinkers[i]._pin, toggleState(blinkers[i]._state));
// ... optional debuggin' output ...
// ... to enable uncomment 'DEBUG_PRINT' above ...
DebugPrint("\'blinker\'");
DebugPrint(i);
DebugPrint(": ");
DebugPrintln(((LOW == blinkers[i]._state) ? "HIGH" : "LOW"));
}
}
}
void setup()
{
Serial.begin(9600);
unsigned long tms = millis();
for ( size_t i = SIZEOF_ARRAY(blinkers); i--; )
{
pinMode(blinkers[i]._pin, OUTPUT);
blinkers[i]._tmsTarget = (tms + blinkers[i]._dmsInterval);
}
}
}
EDIT: Change in conditionals that set LED pins for my 2 boards.
Thank you for that, lloyddean. I will take a look through the example you provided and see what I can learn from it. I appreciate the "many ways to skin a cat" nature of the responses here. If I can just figure out how to make sure all LEDs are extinguished when the rocker is turned off, I'll be very happy.
wildbill:
Re-read reply #5. The issue is nailed there.
So I spent time trying to match the appropriate if and else statements. I simplified the case to just one flashing LED:
void loop()
{
unsigned long m = millis();
if (pressed[2])
{
if (m - time1 > interval1)
{
time1 = m;
if (value1 == LOW)
value1 = HIGH;
else value1 = LOW;
digitalWrite(9, value1);
}
}
else digitalWrite(9,LOW);
}
But the above, which was what I thought it should look like, didn't work. Nor did any of the other iterations I tried. Clearly I should stick to cutting plywood, as I am just not able to get my head around making this work. In the original series, they used flashing Christmas lights. Maybe I'll go that way and claim more authenticity. Thanks for all the attempts at help, guys!
Step back, take a break, come back and try again later.
When you come back to it, put { and } after every if and else statement (before and after the stuff that makes up the body of the if or else block. Then, auto format again.
I'd send someone looking for me to do a code review away if they did this:
else value1 = LOW;
else is a statement. value1 = LOW; is a statement. ONE statement per line!
else digitalWrite(9,LOW);
Same here...
You can use Serial.begin() in setup() and Serial.print() and Serial.println() in setup() and loop() to output messages and values, to help understand what the code is doing.
Have we determined that this is a software issue, rather than a hardware issue? The internal pullup resistor is not being used for the switch, so there needs to be an external pullup or pulldown resistor. Is there? How IS the switch wired?
Step back, take a break, come back and try again later.
Yeah, you're right. I'll take another stab at it today. If I can get it working, it will give the guy who will be getting the chair as a birthday gift, much more flexibility in the future. He is a programmer, and will have no trouble cleaning up my mess, I'm sure!
void loop()
{
unsigned long m = millis();
if (pressed[2])
{
if (m - time1 > interval1)
{
time1 = m;
if (value1 == LOW)
value1 = HIGH; // value1 = HIGH can we use it now?
else value1 = LOW;
digitalWrite(9, value1);
}
}
else digitalWrite(9,LOW);
}
I assume reply #13 didn't have all the code you're using, because there's nothing there that reads the state of the switch. If there were, it looks like it would work. Can you post the latest iteration?
PaulS:
When you come back to it, put { and } after every if and else statement (before and after the stuff that makes up the body of the if or else block. Then, auto format again.
Yep, that's what I'm planning to do. Your original advice on where to place the brackets, made it a lot easier for me to start to get a handle on what the code is supposed to be doing (as I mentioned, this was frankencode that I cobbled together from other examples.
I'd send someone looking for me to do a code review away if they did this:
else value1 = LOW;
else is a statement. value1 = LOW; is a statement. ONE statement per line!
I just noticed that this morning when taking another look. Again, this was probably my fault due to sloppy cut/paste work.
else digitalWrite(9,LOW);
Same here...
Ah... now this is certainly my fault. I must be misunderstanding how this is supposed to work. My simplistic understanding was that the "if" part tells the arduino to flash the LED if the switch grounds the input pin, and the "else" part was telling it to turn the LED off if the switch was not grounding the input pin (i.e., the input pin was HIGH). I'll have to do some research to see how the else statement is meant to work.
Have we determined that this is a software issue, rather than a hardware issue? The internal pullup resistor is not being used for the switch, so there needs to be an external pullup or pulldown resistor. Is there? How IS the switch wired?
I thought that by explicitly setting the input pins to HIGH (earlier in the sketch), I was in effect setting a pull up resistor. Is that not the case? The switches are wired with one pole going to the input pin and one to ground. So when the switch is thrown, it takes the voltage on the input pin from +5 to 0. That part of it was working, but I'll do some more reading to see if I have it set up properly.
Thanks again for your input. I really appreciate the guidance!
