Square wave generator and frequency meter

Hello,

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:

unsigned long waveHz = 10000;
unsigned int exacttime, numofovf;
volatile unsigned int timeovf;
unsigned long waveTime;

void setup() {

  pinMode(2, OUTPUT);
  waveTime = (1.0/(waveHz*2.0))*16000000;
  numofovf = waveTime/65536;
  exacttime = waveTime-(numofovf*65536);
  cli();           // disable all interrupts
  TCCR1A = 0;
  TCCR1B = 0;
  TCNT1  = 0;
  OCR1A = exacttime;           
  TCCR1B |= (1 << CS10);    // no prescaler
  TIMSK1 |= (1 << OCIE1A);
  TIMSK1 |= (1 << TOIE1); // enable timer compare interrupt
  sei();  
}

void loop() {
}

ISR(TIMER1_COMPA_vect){
  if(timeovf == numofovf){
    TCNT1 = 0;
    timeovf = 0;
    PORTD ^= bit(2);
  }
}

ISR(TIMER1_OVF_vect){
  timeovf++;
}

Second project is frequency meter. You input some signal on pin d2 and it prints out frequency of that signal via serial connection. Here is the code:

unsigned int timervalue, overflows, timeovf;
float timeofwave, hz;
void setup() {
  pinMode(2, INPUT);
  pinMode(LED_BUILTIN, OUTPUT);
  attachInterrupt(digitalPinToInterrupt(2), wavecheck, RISING);
  Serial.begin(9600);
  cli();          
  TCCR1A = 0;
  TCCR1B = 0;
  TCNT1  = 0;
  TCCR1B |= (1 << CS10);    
  TIMSK1 |= (1 << OCIE1A); 
  sei();  
}

void loop() {
  timeofwave = overflows*65535+timervalue;
  sei();
  hz = 16000000/timeofwave;
  Serial.println(hz, 2);
}

ISR(TIMER1_OVF_vect){
  timeovf++;
}

void wavecheck() {
  cli();
  timervalue = TCNT1;
  TCNT1 = 0;
  overflows = timeovf;
  timeovf = 0;
}

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.

Very much thanks and have a good day.

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

If you want more more accurate detection of period and frequency, here is a sketch I wrote to use the InputCapture feature of Timer1:

// Measures the HIGH width, LOW width, frequency, and duty-cycle of a pulse train
// on Arduino UNO Pin 8 (ICP1 pin).  

// Note: Since this uses Timer1, Pin 9 and Pin 10 can't be used for
// analogWrite().

void setup()
{
  Serial.begin(115200);
  while (!Serial);

  // For testing, uncomment one of these lines and connect
  // Pin 3 or Pin 5 to Pin 8
  // analogWrite(3, 64);  // 512.00, 1528.00, 2040.00, 25.10%, 490.20 Hz
  // analogWrite(5, 64);  // 260.00, 764.00, 1024.00, 25.39%, 976.56 Hz

  noInterrupts ();  // protected code
  // reset Timer 1
  TCCR1A = 0;
  TCCR1B = 0;
  TCNT1 = 0;
  TIMSK1 = 0;

  TIFR1 |= _BV(ICF1); // clear Input Capture Flag so we don't get a bogus interrupt
  TIFR1 |= _BV(TOV1); // clear Overflow Flag so we don't get a bogus interrupt

  TCCR1B = _BV(CS10) | // start Timer 1, no prescaler
           _BV(ICES1); // Input Capture Edge Select (1=Rising, 0=Falling)

  TIMSK1 |= _BV(ICIE1); // Enable Timer 1 Input Capture Interrupt
  TIMSK1 |= _BV(TOIE1); // Enable Timer 1 Overflow Interrupt
  interrupts ();
}

volatile uint32_t PulseHighTime = 0;
volatile uint32_t PulseLowTime = 0;
volatile uint16_t Overflows = 0;

ISR(TIMER1_OVF_vect)
{
  Overflows++;
}

ISR(TIMER1_CAPT_vect)
{
  static uint32_t firstRisingEdgeTime = 0;
  static uint32_t fallingEdgeTime = 0;
  static uint32_t secondRisingEdgeTime = 0;

  uint16_t overflows = Overflows;

  // If an overflow happened but has not been handled yet
  // and the timer count was close to zero, count the
  // overflow as part of this time.
  if ((TIFR1 & _BV(TOV1)) && (ICR1 < 1024))
    overflows++;

  if (PulseLowTime == 0)
  {
    if (TCCR1B & _BV(ICES1))
    {
      // Interrupted on Rising Edge
      if (firstRisingEdgeTime)  // Already have the first rising edge...
      {
        // ... so this is the second rising edge, ending the low part
        // of the cycle.
        secondRisingEdgeTime = overflows; // Upper 16 bits
        secondRisingEdgeTime = (secondRisingEdgeTime << 16) | ICR1;
        PulseLowTime = secondRisingEdgeTime - fallingEdgeTime;
        firstRisingEdgeTime = 0;
      }
      else
      {
        firstRisingEdgeTime = overflows; // Upper 16 bits
        firstRisingEdgeTime = (firstRisingEdgeTime << 16) | ICR1;
        TCCR1B &= ~_BV(ICES1); // Switch to Falling Edge
      }
    }
    else
    {
      // Interrupted on Falling Edge
      fallingEdgeTime = overflows; // Upper 16 bits
      fallingEdgeTime = (fallingEdgeTime << 16) | ICR1;
      TCCR1B |= _BV(ICES1); // Switch to Rising Edge
      PulseHighTime = fallingEdgeTime - firstRisingEdgeTime;
    }
  }
}

void loop()
{
  noInterrupts();
  uint32_t pulseHighTime = PulseHighTime;
  uint32_t pulseLowTime = PulseLowTime;
  interrupts();

  // If a sample has been measured
  if (pulseLowTime)
  {
    // Display the pulse length in microseconds
    Serial.print("High time (microseconds): ");
    Serial.println(pulseHighTime / 16.0, 2);
    Serial.print("Low time (microseconds): ");
    Serial.println(pulseLowTime / 16.0, 2);

    uint32_t cycleTime = pulseHighTime + pulseLowTime;
    Serial.print("Cycle time (microseconds): ");
    Serial.println(cycleTime / 16.0, 2);

    float dutyCycle = pulseHighTime / (float)cycleTime;
    Serial.print("Duty cycle (%): ");
    Serial.println(dutyCycle * 100.0, 2);

    float frequency = (float)F_CPU / cycleTime;
    Serial.print("Frequency (Hz): ");
    Serial.println(frequency, 2);
    Serial.println();

    delay(1000);  // Slow down output

    // Request another sample
    noInterrupts();
    PulseLowTime = 0;
    interrupts();
  }
}

Note carefully the first remark; it only works on pin 8.

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

attachInterrupt(digitalPinToInterrupt(2), wavecheck, RISING);

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! :slight_smile:

He's probably waiting to see your responses to the issues I mentioned...

i responded you up there lol

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.

But cli does exactly opposite - which means disabling interrupts not enabling them...

also if it should overflow unsigned int calcullation then tell me why it actually doesnt in my case?

You could try this way to do a square wave generator.

For anyone interested here's the .h for the blinker object..

#ifndef blinker_h
#define blinker_h

#include "squareWave.h"

// Some defaults in case the user just doesn't care..
#define defPin 13
#define defOnMs 50
#define defPeriodMs 400

class blinker : public squareWave {
	
public:
				blinker(int inPin=defPin,float inOnMs=defOnMs, float inPeriodMs=defPeriodMs,bool inInverse=false);
	virtual	~blinker(void);
				
	virtual	void	setOnOff(bool onOff);			// Start or stop the blinking..
				bool	blinking();							// We blinking or what?
	virtual	void	pulseOn(void);
	virtual	void	pulseOff(void);
	
	protected:

		bool  	init;
		int      pin;
		bool  	inverse;
};

#endif

And it's built on the squareWave object that has even more delightful options to play with.

-jim lee

Hello, where can i get this sqareWave.h?

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()?

because i disable them in interrupt with cli, so i reenable them after all work is done to get another sample.

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.

If you have updated your sketch please re-post it in a new message.