SD Card Datalogging while running Interrupts

How's it goin?

I have a sketch that calculates the pulse width of a signal using interrupts. I have debugged this code and everything works fine. But I would also like to record the pulse width values, as well as some another analog voltage to an SD logger. I have run some example codes and I have them functioning as well.

My problem, is that when I combine the two, the pwm input signal seems to break the data logging. I have tried suspending the interrupts while logging is taking place using noInterrupts() and interrupts(), but that didn't seem to work.

Does anyone have any idea why the interrupts are messing with the data logging?

Yes it is line 83, if not post the code.

#include <TimerOne.h>        // http://playground.arduino.cc/Code/Timer1

#include <SD.h>
#include <Wire.h>
#include "RTClib.h"
RTC_DS1307 RTC; // define the Real Time Clock object
#define PWMpin 18    


#define LOG_INTERVAL  100 // mills between entries (reduce to take more/faster data)

// how many milliseconds before writing the logged data permanently to disk
// set it to the LOG_INTERVAL to write each time (safest)
// set it to 10*LOG_INTERVAL to write all data every 10 datareads, you could lose up to 
// the last 10 reads if power is lost but it uses less power and is much faster!
#define SYNC_INTERVAL 1000 // mills between calls to flush() - to write data to the card
uint32_t syncTime = 0; // time of last sync()

#define ECHO_TO_SERIAL   1 // echo data to serial port
#define WAIT_TO_START    0 // Wait for serial input in setup()

// the digital pins that connect to the LEDs
#define redLEDpin 2
#define greenLEDpin 3
#define thrustVPin 0
#define switchPin 9


// for the data logging shield, we use digital pin 10 for the SD cs line
const int chipSelect = 10;
float thrustV = 0;
int PWMsignal = 0;
bool start = false;

File logfile;

void error(char *str)
{
  Serial.print("error: ");
  Serial.println(str);
  
  // red LED indicates error
  digitalWrite(redLEDpin, HIGH);

  while(1);
}

unsigned int time = 0;      

byte state=0;
byte burp=0;    // a counter to see how many times the int has executed
byte cmd=0;     // a place to put our serial data

void setup() {
  
  Serial.begin(115200);
    
  // initialize the SD card
  Serial.print("Initializing SD card...");
  
  // make sure that the default chip select pin is set to
  // output, even if you don't use it:
  pinMode(10, OUTPUT);
  pinMode(switchPin, INPUT);
  
  if (!SD.begin(10,11,12,13)) {
    error("Card failed, or not present");
  }
  Serial.println("card initialized.");
  
  // create a new file
  char filename[] = "LOGGER00.CSV";
  for (uint8_t i = 0; i < 100; i++) {
    filename[6] = i/10 + '0';
    filename[7] = i%10 + '0';
    if (! SD.exists(filename)) {
      // only open a new file if it doesn't exist
      logfile = SD.open(filename, FILE_WRITE); 
      break;  // leave the loop!
    }
  }
  
  if (! logfile) {
    error("couldnt create file");
  }
  
  Serial.print("Logging to: ");
  Serial.println(filename); 
  
  logfile.println("Time, PWM, ThrustV");    

 
  // If you want to set the aref to something other than 5v
  analogReference(EXTERNAL);
  
    Serial.print("PinChangeInt ReciverReading test");
    Serial.println();            //warm up the serial port

    Timer1.initialize(2200);    //longest pulse in PPM is usally 2.1 milliseconds,
                                //pick a period that gives you a little headroom.
    Timer1.stop();                //stop the counter
    Timer1.restart();            //set the clock to zero

    pinMode(PWMpin, INPUT);      //set the pin to input
    digitalWrite(PWMpin, HIGH);  //use the internal pullup resistor
    

    attachInterrupt(5, rise, RISING); // attach a PinChange Interrupt to our first pin
}

void loop() {

    cmd=Serial.read();        //while you got some time gimme a systems report
    if (cmd=='p')
    {
        Serial.print(PWMsignal,DEC);
        Serial.print("\t");
        Serial.print(thrustV, 3);
        Serial.println();
    }
    
    cmd=0;
    switch (state)
    {
        case RISING: //we have just seen a rising edge
            detachInterrupt(5);
            attachInterrupt(5, fall, FALLING); //attach the falling end
            state=255;
            break;
        case FALLING: //we just saw a falling edge
            detachInterrupt(5);
            attachInterrupt(5, rise,RISING);
            state=255;
            break;
        default:
            //do nothing
            break;
    }
    
    shouldRecord();
    if (start){
      //Serial.print("Recording");
      delay((LOG_INTERVAL -1) - (millis() % LOG_INTERVAL));
      digitalWrite(greenLEDpin, HIGH);
      thrustV = analogRead(thrustVPin);
      delay(10);
      uint32_t time = millis();
      logfile.print(time);
      logfile.print(", ");
      logfile.print(PWMsignal);
      logfile.print(", ");
      logfile.println(thrustV);
    }
    else digitalWrite(greenLEDpin, LOW);
    
  if ((millis() - syncTime) < SYNC_INTERVAL) return;
     syncTime = millis();
  
  // blink LED to show we are syncing data to the card & updating FAT!
  digitalWrite(redLEDpin, HIGH);
  logfile.flush();
  digitalWrite(redLEDpin, LOW);
 
}

void shouldRecord(){
  if (digitalRead(switchPin) == LOW){
    start = true;
  } 
}

void rise()        //on the rising edge of the currently intresting pin
{
    Timer1.restart();        //set our stopwatch to 0
    Timer1.start();            //and start it up
    state=RISING;

}

void fall()        //on the falling edge of the signal
{
    state=FALLING;
    //time=readTimer1();    // read the time since timer1 was restarted
    PWMsignal=Timer1.read();    // The function below has been ported into the
                            // the latest TimerOne class, if you have the
                            // new Timer1 lib you can use this line instead
    Serial.println(time,DEC);
    Timer1.stop();
    
}

I suggest you look at lines 48 and 147 - neither of these are correct.

You also do not seem to have declared the variables you use in the interrupt routines as volatile.

I don't like the way you are attaching and detaching interrupt handlers to change how you respond to an interrupt. It would IMO be far better to have a single handler that handles all the modes you need, and just set a variable to select which mode it is in.

Global variable burp seems to be redundant, and cmd doesn't need to be global.

Most of this code I have just copy and pasted together from existing bits of code. The data logging code is an adaptation from the data logging tutorial. The pwm interrupt code is just some code I found online because the pulse in function wasn't fast enough for what I needed it for.

I haven't used interrupts before, but I understand how to use them now. But I think there is a problem with the interrupts suspending the serial stream while it is trying to log to the SD card.

If anyone knows of a better way to approach this, I'm all ears haha.

Thanks again for the feedback

Mohrad:
I haven't used interrupts before, but I understand how to use them now.

My advice is to not use interrupts at all unless it is necessary, and then only reluctantly and to the minimum extent possible.

If you want further feedback on your code, post the code with the issues that have already been pointed out fixed, and explain what (if anything) it's doing wrong.

I don't know whether the SD library disables interrupts during a write, but I suppose it's possible.

I just did a project where input characters from the USART are buffered by an ISR, and when a buffer is full, I use sdfatlib to write it to an SD card. The write to SD is done in the mainline code (i.e. not the ISR) so there is a good chance that the sdfatlib code gets interrupted, probably repeatedly. I haven't seen any problems with this arrangement.

I am trying to integrate SdFat with the Adafruit BLE library (Adafruit_BLE_UART) which uses interrupts. The BLE library seems to be blocking SdFat from running. Any insight as to making these play nice?