I am trying to use a 60 Hz square wave (derived from an ac/ac adapter) to trigger an external interrupt in order to create a timing signal I can use for a time-telling clock. Although the square wave seems to be of a high enough quality, and the code seems ok, the clock runs almost, but not quite, twice as fast as it should. I'm quite new to programming, and find myself pretty stumped. Is there a problem with my code? or is there some glitch in my square wave that I'm not seeing on the scope somehow?
Square wave, as measured on pin 4 of ATMEGA328 (Arduino Dig2) using Tek TDS3014B:
Freq: 60.0Hz
High: 4.7V
Low: 0.6V
Rise time: 0.9ms
Fall time: 0.4ms
Duty cycle: High 53%
The code:
#define latchPin 19 //Pin connected to RCK of Shift Register
int clockPin = 13; //Pin connected to SCK of Shift Register
int dataPin = 10; //Pin connected to SER of Shift Register
#define clockFreq 60 //The frequency of the timing clock in Hz
volatile int clockCount = 0; //interrupt counter
int lastclockCount = 0; //Interrupt counter state change detection
int secOnes = 0; //Seconds ones register
int secTens = 0; //Seconds tens register
int minOnes = 0;
int minTens = 0;
int hourOnes = 2;
int hourTens = 1;
void setup() {
pinMode(2, INPUT);
attachInterrupt(0, clock, FALLING); // create interrupt. activates on falling edge of pulse applied to Dig2 (interrupt channel 0)
interrupts();
//set up pins for communication with shift registers
pinMode(latchPin, OUTPUT);
pinMode(clockPin, OUTPUT);
pinMode(dataPin, OUTPUT);
}
loop(){
//Time display routine
digitalWrite(latchPin, LOW); //ground latchPin and hold low for as long as you are transmitting
shiftOut(dataPin, clockPin, MSBFIRST, (hourTens * 16 + hourOnes));
shiftOut(dataPin, clockPin, MSBFIRST, (minTens * 16 + minOnes));
shiftOut(dataPin, clockPin, MSBFIRST, (secTens * 16 + secOnes));
digitalWrite(latchPin, HIGH); //return the latch pin high to end transmission
//Timing routine
if (clockCount != lastclockCount){ //Only need to go throug if a new interrupt has been triggered
lastclockCount = clockCount; //Reset interrupt state change detection
if((clockCount % clockFreq) == 0){ //Increment time by one second after enough cycles of the square wave accumulate
if ((secOnes = (secOnes + 1) % 10) == 0){ //Increment seconds ones register and check for rollover to tens
if ((secTens = (secTens + 1) % 6) == 0){ //Increment seconds tens register and check for rollover to minutes
if ((minOnes = (minOnes + 1) % 10) == 0){ //Increment minutes ones register and check for rollover to tens
if ((minTens = (minTens + 1) % 6) == 0){ //Increment minutes tens register and check for rollover to hours
if((hourTens * 10 + hourOnes) > 11){ //Hours increment and handling of 12:00AM/PM
hourOnes = 0;
hourTens = 0;
}
if ((hourOnes = (hourOnes + 1) % 10) == 0){
hourTens = hourTens + 1;
}
}
}
}
}
}
}
}
void clock(){
clockCount = clockCount + 1; //Interrupt routine
}
I am trying to use a 60 Hz square wave (derived from an ac/ac adapter)
Can we see a drawing of how this square wave is developed? If there is a full wave rectifer used then that will double the frequency to twice the line frequency.
Bill: That is all of the code for the clock so far. I still have not implemented any way to set it! It is a binary clock display on discrete neon lamps. The doubling appears in the visual clock output. In 20 "real" seconds (as measured by a stopwatch), ~32 seconds elapse on the clock display. I chose FALLING as this is the sharpest edge generated by the circuit I'm using to create the square wave.
Lefty: The circuit I'm using can sort of be found here: http://www.discovercircuits.com/PDF-FILES/60hzclk.pdf
As I'm driving it off of a 16VAC wall adapter, not the line voltage directly, I have it directly coupled (left out the capacitor) and increased the series resistance to 10K. The pull-up resistor flavor was easier for me to implement so again, that is where the FALLING choice came from. Aside from how I implemented it, I'm assuming the direct measurements I took with my scope show the issue is in my software...
That's not a "square wave". The input is hanging around in the "undefined" area between the "high" and "low" thresholds for thousands of CPU clock cycles. You may have been busted for loitering here. I'd start with adding a simple analog comparator to square up those edges. There's a good chance that'll be enough to clear up the problem, but there's also a chance that there's some sort of glitching going on that you're not seeing with the scope set to a low sweep frequency to display the 60Hz.
I am already completely out of space on perf-board I would like to use. Is there any way to use my square-wave err, I mean trapezoidal-wave [smiley=wink.gif] with some kind of software work-around?
I will bread-board it and try with a comparator when I get home from work. Thanks!
Try setting a flag in your interrupt routine instead of incrementing clockCount. You can then test the flag in the main loop. When it is set, delay several msec to get past the troublesome falling edge. Then, clear the flag (you might turn off interrupts while you do this) and increment clockCount. Note that the rising edge could still cause problems if it is noisy enough to trigger a falling edge interrupt.
const byte latchPin = 19; //Pin connected to RCK of Shift Register
const byte clockPin = 13; //Pin connected to SCK of Shift Register
const byte dataPin = 10; //Pin connected to SER of Shift Register
const unsigned clockFreq = 60; //The frequency of the timing clock in Hz
DO NOT access clockCount in loop without protecting it...
CodingBadly: Thank you so much for working on my code, that was very kind. I especially want to thank you for providing that link to your detailed explanation. I really wish they would include all your information on the attachInterrupt() reference page!
I will report back once I've had an opportunity to implement these fixes.
If you are having noise problems on your interrupt input, my earlier suggestion was effectively a way to "debounce" the signal. It works by setting a flag in the interrupt routine and incrementing the clockCount in the main loop after the flag has been detected and the debounce period has elapsed. You can play with the delay value as needed. 2 msec would get you past the falling edge but if the rising edge is also non-monotonic, you could increase the delay to something more than half the period (10 msec?) to cover it as well.
Here is what I mean, building on Coding Badly's modifications to your code:
const byte latchPin = 19; //Pin connected to RCK of Shift Register
const byte clockPin = 13; //Pin connected to SCK of Shift Register
const byte dataPin = 10; //Pin connected to SER of Shift Register
const unsigned clockFreq = 60; //The frequency of the timing clock in Hz
volatile byte clockFlag = false; //Interrupt flag
unsigned clockCount = 0; //interrupt counter
int secOnes = 0; //Seconds ones register
int secTens = 0; //Seconds tens register
int minOnes = 0;
int minTens = 0;
int hourOnes = 2;
int hourTens = 1;
void setup() {
pinMode(2, INPUT);
attachInterrupt(0, clock, FALLING); // create interrupt. activates on falling edge of pulse applied to Dig2 (interrupt channel 0)
interrupts();
//set up pins for communication with shift registers
pinMode(latchPin, OUTPUT);
pinMode(clockPin, OUTPUT);
pinMode(dataPin, OUTPUT);
}
void loop(){
//Time display routine
digitalWrite(latchPin, LOW); //ground latchPin and hold low for as long as you are transmitting
shiftOut(dataPin, clockPin, MSBFIRST, (hourTens * 16 + hourOnes));
shiftOut(dataPin, clockPin, MSBFIRST, (minTens * 16 + minOnes));
shiftOut(dataPin, clockPin, MSBFIRST, (secTens * 16 + secOnes));
digitalWrite(latchPin, HIGH); //return the latch pin high to end transmission
//Timing routine
if (clockFlag){ //Only need to go through if a new interrupt has been triggered
delay(2); //Debounce interrupt input
uint8_t SaveSREG = SREG;
cli();
clockFlag = false;
SREG = SaveSREG;
clockCount++;
if((clockCount % clockFreq) == 0){ //Increment time by one second after enough cycles of the square wave accumulate
if ((secOnes = (secOnes + 1) % 10) == 0){ //Increment seconds ones register and check for rollover to tens
if ((secTens = (secTens + 1) % 6) == 0){ //Increment seconds tens register and check for rollover to minutes
if ((minOnes = (minOnes + 1) % 10) == 0){ //Increment minutes ones register and check for rollover to tens
if ((minTens = (minTens + 1) % 6) == 0){ //Increment minutes tens register and check for rollover to hours
if((hourTens * 10 + hourOnes) > 11){ //Hours increment and handling of 12:00AM/PM
hourOnes = 0;
hourTens = 0;
}
if ((hourOnes = (hourOnes + 1) % 10) == 0){
hourTens = hourTens + 1;
}
}
}
}
}
}
}
}
void clock(){
clockFlag = true; //Interrupt routine
}
The problem is with my soft edges. I hooked up a function generator with nanosecond rise/fall times and the clock immediately started behaving correctly. Over three minutes, it didn't lose or gain a single second. Of course, at this point, I'd still much rather find a work around in the software for my crappy, but simply created timing signal...
To that end, I tried to implement a rather simple minded "debounce" technique. To the very end of my loop() I added:
delay(1); //Delay for more than the maximum rise or fall time of my signal
interrupts(): //Re-enable the interrupt only after the signal in no longer in the undefined region.
And to my clock() function I added:
void clock(){
clockCount = clockCount + 1;
noInterrupts(); //Turn-off interrupts to prevent retriggering on same edge
}
I did all of that right before going out for dinner. My very simple approach worked, well, mostly. In 2.5 hours the clock gained an additional 3 minutes. I could extend the delay to something like 4 or 8 milliseconds to be sure I've gotten out of the undefined region on the input, but I'm also suspect that using noInterrupts() inside an interrupt isn't kosher. Of course there still may be some actual glitch in the timing signal, but I've used two different scope to check it out, and the one at my work is much better for detecting glitches.
I thought about debouncing it cjands way before he was kind enough to post the code, but won't my clock() interrupt interrupt the delay() function? Without being able to turn-off my clock() interrupt from inside the function, I don't see a way to "de-bounce" the crappy edge.
Here is what I mean, building on Coding Badly's modifications to your code
In my message I made this statement...
DO NOT access clockCount in loop without protecting it
That also applies to clockFlag in your code.
While it is unlikely to cause problems, there is a race condition in your code.
Add this...
byte CheckAndClearFlag( volatile byte& Flag )
{
byte Result;
uint8_t SaveSREG = SREG;
cli();
Result = Flag;
Flag = false;
SREG = SaveSREG;
return( Result );
}
Change the if to this...
//Timing routine
if ( CheckAndClearFlag( clockFlag ) ) { //Only need to go through if a new interrupt has been triggered
delay(2); //Debounce interrupt input
...
I'm not convinced it does (but I'm always willing to be enlightened). My flag is a single byte that I test for true or false. The ISR only sets the flag to true while the main loop only clears it. In fact, I'm not even convinced the main loop needs to disable interrupts before clearing the flag in this particular example.
Also, I believe your changes would defeat my debounce feature - I deliberately inserted the delay between checking the flag and clearing it so that any spurious interrupts would be ignored.
I thought about debouncing it cjands way before he was kind enough to post the code, but won't my clock() interrupt interrupt the delay() function? Without being able to turn-off my clock() interrupt from inside the function, I don't see a way to "de-bounce" the crappy edge.
Yes, the noisy edges could cause additional interrupts to occur during the call to delay() but those would have no effect since the interrupt routine only sets a flag. The idea is to set the flag on the first interrupt and then wait to act on it (including clearing the flag) until the noise danger has passed.
To the very end of my loop() I added:
delay(1); //Delay for more than the maximum rise or fall time of my signal
interrupts(): //Re-enable the interrupt only after the signal in no longer in the undefined region
Calling interrupts without calling noInterrupts inside of loop has no effect. In loop, interrupts are already enabled.
And to my clock() function I added
void clock(){
clockCount = clockCount + 1;
noInterrupts(); //Turn-off interrupts to prevent retriggering on same edge
}
Same as above but the other way around. Inside an interrupt service routine, interrupts are already disabled. Calling noInterrupts here has no effect.
I'm also suspect that using noInterrupts() inside an interrupt isn't kosher
It adds a very tiny delay but has no other effect.
I thought about debouncing it cjands way before he was kind enough to post the code, but won't my clock() interrupt interrupt the delay() function?
Yes. But if the debounce is coded correctly, it doesn't matter.
I'm not convinced it does (but I'm always willing to be enlightened)
No need. I made a mistake.
My flag is a single byte that I test for true or false. The ISR only sets the flag to true while the main loop only clears it. In fact, I'm not even convinced the main loop needs to disable interrupts before clearing the flag in this particular example.
For the benefit of the curious: this is the ONE case when it is not necessary to protect the data in loop.
Also, I believe your changes would defeat my debounce feature
They do.
A much better place to clear the flag is at the end of the if. The delay may no longer be necessary...
const byte latchPin = 19; //Pin connected to RCK of Shift Register
const byte clockPin = 13; //Pin connected to SCK of Shift Register
const byte dataPin = 10; //Pin connected to SER of Shift Register
const unsigned clockFreq = 60; //The frequency of the timing clock in Hz
volatile byte clockFlag = false; //Interrupt flag
unsigned clockCount = 0; //interrupt counter
int secOnes = 0; //Seconds ones register
int secTens = 0; //Seconds tens register
int minOnes = 0;
int minTens = 0;
int hourOnes = 2;
int hourTens = 1;
void setup() {
pinMode(2, INPUT);
attachInterrupt(0, clock, FALLING); // create interrupt. activates on falling edge of pulse applied to Dig2 (interrupt channel 0)
interrupts();
//set up pins for communication with shift registers
pinMode(latchPin, OUTPUT);
pinMode(clockPin, OUTPUT);
pinMode(dataPin, OUTPUT);
}
void loop(){
//Time display routine
digitalWrite(latchPin, LOW); //ground latchPin and hold low for as long as you are transmitting
shiftOut(dataPin, clockPin, MSBFIRST, (hourTens * 16 + hourOnes));
shiftOut(dataPin, clockPin, MSBFIRST, (minTens * 16 + minOnes));
shiftOut(dataPin, clockPin, MSBFIRST, (secTens * 16 + secOnes));
digitalWrite(latchPin, HIGH); //return the latch pin high to end transmission
//Timing routine
if (clockFlag){ //Only need to go through if a new interrupt has been triggered
clockCount++;
if((clockCount % clockFreq) == 0){ //Increment time by one second after enough cycles of the square wave accumulate
if ((secOnes = (secOnes + 1) % 10) == 0){ //Increment seconds ones register and check for rollover to tens
if ((secTens = (secTens + 1) % 6) == 0){ //Increment seconds tens register and check for rollover to minutes
if ((minOnes = (minOnes + 1) % 10) == 0){ //Increment minutes ones register and check for rollover to tens
if ((minTens = (minTens + 1) % 6) == 0){ //Increment minutes tens register and check for rollover to hours
if((hourTens * 10 + hourOnes) > 11){ //Hours increment and handling of 12:00AM/PM
hourOnes = 0;
hourTens = 0;
}
if ((hourOnes = (hourOnes + 1) % 10) == 0){
hourTens = hourTens + 1;
}
}
}
}
}
}
delay(2); //Debounce interrupt input
[glow]clockFlag = false;[/glow]
}
}
void clock(){
clockFlag = true; //Interrupt routine
}
Thanks! I can see how using a counter was messing up the way I was thinking about the problem. The flag is clearly the way to go.
As it turns out, both edges are sufficiently crappy that a delay of greater than half the period is required or it runs at roughly double the frequency.
I'm now curious to know how my broken debounce implementation still improved the performance of the clock. Clearly it wasn't doing at all what I thought it was in terms of turning the interrupt on and off, but the delay still greatly reduced the number of unwanted interrupts. Oddly, that worked with only a 1-4ms delay (alright, it only sort-of worked), but cjands requires 8ms or more.
Thanks again to everyone for their generous help! I've set up the clock to run overnight to see how the new, much improved code keeps time compared to another digital clock that keeps time off the 60Hz AC signal.
I'm now curious to know how my broken debounce implementation still improved the performance of the clock.
Me too! Any chance you still had a scope probe attached when you ran that test? They are notorious for cleaning up signal integrity problems
Otherwise, I wonder if the noInterrupts() call you added to the interrupt routine helped by simply making the routine take longer to execute. Extending the time with interrupts disabled could have gotten you past the worst of the noise. It shouldn't have helped with any glitches on the rising edge, though.
