Project Guidance and help cleaning up code

Long story short I'm a plc programmer (ladder logic, function block) just learning arduino interface and structured text( C, C++ coding). I read the forum rules and hope I post everything correct. sorry if I made a mistake. Anyways I'm setting up a datalogger for some analog devices and using a flow meter to calculate flow and totalize it and send it to a sd card.

I am using a atlas scientific ez flow totalizer and a random flow meter that uses pulses. The analog devices our a transducer, voltage monitor and a amp reading. So my problem is I don't know how to properly write the code and I would like some pointers to make my code more efficient. Also when I incorporate my flow text into program my data log file keeps having in error pop up.

I would appreciate any pointers people have to keep me from scratching my head, also if you see any long term data logging problems. I'm hoping to incorporate a 2nd flow meter at some point. I also have a data log problem, when it goes from to day 9 to day 10 of month the 0 in the ten is causing my log files to not file chronologically.

Thanks for any input.

float value1 = 5;
float value2;
float sensorPin = A0;
float analogPin0 = 0;
float sensorPin1 = A1;
float analogPin1 = 0;
float analog0AVG = 0;
float analog1AVG = 0;
float analog2AVG = 0;
float sensorPin2 = A2;
float analogPin2 = 0;
#include <SPI.h>
#include <SD.h>

const int chipSelect = 10;
// Date and time functions using a DS1307 RTC connected via I2C and Wire lib
#include <Wire.h>
#include "RTClib.h"

RTC_PCF8523 rtc;

char daysOfTheWeek[7][12] = {"Sunday", "Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday"};
unsigned long LOG_INTERVAL = 15000;
unsigned long previousMillis = 0;

#include <SoftwareSerial.h>      //we have to include the SoftwareSerial library, or else we can't use it.  
#define rx 2                     //define what pin rx is going to be.
#define tx 3                     //define what pin tx is going to be.

SoftwareSerial myserial(rx, tx); //define how the soft serial port is going to work.



char sensordata[30];                //we make a 48 byte character array to hold incoming data from the Flow Meter Totalizer.  
char sensordatajunk[30];
byte received_from_computer=0;     //we need to know how many characters have been received.                                 
byte sensor_bytes_received=0;       //we need to know how many characters have been received.
byte string_received=0;            //used to identify when we have received a string from the Flow Meter Totalizer.
float f;
char *flotot;
void setup() {
  
  while (!Serial) {
    delay(1);  // for Leonardo/Micro/Zero
  }

  Serial.begin(9600);
  if (! rtc.begin()) {
    Serial.println("Couldn't find RTC");
    while (1);
  }

  if (! rtc.initialized()) {
    Serial.println("RTC is NOT running!");
    // following line sets the RTC to the date & time this sketch was compiled
    rtc.adjust(DateTime(F(__DATE__), F(__TIME__)));
    // This line sets the RTC with an explicit date & time, for example to set
    // January 21, 2014 at 3am you would call:
    rtc.adjust(DateTime(2019, 2, 26, 14, 26, 0));
  }
  // put your setup code here, to run once:


  while (!Serial) {
    ; // wait for serial port to connect. Needed for native USB port only
  }


  Serial.print("Initializing SD card...");
delay(1000);
  // see if the card is present and can be initialized:
  if (!SD.begin(chipSelect)) {
    Serial.println("Card failed, or not present");
    // don't do anything more:
    return;
  }
  Serial.println("card initialized.");
  myserial.begin(9600);        //enable the software serial port
}

void loop() {
  // put your main code here, to run repeatedly:
  unsigned long currentMillis = millis();
 
  if (currentMillis - previousMillis >= LOG_INTERVAL) {
    previousMillis = currentMillis;
      

    // create a filename

    DateTime now;
    now = rtc.now();
    int YR = now.year();
    String YRs = String(YR, DEC);
    int MO = now.month();
    String MOs = String(MO, DEC);
    int DY = now.day();
    String DYs = String(DY, DEC);
    String DATE = YRs + MOs + DYs;
    String filename = DATE + ".txt";
   
    String dataString = "";
    dataString += String(now.year(), DEC); dataString += " / ";
    dataString += String(now.month(), DEC); dataString += " / ";
    dataString += String(now.day(), DEC); dataString += " ( ";
    dataString += String(now.hour(), DEC); dataString += " - ";
    dataString += String(now.minute(), DEC); dataString += " . ";
    dataString += String(now.second(), DEC); dataString += " ) ";
    dataString += String(analog0AVG); dataString += "= Volts, ";
    dataString += String(analog1AVG); dataString += "= Amps, ";
    dataString += String(analog2AVG); dataString += "= Level ";
    
    File dataFile = SD.open(filename, FILE_WRITE);

   
    if (dataFile) {
      dataFile.println(dataString);
      dataFile.close();

    }
   
    else {
      Serial.println("error opening datalog.txt");
    }
    {
      value2 = value1 ++;
      analogPin0 = analogRead(sensorPin);
      analogPin1 = analogRead(sensorPin1);
      analogPin2 = analogRead(sensorPin2);
      analog0AVG = analogPin0 / 1024 * 150;
      analog1AVG = analogPin1 / 1024 * 20;
      analog2AVG = analogPin2 / 1024 * 34.605;
      Serial.print("Level=");
      Serial.print(analog2AVG);
      Serial.print("\tFlow Total=");
      Serial.print(f);
      Serial.print("\tAmps=");
      Serial.print(analog1AVG);
      Serial.print("\tVolts=");
      Serial.println(analog0AVG);

    }
    Serial.print(now.year(), DEC);
    Serial.print('/');
    Serial.print(now.month(), DEC);
    Serial.print('/');
    Serial.print(now.day(), DEC);
    Serial.print(" (");
    Serial.print(daysOfTheWeek[now.dayOfTheWeek()]);
    Serial.print(") ");
    Serial.print(now.hour(), DEC);
    Serial.print(':');
    Serial.print(now.minute(), DEC);
    Serial.print(':');
    Serial.print(now.second(), DEC);
    Serial.println();
  }
  flow();
    delay(200);
}
void flow(){//reads all flows, both instantaneous and totals

    myserial.print("R");                       
    myserial.print("\r");                      
    delay(200);
    
    if (myserial.available() > 0) {                 //If data has been transmitted from an Atlas Scientific device
      sensor_bytes_received = myserial.readBytesUntil(13, sensordata, 30); //we read the data sent from the Atlas Scientific device until we see a <CR>. We also count how many character have been received
      sensordata[sensor_bytes_received] = 0;         
      flotot = strtok(sensordata, ",");          
      f=atof(flotot);
      delay(10);
      if (myserial.available() > 0) {                 //If data has been transmitted from an Atlas Scientific device
      int junk = myserial.readBytesUntil(13, sensordatajunk, 30);}
      delay(10);
    }
}

You appear to have forgotten to use Tools->Auto Format before Edit->Copy for Forum when posting your sketch.

Serial.begin(); goes BEFORE "while (!Serial)". And you don't need "while (!Serial)" twice.

      analogPin0 = analogRead(sensorPin);
      analogPin1 = analogRead(sensorPin1);
      analogPin2 = analogRead(sensorPin2);

It's a little odd to be reading an integer into a global 'float' variable.

      analog0AVG = analogPin0 / 1024 * 150;
      analog1AVG = analogPin1 / 1024 * 20;
      analog2AVG = analogPin2 / 1024 * 34.605;

If all of your sensors are the same, why multiply by different 'magic' (unexplained) numbers. If all f your sensors are different, why are they all named 'sensorPinX'?

Have not finished documenting code yet, but my analog sensors are for voltage 0-150, amps 0-20, level using pressure tranducer 0-34.065. That's what the conversions are for and I overlooked the serial begin.
I guess I should put the global variables into the local variables( correct)? instead.

Or do you mean to change it to a integer instead of a floating point value.

float sensorPin = A0;
float analogPin0 = 0;
float sensorPin1 = A1;
float analogPin1 = 0;

Why floats for pin numbers?

I changed them to integers,
I'm used to having plenty of memory so never been a factor, but correct I dont need floats and I have made the correction.

How about some sensible names?

If sensorPin is a pin number, it kinda looks like
analogPin0 is a pin number too.

I see what you mean I added a step that i didn't need. Do you see a issue with the datalogger. should i move it from the loop and add a interrupt.

JamesAK2019:
Do you see a issue with the datalogger. should i move it from the loop and add a interrupt.

An interrupt is unnecessary. Leave the timer parts in loop() and move the body to a function, like "log()".

Hi,
What model Arduino are you using?

Just a suggestion for variable names;

int voltagePin = A0;
int ampsPin = A1;
int pressurePin = A2;
int rawVolts;
int rawAmps;
int rawPressure;
float voltsVal;
float ampsVal;
float pressureVal;

void setup()
{
  pinMode (voltagePin, INPUT);
  pinMode(ampsPin, INPUT);
  pinMode(pressurePin, INPUT);
}

void loop()
{
  rawVolts = analogRead(voltagePin);
  rawAmps = analogRead(ampsPin);
  rawPressure = analogRead(pressurePin);
  voltsVal = (float)rawVolts / 1024.0 * 150.0; // Voltage Volts
  ampsVal = (float)rawAmps / 1024.0 * 20.0;    // Current Amps
  pressureVal = (float)rawPressure / 1024.0 * 34.605;  // Pressure  kPa
}

Tom... :slight_smile:

Hi Tom,

I'm using the Uno and I appreciate your input and will definitely incorporate your variable names. Much better then my generic ones. I found I have a memory issue when sd card writes so will have to delete all unnecessary bytes or get rid of string?.

Hi,

I'm not sure but this may help;

Tom... :slight_smile:

Serial.println("Couldn't find RTC");

Certainly needs more F().

Here is a partial clean-up of the code. Mostly: meaningful names, make variables local if that don't have to be global, move logging into a function, naming many of the constants.

const float VoltageFullScale = 150.0; // Volts
const byte  VoltageSensorPin = A0;
float VoltageReading;

const float CurrentFullScale = 20.0; // Amps
const byte  CurrentSensorPin = A1;
float CurrentReading;

const float PressureFullScale = 34.605; //  ?
const byte PressureSensorPin = A2;
float PressureReading;

#include <SPI.h>
#include <SD.h>

const int SDChipSelect = 10;
// Date and time functions using a DS1307 RTC connected via I2C and Wire lib
#include <Wire.h>
#include "RTClib.h"

// I do't have that version of RTCLIB
// RTC_PCF8523 rtc;
DS1307 rtc;

const char *daysOfTheWeek[7] = {"Sunday", "Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday"};

const unsigned long LogInterval = 15000;


#include <SoftwareSerial.h>      //we have to include the SoftwareSerial library, or else we can't use it.  
#define rx 2                     //define what pin rx is going to be.
#define tx 3                     //define what pin tx is going to be.
SoftwareSerial AtlasFlowSensor(rx, tx); //define how the soft serial port is going to work.

void setup()
{
  Serial.begin(115200);
  while (!Serial)
  {
    delay(1);  // for Leonardo/Micro/Zero
  }

  if (! rtc.begin())
  {
    Serial.println("Couldn't find RTC");
    while (1);
  }

  // I don't have that version of RTCLIB

  //if (! rtc.initialized())
  if (! rtc.isrunning())
  {
    Serial.println("RTC is NOT running!");
    // following line sets the RTC to the date & time this sketch was compiled
    rtc.adjust(DateTime(F(__DATE__), F(__TIME__)));
    // This line sets the RTC with an explicit date & time, for example to set
    // January 21, 2014 at 3am you would call:
    rtc.adjust(DateTime(2019, 2, 26, 14, 26, 0));
  }

  Serial.print("Initializing SD card...");

  // see if the card is present and can be initialized:
  if (!SD.begin(SDChipSelect))
  {
    Serial.println("SD Card failed, or not present");
    // don't do anything more:
    while (1);
  }
  Serial.println("SD Card initialized.");

  AtlasFlowSensor.begin(9600);        //enable the software serial port
}

void loop()
{
  unsigned long currentMillis = millis();
  static unsigned long logIntervalStart = 0;

  VoltageReading = (analogRead(VoltageSensorPin) * VoltageFullScale) / 1024.0;
  CurrentReading = (analogRead(CurrentSensorPin) * CurrentFullScale) / 1024.0;
  PressureReading = (analogRead(PressureSensorPin) * PressureFullScale) / 1024.0;
  float f = flow();

  Serial.print("Level=");
  Serial.print(PressureReading);
  Serial.print("\tFlow Total=");
  Serial.print(f);
  Serial.print("\tAmps=");
  Serial.print(CurrentReading);
  Serial.print("\tVolts=");
  Serial.println(VoltageReading);

  // At intervals, log everything to the SD card.
  if (currentMillis - logIntervalStart >= LogInterval)
  {
    logIntervalStart += LogInterval;
    Log();
  }
}

void Log()
{
  // create a filename

  DateTime now;
  now = rtc.now();
  int YR = now.year();
  String YRs = String(YR, DEC);
  int MO = now.month();
  String MOs = String(MO, DEC);
  int DY = now.day();
  String DYs = String(DY, DEC);
  String DATE = YRs + MOs + DYs;
  String filename = DATE + ".txt";

  String dataString = "";
  dataString += String(now.year(), DEC); dataString += " / ";
  dataString += String(now.month(), DEC); dataString += " / ";
  dataString += String(now.day(), DEC); dataString += " ( ";
  dataString += String(now.hour(), DEC); dataString += " - ";
  dataString += String(now.minute(), DEC); dataString += " . ";
  dataString += String(now.second(), DEC); dataString += " ) ";
  dataString += String(VoltageReading); dataString += "= Volts, ";
  dataString += String(CurrentReading); dataString += "= Amps, ";
  dataString += String(PressureReading); dataString += "= Level ";

  File dataFile = SD.open(filename, FILE_WRITE);

  if (dataFile)
  {
    dataFile.println(dataString);
    dataFile.close();
  }
  else
  {
    Serial.println("error opening datalog.txt");
  }

  Serial.print(now.year(), DEC);
  Serial.print('/');
  Serial.print(now.month(), DEC);
  Serial.print('/');
  Serial.print(now.day(), DEC);
  Serial.print(" (");
//  Serial.print(daysOfTheWeek[now.dayOfTheWeek()]);
  Serial.print(") ");
  Serial.print(now.hour(), DEC);
  Serial.print(':');
  Serial.print(now.minute(), DEC);
  Serial.print(':');
  Serial.print(now.second(), DEC);
  Serial.println();
}

// Read data from Atlas flow sensor
float flow() //reads all flows, both instantaneous and totals
{
  static float f = 0.0;

  // Request a reading from the Atlas Scientific flow sensor
  AtlasFlowSensor.print("R");
  AtlasFlowSensor.print("\r");
  delay(200);

  // If characters have started arriving
  if (AtlasFlowSensor.available()) 
  {
    char flowData[31];
    // Read characters from the Atlas Scientific flow sensor until we see a comma. 
    // Receive a count of how many character have been received
    int sensor_bytes_received = AtlasFlowSensor.readBytesUntil(',', flowData, 30); 
    flowData[sensor_bytes_received] = '\0';  // Add null terminator
    f = atof(flowData);  // Convert to a floating-point number

    // Flush out the rest of the input
    delay(10);
    while (AtlasFlowSensor.available())
    {
      AtlasFlowSensor.read();
    }
    delay(10);
  }
  return f;
}

Impressive John reading through your code makes me wonder what I was thinking and using a static for flow, I should have done that and reading through your commenting makes it much easier to read.