michalmonday / CSV-Parser-for-Arduino

It turns CSV string into an associative array (like dict in python)
MIT License
57 stars 12 forks source link

Clearing the CSV_Parser variable #29

Open ThePrimeDev opened 3 months ago

ThePrimeDev commented 3 months ago

Hey! I need to use my csvparser variable globally, and read in things with << but I don't know how to clear it so the new config doesnt get pasted after the old one. I tried using cp = CSV_Parser(mystring, "ucucucucucs"); but somewhy I get random esp32 crash using it..

michalmonday commented 3 months ago

Hey, currently it does not have any reset method. The only way to reset it is to create a new object. Your way of doing it seems correct (assuming that "mystring" wasn't deallocated).

One thing to beware is this kind of situation:

CSV_Parser cp(csv_str, /*format*/ "sLdcfx-");
int32_t *longs =          (int32_t*)cp["my_longs"];
cp = CSV_Parser(csv_str, /*format*/ "sLdcfx-");
// now longs is a pointer to deallocated memory
// Serial.print(longs[0]); could crash the program

// to avoid the crash we'd have to use:
// longs = (int32_t*)cp["my_longs"]; 
// and now we can access longs again safely

When assigning new object to existing object (without using "new" keyword), the old one is destroyed (destructor function is called which deallocates memory in case of CSV_Parser object).

Is there a chance you'd send the code so I could reproduce the crash?

MarksWizard commented 1 month ago

Hey there, I am also looking for help. I think I am having issues with pointing to deallocated memory, but unsure how to solve my issue. I get an error "assert failed: heap_caps_realloc_base heap_caps.c:432 (heap != NULL && "realloc() pointer is outside heap areas" when this code is run twice (it loads correctly the first time).

CSV_Parser errorCSV(/*format*/ "ss", /*has_header*/ true, /*delimiter*/ ','); 
char **err_Date = 0;
char **err_Text = 0;

if (errorCSV.getRowsCount() >> 0){
     Serial.print("Parser has ");
     Serial.print(errorCSV.getRowsCount());
     Serial.print(" rows before");

     errorCSV = CSV_Parser(/*format*/ "ss", /*has_header*/ true, /*delimiter*/ ',');

     Serial.print(" and Parser has ");      // This section shows that the rows are being cleared, returns 0 rows after
     Serial.print(errorCSV.getRowsCount());
     Serial.println(" rows after");

     err_Date = (char**)errorCSV["Date"];      // I put these here to try your solution
     err_Text = (char**)errorCSV["Error"];
    }
    errorCSV.readSDfile("/ErrorLog.txt");
    Serial.println("Error log loaded...");
michalmonday commented 1 month ago

Hello, it seems to me that this code should never satisfy/enter the "if rows count > 0" condition scope.

The first line creates an object: CSV_Parser errorCSV(/*format*/ "ss", /*has_header*/ true, /*delimiter*/ ',');

This object did not parse anything yet so the getRowsCount should return 0. There is errorCSV.readSDfile("/ErrorLog.txt"); line but it's after the condition. If this whole snippet is inside a loop or a callback, a new object would be called every time this code is ran, making the getRowsCount return 0 each time.

Is the code missing some parts? I'd like to try to reproduce the issue and then hopefully find the reason why it happens.

Btw currently, using string for indexing (e.g. errorCSV["Date"] rather than using numbers like errorCSV[0]) only works after parsing the CSV (because these strings are not known before parsing the header).

Not related: (errorCSV.getRowsCount() >> 0) condition should probably have ">" (higher than) instead of ">>" (bit shift to the right). But it shouldn't have any impact on the issue.

MarksWizard commented 1 month ago

Thanks for your response!

Sorry that my previous message was confusing for a number of reasons, because I tried to provide the smallest snippet of code that still triggered the issue. The variables at the top were actually global in my code. The rest of the code was part of a function that would print the error log to a webpage. The first call of the function would load the CSV properly and display the error log. Running it a second time (page refresh) would clear the parser (because row count is > 0), but crash when the code got to the line errorCSV.readSDfile("/ErrorLog.txt"); again. Without the if statement, the parser combines the previous data with the new data. Admittedly, my ability to write code is based heavily on examples by generous people like you!

I actually solved my issue yesterday. I realized that the variables really didn't need to be global, since everything is done within the function. I also added 2 lines csv_file.seek(0); rows_count = 0; to CSV_parser.cpp under the read SD File, I am not sure they're necessary but with these revisions the code is working as intended.

bool CSV_Parser::readSDfile(const char *f_name) {
  // open the file. note that only one file can be open at a time,
  // so you have to close this one before opening another.

  File csv_file = SD.open(f_name);
  if (!csv_file) 
    return false;

  csv_file.seek(0);
  rows_count = 0;

  // read file and supply it to csv parser
  while (csv_file.available())
    *this << (char)csv_file.read();

  csv_file.close();

  // ensure that the last value of the file is parsed (even if the file doesn't end with '\n')
  parseLeftover();
  return true;
}
{
    //The snippet of code I posted was previously here, but errorCSV was declared globally.  Now it resets every time the function is called.

    CSV_Parser errorCSV(/*format*/ "ss", /*has_header*/ true, /*delimiter*/ ',');  //parameters for loading the error log csv

    if (errorCSV.readSDfile("/ErrorLog.txt")) {     //load in the Error Log file if it exists
    Serial.print("Error log loaded successfully: ");        
    Serial.println (errorCSV.getRowsCount());

    err_Date = (char**)errorCSV["Date"];
    err_Text = (char**)errorCSV["Error"];

     String errList = "<h4><u>Error Log (";
     errList += errorCSV.getRowsCount();
     errList += "):</u></h4><h5>";
     for(int row = 0; row < errorCSV.getRowsCount(); row++) {
        errList += err_Date[row];
        errList += "   ";
        errList += err_Text[row];
        errList += "<br>";
      }

      errList += "</h5>";
      return errList;
    } 
  }

I copied the readSDfile function in CSV_parser.cpp and made a readSPIFFSfile for accessing the ESP32 onboard flash. The only difference is that it needs SPIFFS.h and uses SPIFFS.open instead of SD.open. Please consider adding the option of passing the FS to the readfile function in a future update. Thanks for writing the library, it has been useful for 2 of my projects!

MarksWizard commented 1 month ago

Hey there it's me again. I wanted to give an example of what I mean by passing the FS to the read file. For my application, I log all error messages to a CSV file located on SPIFFS since I want to log any issues when I initialize, read, or write to the SD card. I use your library to parse and display the error log on an "admin" page.

I have tested that this code works, if you could include something like this in a future version I would be grateful.

In CSV_Parser.cpp, I added #include and #include

/*  https://github.com/michalmonday/CSV-Parser-for-Arduino  */

#include "CSV_Parser.h"

#ifndef CSV_PARSER_DONT_IMPORT_SD
    #include <SD.h>
    #include <SPIFFS.h>
    #include <FS.h>

#endif

I verified that I do not need the 2 lines that I added in my previous post and modified the function in CSV_Parser.cpp. I am not sure if there is a way to check if the fs argument is passed, and if not default to SD to ensure backwards compatibility?

bool CSV_Parser::readSDfile(fs::FS &fs, const char *f_name) {
  // open the file. note that only one file can be open at a time,
  // so you have to close this one before opening another.

  File csv_file = fs.open(f_name);
  if (!csv_file) 
    return false;

  // read file and supply it to csv parser
  while (csv_file.available())
    *this << (char)csv_file.read();

  csv_file.close();

  // ensure that the last value of the file is parsed (even if the file doesn't end with '\n')
  parseLeftover();
  return true;
}

I added #include and #include in CSV_Parser.h. I am not sure if there's a way for me to only include these in the .cpp or .h, I got errors when I left them out of the .h file.

#ifndef CSV_PARSER_H
#define CSV_PARSER_H

 #include <SPIFFS.h>
 #include <FS.h>

#ifndef NON_ARDUINO
  #include <Arduino.h>

#else 
  #include <non_arduino_adaptations.h>
    extern SerialClass Serial;
#endif

The last thing to do is modify this line in CSV_Parser.h

#ifndef CSV_PARSER_DONT_IMPORT_SD
  bool readSDfile(fs::FS &fs, const char *f_name);
#endif

Now I can parse CSV files from either the onboard flash or SD card by passing the appropriate file system. This should also make it easier to load other filesystems such as Issue #14.

if (errorCSV.readSDfile(SPIFFS, "/ErrorLog.txt")) {
Serail.println("Error Log loaded successfully from SPIFFS");
}
if (errorCSV.readSDfile(SD, "/ErrorLog.txt")) {
Serail.println("Error Log loaded successfully from SD card");
}
michalmonday commented 1 week ago

Hello, sorry for late reply. This would be useful but SPIFFS.h and FS.h includes are not available for every board setting (e.g. Arduino Leonardo, Esp8266).

I tried using a template to use any type as the first parameter in the new readFile(T &t, const char *fname) but it seems that it requires instantiation in the CSV_Parser.cpp file like:

template bool CSV_Parser::readFile<SDLib::SDClass&>(SDLib::SDClass&, const char *);

This instantiation seems to work with Arduino Leonardo I used, but breaks compilation when Esp8266 or Esp32 boards are selected. Without this instantiation there's an undefined reference error.

I can't find a way to implement this that would work with AVR and Esp boards, not sure if there's a way to do that.