Im working on 2 separate project at the same time, but i think they both need to be repaired at once.
So first project is a simple square wave generator. You put in wave frequency, and it should output square wave with that exact frequency on pin d2. Here is code:
So basically everything works fine on frequencies from 1hz to 100hz. But strange thing starts to happen for higher frequencies. If i generate a 10000hz wave the frequency meter shows 9500hz.
Thats a preety big error. I expected to see 10000hz not 9500hz. I get even bigger mess if i generate 100000hz signal. It reads anywhere from 33000hz to 170000hz randomly.
Now the question is: what is wrong with my implementation - is it the wave generator or wave reader? Maybe its both? Please help me understand what i did wrong and if its even doable.
Ironically, 'overflows' is an unsigned int. What is the biggest number you think an int can represent?
There are other issues... you use sei() in loop() then never any cli(). You never call the function 'wavecheck()' and it's not clear what it even does...
Ye so basically unsigned int is more than enough because i dont input signal lower than 1hz. If timer 1 goes up by 1 with frequency of 16mhz, and it overflows at 65535 then an overflow will happen every 4milisecond which would take 262140ms to take up this whole int space. 262 seconds of pause between signals to get this int full. It means that if my signal frequency is any greater than 0.0038hz this is good to go.
Also i use cli() in function wavecheck - which btw is called every time there is rising edge on pin 2...
its great, but i dont wanna just copy paste your code... If you could point some flaws in my code or at least how our code differs that would be great!
No. C expressions are evaluated as 'int' unless told otherwise. So in the first
timeofwave = overflows*65535+timervalue;
Although 'timeofwave' is a float, the expression is evaluated left to right, the first calculation is 'overflows*65535', overflows is an unsigned int, any value greater than one will overflow the unsigned int calculation because the result of an unsigned int calculation is also an unsigned int.
You can fix it with
timeofwave = overflows*65535UL+timervalue;
'cli()' in 'waveCheck()' is not good, you are enabling interrupts inside an interrupt, which allows other interrupts to occur. It's not that this is specifically harmful, it's just a sign that it's probably not doing what you expect it to do. If you're trying to protect the variables while you do an atomic read of them in loop(), you should move the 'cli()' to loop() where it belongs.
You were lucky. the constant 65535 is interpreted by the compiler as a long. I tested numbers below 32768 and it fails. In this case, you get away with it.
Interrupts are automatically disabled when an ISR is called. You don't have to tamper with them. Why do you enable interrupts every time through loop() with sei()?
Modern software wouldn't work at all if things were that cumbersome. Interrupts are automatically re-enabled by the processor when a RETI instruction causes a return from the ISR.
Also, two wrongs don't make a right. You had no reason to disable them in the ISR in the first place.
i changed my code a little so now cli and sei are outside of interrupt call function. Also this doesnt solve my main problem - which is why on higher frequencies they are waaaay off.
Just a note, the frequency generation and measurement is based on a 16000 Mhz clock. Although folks think of a crystal as the epitome of accuracy, they can have significant error.