-1

I am testing on an Arduino UNO. I want to use timer1 and interrupts to create various LED patterns. The LEDs may span multiple ports i.e PORTB and PORTD. I want to use a "volatile uint8_t*" pointer to store whether to toggle a pin on PORTB or PORTD i.e have the pointer store &PORTB or &POINTD. And then use it as necessary. However, once set, I cannot seem to be able to change the pointer in the loop.

Some more details: I'm using timer1 in CTC mode (clear on compare match) and making use of both OCR1A and OCR1B interrupts. When output compare A interrupt matches, the led is turned on and a flag is set for the main loop to change leds. When output compare B interrupt matches, the led is turned off. The switch statement works and the led pins can be changed i.e next_led and current_led, however, the next_port and current_port statements are ignored in the main loop. The led pin change works when changing from case 3 to case 1 (when they are both PORTB), but when changing the next_port variable to PORTD this is completely ignored.

At the very beginning of the code, if I initialize current_port and next_port with &PORTD, then just PORTD led will light up and the main loop will not change the current_port and next_port variable to &PORTB.

I have tried using volatile uint16_t* for the next_port and current_port and that doesn't work, either.

What am I doing wrong?

int red_led = 9;    //PORTB 1
int green_led = 10; //PORTB 2

int led3 = 5;       //PORTD 5

volatile uint8_t current_led = 1; //port number of led
volatile uint8_t next_led = 1; 

volatile uint8_t *current_port = &PORTB; 
volatile uint8_t *next_port = &PORTB;    

int counter = 0; 
volatile int change_led_flag = 0;
volatile uint8_t led_state = 1;

uint16_t ocr1a_val = 10000-1; //determines period. 16MHz clock, Prescaler=8, 10000-1 ticks = 5ms period (200Hz)
uint16_t ocr1b_val = 9900-1; //determines duty cycle (99% for 10000 ticks)


void setup() {
  pinMode(red_led, OUTPUT);
  pinMode(green_led, OUTPUT);
  pinMode(led3, OUTPUT);

  TCCR1A = 0;
  TCCR1B = 0;

  //set CTC mode
  TCCR1B |= (1<<WGM12);

  //set OCR1A value
  OCR1A = ocr1a_val;
  //set OCR1B value
  OCR1B = ocr1b_val;

  //set pre-scaler to 8
  TCCR1B |= (1 << CS11);

  //enable output compare interrupts
  TIMSK1 |= (1 << OCIE1A);
  TIMSK1 |= (1 << OCIE1B);

  sei();
}

void loop() {
  if(change_led_flag == 1){
  change_led_flag = 0;
  current_led = next_led; //set the current led to what was just turned on
  current_port = next_port; //this statement doesn't do anything.
  switch (led_state) {
    case 1:
      led_state = 2;
      next_led = 2;
      next_port = &PORTB; //this statement doesn't do anything
      break;
    case 2:
      led_state = 3;
      next_led = 5;
      next_port = &PORTD; //this statement doesn't do anything
      break;
    case 3:
      led_state = 1;
      next_led = 1;
      next_port = &PORTB; //this statement doesn't do anything
      break;
    } //end switch statement
  } //end if
  
} //end loop()


//turn on next_led and set change_led_flag
ISR(TIMER1_COMPA_vect){
  *next_port |= (1 << next_led); //turn on led
  change_led_flag = 1; //set flag to update next led
}

//turn off current_led
ISR(TIMER1_COMPB_vect){
  *current_port &= ~(1 << current_led); //turn off led
}

2
  • I get the error: cannot convert 'volatile uint8_t* {aka volatile unsigned char*}' to 'volatile uint16_t* {aka volatile unsigned int*}' in initialization volatile uint16_t *next_port = &PORTB;
    – Tommy Ray
    Oct 14 at 5:06
  • 1
    Welcome to SE/Arduino! Please take the tour to learn how this site works. If you have any additional information, please edit your question, don't post a comment. It will not be read, this is not a forum. -- However, please reduce your program to the most simple example, where you change just the port pointer and use it to access the ports. -- If that does not work generally, you can fall back to use Arduino functions like digitalWrite() or use an enum and a switch, for example. Oct 14 at 6:22
0

I don't know this is your problem, but it is a problem in any case. The pointer itself, not just what it points to, would also need to be volatile qualified. That is:

volatile uint8_t *volatile current_port = &PORTB; 
volatile uint8_t *volatile next_port = &PORTB;    

The volatile after the pointer declarator (the *) determines the volatile quality of the pointer itself. The volatile that you already had prior to the declarator is just qualifying what is pointed-to, the port in this case, which is needed for similar reasons.

Without the pointer itself being volatile, you can't expect to changes to it to be reflected correctly between the ISR contexts and the normal main line of execution that runs setup, loop and and most everything else. The compiler/optimizer may assume since it can't see any way to get to code that can change the value that the value is therefore not changing. ISR dispatches are outside its field of view.

Also in the when you update next_port outside of the ISR, you probaly should make sure interrupts are off while you update it:

noInterrupts();
next_port = &PORTB;
interrupts();

Because the pointer value is a two-byte address, if you don't disable the interrupts while assigning it your ISR can be dispatched midway through the assignment when the pointer object isn't yet valid. I say should above, because in your situation the addresses that happen to be used for ports may all have the same upper byte value. It's a good habit to be in regardless.

1
  • 1
    Thank you so much! Adding the extra volatile fixed the code. (This is the first time I've used a volatile pointer that itself is volatile!) You make some good points on the non-atomic assignments. It's probably a good idea to put the next_led assignment within that no interrupt block as well, so that the ISR won't dispatch with a mismatched led and port. Thanks again!
    – Tommy Ray
    Oct 14 at 15:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.