Trouble integrating SD libary

Projects Programming
1

Hello. I am a mobile developer with some years of experience, but this is my first project on Arduino, or on any microcontroller for that matter, so please assume some naïveté on matters of resources and c++.

I am building a garden irrigation controller. The idea is to have a headless controller that can be programmed via BLE by a mobile app. I am using an Arduino Uno with an AZ Delivery SD shield (a clone of the popular Adafruit SD shield with rtc clock). I am using a DSD Tech HM-19 for BLE; I will add relays for switching water valves on and off, for now I am testing with LEDs.
I am working with the Platform.io IDE on Visual Studio Code.

The mobile app sends programs as simple strings, which are parsed into objects on reception (Arduino side). Any WateringProgram has a number of WateringPhases. Programs are launched at a given time. When completed, a record is saved on the SD card, so I can schedule programs with a repetition interval of days, even if Arduino should restart.

I have solved all the basic problems (program parsing, using RTC, launch on a given time, logging on as card, ble communication); now I am integrating all the building blocks and I am seeing some strange behavior.

After this long premise, here is what happens.

The parsePrograms function parses a string into an array of WateringProgram objects, and logs the result on the serial. It has been working reliably so far.

void parsePrograms() {

  CanvasParser parser = CanvasParser(canvas);
  parser.parsePrograms(programsCount, programs);

  Serial.print("\nThere are ");
  Serial.print(programsCount);
  Serial.println(" programs");

  for (int i = 0; i < programsCount; i++) {
      WateringProgram prog = programs[i];
      prog.prettyPrint();
  }
  Serial.println("parseProgram ended");
}

Now I am trying to integrate the SD library. If I do this:

    if (!SD.begin(chipSelect)) {

the parse program function stops working; in the log I get the correct amount of programs, but phases and other program properties are empty.

Although I am new to microcontrollers, I have debugging skills, so I tried to isolate and understand the problem. To be on the safe side I commented out all loop(), and just tested calling parsePrograms() and SD.begin on the setup function. I found out that the order does not seem to matter; if I call SD.begin, parsePrograms logs programs without phases. I also have tried adding delays, no changes anyway.

I guess this buggy behavior has something to do with memory allocation, but honestly I don't know what to try next. Any suggestion?

2 
Serial.println(F("parseProgram ended"));
}

and so on everywhere you have a string literal in a print.

3

The SD Card uses the SPI pins: 11, 12, and 13 (on an UNO, Mini, or Nano). Are you sure that you aren't using any of those pins for any other purpose? Note: Pin 13 is the LED_BUILTIN pin. I'd be able to tell if I could see your sketch from here.

4

TheMemberFormerlyKNownAsAWOL: I've read about that, that is for storing the string on flash, rather than the heap, right? Could that be the source of my troubles or are you just suggesting some improvements?
(BTW: my first implementation relied heavily on String (capital S), but I am gradually migrating all my code to using char[], after reading some good tutorials)

John, the SD shield I am using should employ pin 10 (this is the value of the chipSelect constant).
I am using some of those pins, but for now I commented out everything apart from the issue I am debugging.
At any date, good suggestion, I'll double check I have commented out my pin code.

Thanks,
Davide

5

I've read about that, that is for storing the string on flash, rather than the heap, right?

Leaving the string in flash, but it never goes on the heap.

6

What does the compiler have to say about how much memory you're using?

7

When I call the SD.begin function I get:

RAM:   [========  ]  78.8% (used 1614 bytes from 2048 bytes)
Flash: [=====     ]  48.2% (used 15550 bytes from 32256 bytes)

If I comment SD.begin out I get:

RAM:   [=====     ]  48.4% (used 992 bytes from 2048 bytes)
Flash: [===       ]  32.3% (used 10418 bytes from 32256 bytes)

To give you more context, the SD.begin call is inside a class I called SDManager, which I developed and tested in another project, where it works without a glitch. To make sure I am not doing anything extra, my SDManager class is all commented out, except for the part that calls SD.begin. I also removed the whole class, and put SD.begin directly in the setup loop, the result is the same.
Another info.
The logging function I call is this one:

void WateringProgram::prettyPrint() {
  String result = String(F("# Program "));

  result = result + String(uid);
  char startTime[5];
  getStartHourMinutes(startTime);
  result = result + String(F(" - ")) + String(startTime) + " " + String(F("every ")) + String(interval) + String(F(" days")) ;
  
  Serial.println(result);
  for (int i = 0; i < phasesCount; i++)
  {
    _phases[i].prettyPrint();
  }
  Serial.print(F("\n"));
}

void WateringPhase::prettyPrint() {
  char outputChar[40];  
  sprintf(outputChar, "Ph %02d; %02d:%02d - d: %02d, ch: %02d", index, _startHour, _startMinute, _duration, _channel);
  Serial.println(outputChar); 
}

When SD.begin is called, either before of after, this functions prints an empty string.
If I remove the call from SD begin, the function works as expected, printing

# Program 1 - 16:30 every 2 days
Ph 00; 16:30 - d: 03, ch: 01
Ph 01; 16:33 - d: 04, ch: 02
Ph 02; 16:37 - d: 01, ch: 03
8

BTW, the WateringProgram prettyPrint uses String, I am in the process of removing String from my codebase, that part is no done yet.

9

A (shocking!) update.
I rewrote the WateringProgram::prettyPrint() function to use char[] instead of String.
Now it prints its output, also with SD.Begin.
Yet...

See the implementation:

void WateringProgram::prettyPrint() {
  String unused; // If I remove it, it all fails...

  char startTime[5];
  getStartHourMinutes(startTime);
  
  char outputChar[40];  
  sprintf(outputChar, "# Program %d - %s every %d days", uid, startTime, interval);
  Serial.println(outputChar);
  for (int i = 0; i < phasesCount; i++)
  {
    _phases[i].prettyPrint();
  }
  Serial.print(F("\n"));
}

You see the first variable, "unused" ? If I remove it, the function doesn't print anything.

I have a strong feeling these weird behaviors are memory related; or might it be some wing settings of my compiler? Baud rates are set allright...

10

My post got eaten. String objects are a bad idea when you have little RAM. I suspect that's what bit you.

11

nutsmuggler:
See the implementation:

void WateringProgram::prettyPrint() {

String unused; // If I remove it, it all fails...

char startTime[5];
 getStartHourMinutes(startTime);
 
 char outputChar[40];  
 sprintf(outputChar, "# Program %d - %s every %d days", uid, startTime, interval);
 Serial.println(outputChar);
 for (int i = 0; i < phasesCount; i++)
 {
   _phases[i].prettyPrint();
 }
 Serial.print(F("\n"));
}




You see the first variable, "unused" ? If I remove it, the function doesn't print anything.

Baud rates and compilers aside, as that is likely alright, you may have a buffer overrun because of your novice handling of strings. Null termination is critical, paramount, imperative, and you will likely avoid problems with it if you use the c standard string library. If you forget a Null termination, under odd conditions, like an unused variable, the adjacent null will prevent the overrun from causing problems, but then you remove that odd condition, and it fails to work.... of course, if you posted your complete code, or enough code to completely present your problem, we could better identify the problem.

12

Thank you all folks.
I guess Perehama's response makes a lot of sense, this unused variable probably occupies the area where a null-termination should be, hence the mess.
I didn't post the whole project because there are a lot of classes around; probably a rookie mistake but I am used to work with systems where memory is cheap and abundant...
At any rate, I am already migrating to c strings and libraries from the infamous capital S class; I will complete the migration for the sake of robustness, and keep an eye on null termination.
I'll let you know if I manage to pinpoint where things went wrong exactly.
Thanks again for now!

13

Hello folks,
I am still in fighting the beast, random data are wrong, and everything changes if I add a simple line of code anywhere. Unpredictably.
I have been looking closely at my usage of chars, \0, etc but I didn't find anything that seems wrong.

Maybe I should make things simpler; I have a general question.

Being a novice I tackled this project with my baggage of mobile developer.
I represented programs with a WateringPrograms class, which has a number of WateringPhases.
These are parsed from a string and then stored into memory.
Most of my bugs seem related to these programs.

Was I wrong using classes for storing model data on a microcontroller?
Should make the structure MUCH simpler, and just store an array of "commands" structs?
I can store my source string on the SD card, and keep in memory only an array of structs containing just int values.
Is this approach more sensible, given the memory constrains?

Cheers,
Davide

14

nutsmuggler:
Was I wrong using classes for storing model data on a microcontroller?
Should make the structure MUCH simpler, and just store an array of "commands" structs?

No reason to avoid classes if they make sense. The standard libraries provided use them extensively.

It does seem that you have memory issues, so it may help to come up with a more compact representation of your data. However, I suspect that you've still got issues with your parser mishandling RAM. Post (or attach) your code.

15

Here is the code. Sorry in advance, it's a handful.

Here are some helper functions:

/*
  CharUtilities.cpp - Library working with char arrays.
  Created by Davide Benini, 12/06/2020.
  MIT license.
*/
#include "CharUtilities.h"

bool charsAreEqual(char *firstInput, char *secondInput) {
  for(int i=0;firstInput[i]!='\0';i++){
        if(firstInput[i]!=secondInput[i])
            return false;
    }
    return true;
}

void copyChars(char *input, char *output) {
  for(int i=0;input[i]!='\0';i++){
    output[i] = input[i];
  }
}


int splittedCharsCount(char data[], char separator) {
    int found = 0;
    int maxIndex = strlen(data) - 1;

    for(int i=0; i<=maxIndex; i++){
        char thisChar = data[i];

        if(thisChar == separator || i==maxIndex){
            found++; 
        }
    }

    return found;
}


void splittedChars(char input[], char separator, int index, char* result) {
    int tokenCounts = 0;
    char* copiedInput = strdup(input);
    char *p = strtok (copiedInput, &separator);
    char *array[10];

    while (p != NULL) {
        array[tokenCounts] = p;
        p = strtok (NULL, &separator);
        tokenCounts++;
     }
     strcpy(result, array[index]);
     free(copiedInput);
}

void substring(char s[], char sub[], int p, int l) {
   int c = 0;
   
   while (c < l) {
      sub[c] = s[p+c-1];
      c++;
   }
   sub[c] = '\0';
}

And here's the CanvasParser class:

/*
  CanvasParser.cpp - Library for parsing strings 
  into watering programs.
  Created by Davide Benini, 12/06/2020.
  MIT license.
*/
#include "CanvasParser.h"
#include "Arduino.h"
#include <CharUtilities.h>

const int MAX_CANVAS_LENGTH = 200;
const int MAX_LINE_LENGTH = 40;
const int MAX_NUMBER_OF_LINES = 20;

char uidField = 'U';
char hourField = 'H';
char phaseField = 'C';
char intervalField = 'I';


CanvasParser::CanvasParser(char aCanvas[]) {
    _canvas = aCanvas;
}
CanvasParser& CanvasParser::operator=(const CanvasParser source) {
    _canvas = source._canvas;
    return *this;
}

int CanvasParser::programsCount() {
    return splittedCharsCount(_canvas, '|');
}

void CanvasParser::parsePrograms(int &count, WateringProgram *programs) {

  //return;
  count = programsCount();

  for (int i = 0; i < count; i++) {
    WateringProgram prog = getProgram(i);
    programs[i] = prog;
  }
}
WateringProgram CanvasParser::getProgram(int index) {
    char result[100];
    splittedChars(_canvas,'|', index, result);
    WateringProgram program = parseSingleProgram(result);
    return program;
}

WateringProgram CanvasParser::parseSingleProgram(char singleProgramCanvas[]) {

    WateringPhase phases[5];

    int uid = 0;
    int programHour = 0;
    int programMinute = 0;
    int hour = 0;
    int minutes = 0;
    int interval = 1;
    int phaseIndex = 0;

    int itemCount = splittedCharsCount(singleProgramCanvas,'\n');

    for(int i = 0; i < itemCount; i++){
        char item[10];

        splittedChars(singleProgramCanvas,'\n',i, item);
        int length = strlen(item);
        char field = item[0];

        char value[length];
        substring(item,value,2,(length-1));

        // UID
        //
        if(field == uidField) {
            uid = atoi(value);
        }

        // Hour and minutes
        //
        if(field == hourField) {
            char hString[2];
            splittedChars(value,':',0, hString);
            char mString[2];
            splittedChars(value,':',1, mString);
            hour = atoi(hString);
            minutes = atoi(mString);
            programHour = hour;
            programMinute = minutes;
        }
        

        // Interval
        if(field == intervalField) {
            interval = atoi(value);
        }
        
        // Phases
        //
        if(field == phaseField) {

            char channelValueChar[2];
            splittedChars(value,' ',0, channelValueChar);
            int channelNumber = atoi(channelValueChar);

            char durationFieldValueChar[3];
            splittedChars(value,' ',1, durationFieldValueChar);
            char *durationValueChar = durationFieldValueChar + 1;
            int duration = atoi(durationValueChar);
      
            WateringPhase phase = WateringPhase(phaseIndex, hour, minutes, duration, channelNumber);
         
            phases[phaseIndex] = phase;

            phaseIndex++;
            
            minutes += duration;
            if(minutes >= 60) {
                int extraHour = minutes / 60;
                hour += extraHour;
                minutes = minutes % 60;
            } 
            if(hour >= 24) {
                hour = hour - 24;
            }
            
        }
        
        
        
    }
    WateringProgram prog = WateringProgram(uid, programHour,programMinute, interval, phases, phaseIndex);
    return prog;
}

Cheers,
Davide

16

nutsmuggler:
Here are some helper functions:

Here are simpler functions:

#include <stdio.h>
#include <string.h>

bool charsAreEqual(char *firstInput, char *secondInput) {
  if (strncmp (firstInput,secondInput,sizeof(firstInput)) == 0) return true;
  return false;
}

void copyChars(char *input, char *output) {
  strncpy ( output, input, sizeof(output));
}

Notice how your functions end up being wrappers around standard string functions? Also, using n-restricted string functions in conjunction with sizeof(buffer) will prevent you from overrunning your buffer.

17

Just to eliminate doubt, I suggest replacing as many of your utility functions as you can, with the ones found in the C standard library.

charsAreEqual() = strcmp()
copyChars() = strcpy()

You have functions like substring() that use parameters for array indexing and have no bounds checking. I didn't look in the places where you're calling them, but it is a risky business.

18 
void copyChars(char *input, char *output) {
  strncpy ( output, input, sizeof(output));
}

Oops

19

TheMemberFormerlyKnownAsAWOL:

void copyChars(char *input, char *output) {

strncpy ( output, input, sizeof(output));
}



Oops

I don't see anything wrong with that, except that I deliberately copy up to the size of output -1 instead of the input, because it's more important to not overwrite what I am writing to.

20

And the size of "output" is ...?