Good code?

I am working on a project and am having intermittent issues with my system state, it changes when I do not expect it. I have 1 button, 2 LEDs and am using a card reader shield. I know it is difficult for you to help without seeing the connections on the shield, but I plan on trying to remove the shield and breadboard the project to verify it is not a solder/connection issue.

What I am really looking for at this point is someone to look at my code and tell me if it is terrible. It is supposed to hold code until button press. When pressed the ledReady LED should light up and then switch b/t states of Busy (record) and Ready (waiting) with each consecutive button press.

My observation is that it jumps out of the record loop unexpectedly. This even occurred when I replaced the while condition with “1”.

Thx!

#include <SD.h>
#include <Wire.h>
#include "RTClib.h"

#define LOG_INTERVAL    1000
#define SYNC_INTERVAL   1000

#define aref_voltage 3.3

RTC_DS1307 RTC;
const int chipSelect = 10;

uint32_t syncTime = 0; //time of last sync


const int buttonPin = 2;
const int ledBusy = 1;
const int ledReady = 0;
int stateChange = 0;
int temp[] = {0, 0, 0, 0, 0};

File logfile;

void error()
{
  digitalWrite(ledReady, HIGH);
  digitalWrite(ledBusy, HIGH);
  while(1);
}

void setup(void) {
  analogReference(EXTERNAL);

  pinMode(ledBusy, OUTPUT);
  pinMode(ledReady, OUTPUT);
  pinMode(buttonPin, INPUT);
  pinMode(10, OUTPUT);
  
  digitalWrite(ledReady, LOW);
  digitalWrite(ledBusy, LOW);
  

  if (!SD.begin(chipSelect)) {
    error();
  }

  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)) {
      logfile = SD.open(filename, FILE_WRITE);
      break; //leave the loop
    }
  }
  
  if (! logfile) {
    error();
  }
    
  //connect to RTC
  Wire.begin();
  if (!RTC.begin()) {
    logfile.println("RTC failed");
  }
  
  logfile.println("millis,stamp,datetime,Temp0 V,Temp0 (F),Temp1 V, Temp1 (F),Temp2 V, Temp2 (F),Temp3 V, Temp3 (F),Temp4 V,Temp4 (F)");
}
  

void loop(void)
{
  
  while (stateChange == LOW){
    stateChange = digitalRead(buttonPin);
  }
  
  delay(450);
  stateChange = LOW;
  digitalWrite(ledReady, HIGH);
  
  while (1){
    stateChange = digitalRead(buttonPin);
    
    if (stateChange == HIGH){
      delay(450);
      stateChange = LOW;
      record();
      digitalWrite(ledReady, HIGH);
      digitalWrite(ledBusy, LOW);
    }
  }
}
      
void record(){
  digitalWrite(ledReady, LOW);
  digitalWrite(ledBusy, HIGH);
  
  while(stateChange == LOW){
    DateTime now;
    
    //delay for the amount of time we want between readings
    delay((LOG_INTERVAL -1) - (millis() % LOG_INTERVAL));
    
    //log millis since starting
    uint32_t m = millis();
    logfile.print(m);
    logfile.print(",");
 
    now = RTC.now();
    logfile.print(now.unixtime()); // seconds since 1/1/1970
    logfile.print(", ");
    logfile.print('"');
    logfile.print(now.year(), DEC);
    logfile.print("/");
    logfile.print(now.month(), DEC);
    logfile.print("/");
    logfile.print(now.day(), DEC);
    logfile.print(" ");
    logfile.print(now.hour(), DEC);
    logfile.print(":");
    logfile.print(now.minute(), DEC);
    logfile.print(":");
    logfile.print(now.second(), DEC);
    logfile.print('"');
   
    
    for (int i=0; i<5; i++) {
      temp[i] = analogRead(i);
      float voltage = temp[i]*aref_voltage/1024;
      float tempC = (voltage - 0.5)*100;
      float tempF = tempC*9/5 +32;
  
      logfile.print(",");
      logfile.print(voltage);
      logfile.print(",");
      logfile.print(tempF);
  }
    
    logfile.println();
            
    //sync to SD card!!
    if ((millis() - syncTime) < SYNC_INTERVAL) return;
    syncTime = millis();
    
    logfile.flush();        
  }
  delay(450);
  stateChange = LOW;
}

HI, can you post a circuit diagram, either CAD or pic of hand drawn, to show you have wired the input and outputs.
Are you using pullup or pull down resistors?
If you have your switch pulling the input high when it is activated, do you have a 10K resistor between the arduino input and gnd to hold the input low when the switch is open?

Tom.... :slight_smile:

+1 to Tom's comments. If you are pulling the pin high with the switch, you need to have a pull-down resistor to ground. If you're pulling it down with the switch, you either need an external pull up resistor, or use the internal pull up resistor by using

digitalWrite(buttonPin, HIGH);

after you define the pin as an input, with the command

pinMode(buttonPin, INPUT_PULLUP);

If you don't do this, the voltage on the pin will float randomly and the results of digitalRead(buttonPin) will be unpredictable.

Hopefully this is sufficient. I got the LEDs from radioshack, generic assorted. I am wondering if the SD card is getting full. After my original post I left the board on for a while and when I came back both LEDs were flashing, Pin 1 LED was brighter than Pin 2 LED.

Also, I expect Pin 0 LED to stay on while waiting, and Pin 1 LED to come on while busy. I just noticed it is working backwards. What is really strange is that even though it looks reversed, I see the activity light on the SD card shield flash when Pin 0 LED is on (should be the waiting LED).

I’m really confused and would be surprised if you can even follow what I am trying to say 'cause I am confusing myself.

Hi, do you have any resistors in series with the LEDs, if not then turn OFF.
Place a 470 Ohm in series with each LED to limit the current, this may also help your intermittent as you are loading the ardunio down. The diagram on the LED bag shows that you need to fit one.
Check the schematic in this link.

The jump out of loop is really the CPU resetting.

The 1K resistor in series with the button is not necessary either.

Tom..... :slight_smile:

Thanks for the advice. I put the 1k LED resistors in and took out the 1k in the button circuit. Still jumps out of loop. Any comments on the code? Does it look efficient and correct?

Edit:
Looks like there is a “return” for the while loop in the record function. I’m not really familiar with it as I found the SD saving procedure elsewhere (might have been in examples but I don’t recall). If the card is full would that cause the syncTime to give strange results?

Still stuck
:~

Got an answer from Adafruit. It was the conditional syncTime return statement. Solution:

    if ((millis() - syncTime) < SYNC_INTERVAL){
      syncTime = millis();
      logfile.flush();
    }

Thx for support, got good advice here as well.