Digital pins affecting each other, when they shouldn't

Hi guys, I will try to be quick.

I’m using Arduino NANO, and controlling 3 LEDs with 4 potentiometers. One potentiometer changes blinking frequency/period length of all LEDs, the other 3 are used for individually changing duty cycle of LEDs.

Duty cycle – ratio between ON and OFF in one period of LED.

This is the code I’m using…

int counter = -1;
static int width1 = 0, width2 = 0, width3 = 0;
static int duty1 = 0, duty2 = 0, duty3 = 0, duty_scaled1 = 0, duty_scaled2 = 0, duty_scaled3 = 0;
static int period = 0, period_scaled = 0;



ISR(TIMER1_OVF_vect) {

  counter++;
  //--------------------------
  if (counter == 0) {
    PORTB |= (1 << PB7);
    digitalWrite(2, HIGH);
  }
  if (counter >= width1) {
    PORTB &= ~(1 << PB7);
    digitalWrite(2, LOW);
  }
  if (counter >= period_scaled) {
    counter = -1;
  }
  //------------------------

  if (counter == 0) {
    PORTB |= (1 << PB7);
    digitalWrite(3, HIGH);
  }
  if (counter >= width2) {
    PORTB &= ~(1 << PB7);
    digitalWrite(3, LOW);
  }
  if (counter >= period_scaled) {
    counter = -1;
  }
  //------------------------
  if (counter == 0) {
    PORTB |= (1 << PB7);
    digitalWrite(4, HIGH);
  }
  if (counter >= width3) {
    PORTB &= ~(1 << PB7);
    digitalWrite(4, LOW);
  }
  if (counter >= period_scaled) {
    counter = -1;
  }
  //------------------------

  TCNT1 = 64910;
}

void timer1_init() {

  TCCR1B |= 3;
  TCNT1 = 64910;
  TIMSK1 |= (1 << TOIE1);
  sei();
}




void setup() {

  DDRB |= (1 << PB7);
  PORTB |= (1 << PB7);
  pinMode(2, OUTPUT);         //LED1
  pinMode(3, OUTPUT);        //LED2
  pinMode(4, OUTPUT);       //LED3
  PORTB &= ~(1 << PB7);
  TCCR1A = 0;
  TCCR1B = 0;
  timer1_init();
  Serial.begin(9600);
  Serial.println("ok");

}


char buffer_printout[50];


void loop() {

  period = analogRead(A0);
  period_scaled = map(period, 0, 1023, 0, 1000);
  duty1 = analogRead(A1);
  duty2 = analogRead(A2);
  duty3 = analogRead(A3);
  duty_scaled1 = map(duty1, 0, 1000, 0, 1000);
  duty_scaled2 = map(duty2, 0, 1000, 0, 1000);
  duty_scaled3 = map(duty3, 0, 1000, 0, 1000);
  width1 = map(duty1, 0, 1000, 0, period_scaled);
  width2 = map(duty2, 0, 1000, 0, period_scaled);
  width3 = map(duty3, 0, 1000, 0, period_scaled);
  sprintf(buffer_printout, "period: %d , Duty1: %d, Duty2: %d, Duty3: %d \r", period_scaled, duty_scaled1,  duty_scaled2, duty_scaled3);
  Serial.print(buffer_printout);
  delay(500);
}

changing frequency works fine.
Serial.print is just so I can see the current values of potentiometers - works fine.
PROBLEM:
Every LED should be working individually, but this is not the case.
LED1 (with duty1) works as it should.
LED2 (with duty2) works properly if LED3 is turned off (duty3 = 0).
LED3 (with duty3) only works properly if LED1 and LED2 are turned off (duty1,duty2 = 0).
There might be other scenarios I didn’t notice.

Changing pins (D2-D12) didn’t change anything.
I would be pleased if anyone could help me to individually change duty cycle of LEDs, even if there is a different way of doing it.
One of my professors helped me write this code, but now because of quarantine he isn’t able to help me much.

One more thing- when I used the same program but for 2 LEDs it worked as it should.

When you write a program that uses registers you have to include a commentary that explains very clearly what every part does. Nobody has the complete register spec for an Atmega328 in his head.

Maybe another way of saying that is - don’t write a program using registers unless you know how to debug it, or at least how to isolate the problem to a specific point.

…R

There are many problems with that code and I'm not surprised that it doesn't work.

For starters, variables shared with interrupt routines must be declared volatile.

Second, multibyte shared variables must be protected from corruption, if they are to be accessed by both the interrupt and the main program. Do this in the main program by turning the interrupts off, making a copy of the variable, interrupts on, then use the copy.

There is no need for interrupts, just control individual PWM outputs.

There are many things wrong with your sketch, but the main reason you are seeing the strange performance is that the sprintf buffer is not large enough with three digit values, and you are overwriting the array bounds.

Try char buffer_printout[55];

There is no PB7 broken out in the nano, and the ide does not actually recognize what you are doing with that pin.

I would turn all the leds, on when counter == 0 and only one statement to reset the counter.

As jremmington says, make the isr variables volatile. Some of the variables in the sketch make no sense, and some are not used. Here is a simplified version of your sketch which I tested with fixed values on potentiometer readings.

volatile int period_scaled = 0;
volatile int counter = -1;
volatile int width1 = 0, width2 = 0, width3 = 0;
int duty1 = 0, duty2 = 0, duty3 = 0;
int period = 0;

char buffer_printout[55];


ISR(TIMER1_OVF_vect) {
  counter++;
  if (counter == 0) {
    digitalWrite(2, HIGH);
    digitalWrite(3, HIGH);
    digitalWrite(4, HIGH);
  }
  if (counter >= width1) {
    digitalWrite(2, LOW);
  }
  if (counter >= width2) {
    digitalWrite(3, LOW);
  }
  if (counter >= width3) {
    digitalWrite(4, LOW);
  }

  if (counter >= period_scaled) {
    counter = -1;
  }
  TCNT1 = 64910;
}

void timer1_init() {
  cli();
  TCCR1A = 0;
  TCCR1B = 0;
  TCCR1B |= 3;//prescaler 64
  TCNT1 = 64910;//65535 - 64910 = 625 ticks to TOP gives 2.5ms between overflow interrupts
  TIMSK1 |= (1 << TOIE1);
  sei();
}

void setup() {
  pinMode(2, OUTPUT);         //LED1
  pinMode(3, OUTPUT);        //LED2
  pinMode(4,OUTPUT);      //LED3
  timer1_init();
  Serial.begin(9600);
  Serial.println("ok");
}

void loop() {

  period = 500;//analogRead(A0);
  period_scaled = map(period, 0, 1023, 0, 1000); //overflow counts for period, 0 to 2.5 seconds
  duty1 = 100;//analogRead(A1);
  duty2 = 200;//analogRead(A2);
  duty3 = 300;//analogRead(A3);
  duty1 = constrain(duty1, 0, 1000);
  duty2 = constrain(duty2, 0, 1000);
  duty3 = constrain(duty3, 0, 1000);
  width1 = map(duty1, 0, 1000, 0, period_scaled);//turn off count
  width2 = map(duty2, 0, 1000, 0, period_scaled);
  width3 = map(duty3, 0, 1000, 0, period_scaled);
  sprintf(buffer_printout, "period: %d , Duty1: %d, Duty2: %d, Duty3: %d \r", period_scaled, duty1, duty2, duty3);
  Serial.println(buffer_printout);
  delay(500);
}

cattledog:
There are many things wrong with your sketch, but the main reason you are seeing the strange performance is that the sprintf buffer is not large enough with three digit values, and you are overwriting the array bounds.

Try char buffer_printout[55];

I just changed buffer_printout[50] to buffer_printout[55] and the program work as it should.
I didn't know this could be affecting the program. If you type "spinning LED ball" in Youtube, is what I'm working on.
Thank you very much, cattledog !

Do you know that PB7 is a crystal pin, ATmega328 chip pin 10. So wha-aye is this for?

  if (counter == 0) {
    PORTB |= (1 << PB7);
    digitalWrite(4, HIGH);
  }

You could write this sketch way shorter if you learn about arrays.

I didn't know this could be affecting the program.

When you write to memory that is being used for another purpose then expect it to affect the program.

Consider using snprintf() instead of sprintf(). It will not prevent the problem but at least if forces to think about it and if you make the size parameter a variable calculated by

sizeof(buffer_printout) - 1

then you will not write beyond the buffer