Arduino Static Struct and interrupt service routine issue

I’ve got an issue with writing to a static struct from inside an ISR. I’m not sure if this is an issue with PIO or an issue with my code, but it’s been bugging me for days and I can’t work out what’s wrong.

Here’s a snippet of my code

.h file

#define MAX_CELLS 4

typedef struct {
   uint8_t VSensPin;          // Analog input to monitor battery voltage

   volatile float Capacity;
   volatile float Voltage;
   volatile float Current;
} cell_t;

class BatteryConditioner {
  public:
    BatteryConditioner(char _vsense_pin);
  private:
    uint8_t cellIndex;
};

.cpp file

uint8_t CellCount = 0;
static cell_t cells[MAX_CELLS];                            // static array of cell structures

static inline void interruptHandler(volatile uint16_t *TCNTn, volatile uint16_t* OCRnA) {
  for (auto i : cells) {
    if (i.VSensPin > 0) {
      i.Voltage = analogRead(i.VSensPin) * (5.0 / 1023.0);
      i.Current = i.Voltage /4;
      i.Capacity += (i.Current * 1000) / (3600.0 * 2);
 
      Serial.println(i.Voltage);
      Serial.println(i.Capacity);
      Serial.println();
    } else {
      break;
    }
  }

  TCNT1 = TIMER_VALUE;      // Reload timer
}

BatteryConditioner::BatteryConditioner(char _v_sens_pin) {
  this->cellIndex = CellCount++;                    // assign a cell index to this instance
  cells[this->cellIndex].VSensPin = _v_sens_pin;
}

The interruptHandler is triggered every 500ms. What should happen is that cells[n].Capacity steadily increases and all the volatile members of the structure are available from anywhere in the code. What actually happens is that any values written within the interruptHandler are lost as soon as the interruptHandler returns.

Is there a problem with the way PIO is compiling this or have I just goofed up? To my mind cells[n].Voltage, cells[n].Current and cells[n].Capacity should behave essentially as static volatile floats, however this is not the case. Can anyone shed any light on this as it’s driving me bananas?

No printf / println inside ISRs!

Oh, that’s not the actual problem, though it can become one…

With for (auto i : cells) you are looping through copies of your array elements. In your ISR, you are only changing copies. You want to get a reference and change those.

You need to have for (auto &i : cells) . In generics code you’d need auto&&.

See Auto Type Deduction in Range-Based For Loops | Petr Zemek

Huh, looking back, the other code doesn’t make much sense. In the consurctor of the BatteryConditioner you incrementally modify the CellCount-th cell ,without checking any boundaries, but in your interruptHandler you iterate over each and every cell_t object in your cells, without checking if they were already assigned a VSensPin. You might want to do a simple for loop ranging from 0 to < CellCount.

Well, don’t I feel like a doughnut! I must have re-read the InterruptHandler 100 times and I didn’t spot that I was working on copies of elements. Thanks for that, I guess that’s what happens when you hammer away at the keyboard and don’t read what you type. I see your point about incrementally modifying CellCount, this is handled in the full code. I only posted a snippet for clarity. As far as iterating over every cell_t object in cells however, I am checking if they are assigned a VSensPin.

for (auto i : cells) {
  if (i.VSensPin > 0) {
    _do stuff_
  } else {
    break;
  }
}

The analog input pins on an arduino are A0 to A5, giving a value of 14 to 19 (I think, don’t quote me) for the value stored in VSensPin. This certainly works, am I missing something?

And yes, you are quite right, an ISR is no place for Serial.print. That is only there for debugging.

Yes this is correct.

However you maybe just want to use a std::vector<cell_t> for simplicity? Then you don’t have to check whether you assigned an object’s properties already, you just iterate through a vector of objects. But that’s already going in the direction of a code review anways.

Does your code work now as expected?

Yes, it’s working perfectly. Thanks for that.

I’m not sure how i’d implement vectors for this. The library is implemented as follows

#include "BatteryConditioner.h"

BatteryConditioner batt1(A0, 2);
BatteryConditioner batt2(A1, 3);

void setup() {
  BatteryConditioner::begin();
}

void loop() {
}

I agree that vectors would make the InterruptHandler a bit neater, but I don’t know enough about vectors to implement them without push_back having to be used for every instance of the library.

Oh hang on, it would just be a few more lines of code in the constructor, wouldn’t it?