Help with Zero Crossing code

Hi.
The code below enters on an ISR everytime the pin 2 (interrupt 0) receives a pulse on negative edge from a zero crossing detector circuit. If I use delays instead of if conditions with micros() the code works fine. This project is about AC control of multiple lamps so I can't use delays.

Arduino is triggering the ISR but it's not generating the phased pulse (dimmer) inside the ISR. Dimmer is constantly 0 V. The code was tested on simulation (ISIS Proteus) and on a real Arduino UNO but both presented the same result.
EDIT: Code revised with suggestions

#define dimmer 3

// Initializing variables

volatile          long pot      = 0.0;          // potentiometer reading

volatile unsigned long t        = micros();     // time
volatile unsigned long lastTime = 0.0;          // last time
volatile unsigned long period   = 8333;         // Period (us)
volatile unsigned long t_on     = 100.0;        // pulse duration

volatile          bool state    = LOW;          // pulse's state - HIGH/LOW

void zeroCross() { // ISR

  // Calculates t1 that depends on potentiometer readings
  unsigned long t1 = 8333L * (100L - pot) / 100L;

  t = micros(); // Added after users suggestions

  if ((t - lastTime) <= t1 || ((t - lastTime) >= (t1 + t_on))) {
    state = LOW;
  }
  else {
    state = HIGH;
  }

  if ((t - lastTime) >= period) {
    lastTime = t;
  }

  digitalWrite(dimmer, state);

}
void setup() {
  pinMode(dimmer, OUTPUT);
  attachInterrupt(0, zeroCross, FALLING);
}

void loop() {
  pot = map(analogRead(A0), 0, 1023, 1, 99); // Reads potentiometer
}

What can I do to make this code work? I'm pretty sure my logic is right :grinning:

I've attached an image where it shows the logic I built for this.


You never update 't'

See Arduino Playground - ACPhaseControl

aarg:
You never update 't'

I update it every 8333 us, right?

Sorry, but I can't see this problem.

You say t = micros() in your initialization code (which only runs once) and ignore micros() afterwards.

JCA34F:
You say t = micros() in your initialization code (which only runs once) and ignore micros() afterwards.

I put t = micros(); inside ZeroCross() ISR and inside loop(), but the problem still remains.

Please post the entire revised sketch in a new post, so that we can have a look at the changes you made.

aarg:
Please post the entire revised sketch in a new post, so that we can have a look at the changes you made.

@aarg I edited the current post, so You can see the changes I made in the code.

danicomartins:
@anon57585045 I edited the current post, so You can see the changes I made in the code.

There is a reason why I asked you to do it specifically that way. If you edit previous posts, it makes the replies look like nonsense and so makes it hard for people to follow from the beginning. But too late now, please post according to the forum guidelines next time.

Have you tried bypassing the pot reading?

void loop() {
//  pot = map(analogRead(A0), 0, 1023, 1, 99); // Reads potentiometer
pot = 50;
}

aarg:
...

Have you tried bypassing the pot reading?

void loop() {

//  pot = map(analogRead(A0), 0, 1023, 1, 99); // Reads potentiometer
pot = 50;
}

Yes, I did, but it didn't work. The output (dimmer) remains at 0 V.

I'm pretty sure my logic is right

I don't think so.

void zeroCross() { // ISR

  // Calculates t1 that depends on potentiometer readings
  unsigned long t1 = 8333L * (100L - pot) / 100L;

  t = micros(); // Added after users suggestions

  if ((t - lastTime) <= t1 || ((t - lastTime) >= (t1 + t_on))) {
    state = LOW;
  }
  else {
    state = HIGH;
  }

  if ((t - lastTime) >= period) {
    lastTime = t;
  }

  digitalWrite(dimmer, state);

}

t-lastTime will always be equal to 8333 and I believe that value will always satisfy the second condition and the output will be LOW.

if ((t - lastTime) <= t1 || ((t - lastTime) >= (t1 + t_on))) {
    state = LOW;

Your ISR is called through the zero-crossing signal. It runs down once per zero-crossing-signal and that's it.
Until the next zero-crossing-signal occures. Again running down once and that's it.

The time when it is called is always when the zero-crossing signal occurs but not at a time that has some delay after the zero-crossing signal.

Timing with millis() or micros() needs a functionality that the if-condition that is used to check if a certain timeperiod is over will be checked repeatedly thousands of times per second. The code you have so far does not check repeatedly.

The basic principle for timing with micros() is:
the ISR just sets a boolean flag. And your main-loop checks if flag is set true. If flag has value true start timing

best regards Stefan

Your code needs to be in loop(), not the ISR. The ISR can record the latest zero-crossing, but its not
happening when the output needs to be toggled.

That can be tested for in loop, or perhaps scheduled using a timer interrupt if you want to do this
in the background with accurate timing.

Do you understand state-transition diagrams?

First of all, Thank You for the time You took to help me. I've made some progression on my code. Let's go

cattledog:
t-lastTime will always be equal to 8333 and I believe that value will always satisfy the second condition and the output will be LOW.

@cattledog You're right. So I changed the conditions to

if (t <= t1 + lastTime) {
      state = LOW;
    }
    else if (t >= t1 + t_on + lastTime) {
      state = LOW;
    }

StefanL38:
Your ISR is called through the zero-crossing signal. It runs down once per zero-crossing-signal and that's it.
Until the next zero-crossing-signal occures. Again running down once and that's it.

The time when it is called is always when the zero-crossing signal occurs but not at a time that has some delay after the zero-crossing signal.

Timing with millis() or micros() needs a functionality that the if-condition that is used to check if a certain timeperiod is over will be checked repeatedly thousands of times per second. The code you have so far does not check repeatedly.

The basic principle for timing with micros() is:
the ISR just sets a boolean flag. And your main-loop checks if flag is set true. If flag has value true start timing

best regards Stefan

MarkT:
Your code needs to be in loop(), not the ISR. The ISR can record the latest zero-crossing, but its not
happening when the output needs to be toggled.

That can be tested for in loop, or perhaps scheduled using a timer interrupt if you want to do this
in the background with accurate timing.

Do you understand state-transition diagrams?

@Stefan and @MarkT, I thought the ISR would last untill all conditions are satisfied and all actions are made, but I was wrong and You're right on your statement. I've made some changes in the code with your suggestions:

void zeroCross() {          // ISR
  flag = HIGH;              // Activates the flag everytime an interrupt occurs
}

With all changes the NEW code is down below. What happens now is that the pulse (dimmer signal) works for some time, falls down to zero (0 V) for some time, and then it works again and so on. t1 should be constant as the pot is constant too, but in first microseconds t1 varies until it reaches the constante value. (attached images)
image graphs:
Zero Cross signal -> blue
dimmer (pulse signal) -> red
Sine rms on lamp -> green

#define dimmer 4                                // OUTPUT on Pin 4

// Initializing variables
         long pot      = 0.0;          // potentiometer reading

unsigned long t        = 0.0;          // time
unsigned long lastTime = 0.0;          // last time
unsigned long period   = 8333;         // Period (us)
unsigned long t_on     = 100.0;        // pulse duration

          bool state    = LOW;          // pulse's state - HIGH/LOW
volatile  bool flag     = LOW;          // flag that indicates ISR triggering

void zeroCross() {          // ISR
  flag = HIGH;              // Activates the flag everytime an interrupt occurs
}

void setup() {
  pinMode(dimmer, OUTPUT);
  attachInterrupt(digitalPinToInterrupt(2), zeroCross, FALLING);
}

void loop() {
  pot = 50.0; // potentiometer reading - it actually comes from an analogRead(A0)
              // It is set to 50 just to simplify the test

  // Calculates t1 that depends on potentiometer readings
  unsigned long t1 = 8333L * (100L - pot) / 100L;

  t = micros();

  // If flag is HIGH, the dimmer signal routine starts
  if (flag == HIGH) {
    if (t <= t1 + lastTime) {
      state = LOW;
    }
    else if (t >= t1 + t_on + lastTime) {
      state = LOW;
    }
    else {
      state = HIGH;
    }
    if (t - lastTime >= period) { // period end check
      lastTime = t;
      flag = LOW;                   // If period has passed away, it sets flag to LOW
    }
  }

  digitalWrite(dimmer, state);    // OUTPUT
}

readings1.png

readings2.png

I think you may be having conflicts between when the flag is set HIGH in the interrupt and LOW in loop based on the period.

Try this variation where the flag is set LOW after the 100us trigger signal to the ssr is brought LOW and the zero cross time ("lastTime") is set in the ISR.

Your timing conditions should be expressed as subtractions to prevent rollover issues.

#define dimmer 4                                // OUTPUT on Pin 4

// Initializing variables
long pot      = 0.0;          // potentiometer reading

unsigned long t        = 0.0;          // time
volatile unsigned long lastTime = 0.0;          // last time
unsigned long period   = 8333;         // Period (us)
unsigned long t_on     = 100.0;        // pulse duration

bool state    = LOW;          // pulse's state - HIGH/LOW
volatile  bool flag     = LOW;          // flag that indicates ISR triggering

void zeroCross() {          // ISR
  flag = HIGH;              // Activates the flag everytime an interrupt occurs
  lastTime = micros();   
}

void setup() {
  pinMode(dimmer, OUTPUT);
  attachInterrupt(digitalPinToInterrupt(2), zeroCross, FALLING);
}

void loop() {
  pot = 50.0; // potentiometer reading - it actually comes from an analogRead(A0)
  // It is set to 50 just to simplify the test

  // Calculates t1 that depends on potentiometer readings
  unsigned long t1 = 8333L * (100L - pot) / 100L;

  t = micros();

  // If flag is HIGH, the dimmer signal routine starts
  if (flag == HIGH) 
  {
    //if (t <= t1 + lastTime) 
      if (t - lastTime <= t1) 
    {
      state = LOW;
    }
    //else if (t >= t1 + t_on + lastTime) 
     else if (t - lastTime >= t1 + t_on) 
    {
      state = LOW;
      flag = LOW;    
    }
    else 
    {
      state = HIGH;
    }
  //  if (t - lastTime >= period) 
  //  { // period end check
   //   lastTime = t;
   //   flag = LOW;                   // If period has passed away, it sets flag to LOW
   // }
  }

  digitalWrite(dimmer, state);    // OUTPUT
}

All this said, I think that you are going to be more successful following the timer based AC phase control tutorial referenced earlier in the thread.

cattledog:
I think you may be having conflicts between when the flag is set HIGH in the interrupt and LOW in loop based on the period.

Try this variation where the flag is set LOW after the 100us trigger signal to the ssr is brought LOW and the zero cross time ("lastTime") is set in the ISR.

Your timing conditions should be expressed as subtractions to prevent rollover issues.

All this said, I think that you are going to be more successful following the timer based AC phase control tutorial referenced earlier in the thread.

That was a good point. With your code some pulses are not being triggered, in other words there are some skiping pulses (attached image). Can we fix that? Note that t1 is always at 4333 us which is right because pot = 50. So I think we're close to solve the problem. :slight_smile:

So my intention is to make a PID to control Vrms on a resistance load and with that the temperature will be controlled as well. I need to keep the code as simple as possible because i'm not an expert on internal Timers/Registers of Arduino as shown in the referenced tutorial

Newreadings1.png

danicomartins:
That was a good point. With your code some pulses are not being triggered, in other words there are some skiping pulses (attached image). Can we fix that? Note that t1 is always at 4333 us which is right because pot = 50. So I think we're close to solve the problem. :slight_smile:

I solved this particular problem by making lastTime = t instead of lastTime = micros

void zeroCross() {          // ISR
  flag = HIGH;              // Activates the flag everytime an interrupt occurs
  lastTime = t;   
}
[\code]

BUT if I do

pot = map(analogRead(A0), 0, 1023, 1, 99);

the output pulse is always 0 V. Solved a problem and got another one :P

did you read this?

there are two working simple sketches

pot = map(analogRead(A0), 0, 1023, 1, 99);

analogRead() is relatively slow and blocking. It takes about 110 microseconds which is longer than the pulse timing. I think that reading the pot every pass through loop() is creating problems for the tight timing of the sketch

There is not need to read the pot every pass through loop. How responsive does the duty cycle change need to be?

Try putting the analogRead() on a blink without delay style millis() timer so that you only read the value every second.