stevemarple / IniFile

Arduino library to parse ini files.
GNU Lesser General Public License v2.1
87 stars 45 forks source link

Bug fix, Add support for ESP32 & SdFat library #23

Open toybox01 opened 5 years ago

stevemarple commented 5 years ago

What bug does this fix? The regression tests pass without these changes.

Generally the patch looks good and it passes the regressions tests in the test/ directory (run make clean; make to test).

The patch can be simplified by reducing the number of conditional compilation sections which should make future maintenance easier. Could you please introduce a typedef inside the start of the class to define an IniFile::mode_t type that is used instead of extra conditional compilation compilations steps that select uint8_t / oflag_t / const char*? The class definition would look like

class IniFile {
public:
#if defined(PREFER_SDFAT_LIBRARY)
    typdef oflag_t mode_t;
#elif defined(ARDUINO_ARCH_ESP32)
    typdef const char* mode mode_t;
#else
    typedef uint8_t mode_t;
#endif

    enum error_t {
... 

and then use mode_t mode where I used uint8_t mode, instead of additional conditional compilation blocks?

toybox01 commented 5 years ago

Going in infinite loop when ini file in a special condition

Before

if (isCommentChar(*cp)) {
    // return (err == errorEndOfFile ? errorSectionNotFound : errorNoError);
    if (err == errorSectionNotFound) {
        _error = err;
        return true;
    }
    else
        return false; // Continue searching
}

After

if (isCommentChar(*cp)) {
    // return (err == errorEndOfFile ? errorSectionNotFound : errorNoError);
    if (err == errorEndOfFile) {
        _error = errorSectionNotFound;
        return true;
    }
    else
        return false; // Continue searching
}
toybox01 commented 5 years ago

sample.ino

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

IniFile Config("/config.ini");

void setup() {
  Serial.begin(9600);

  if (!SD.begin(4)) {
    Serial.println(F("Card failed, or not present"));
    while (1);
  }

  char buffer[128];
  if (!Config.open() || !Config.validate(buffer, sizeof(buffer))) {
    Serial.println(F("Failed to load configuration file"));
    while (1);
  }

  if (Config.getValue("section", "key", buffer, sizeof(buffer))) {
    Serial.print("section/key: ");
    Serial.println(buffer);
  }
  Config.getValue("abracadabra", "key", buffer, sizeof(buffer)); // oops

  Serial.println(F("Pass"));
}

void loop() {
}

config.ini

[section]
key = value

# when section doesn't exist and the last line is a comment
stevemarple commented 5 years ago

I have pushed several commits based on the pull request you submitted. I made sure you are credited with those changes, I also added you to the contributors list. I wanted separate commits for the feature enhancements and the bug fix to allow changes to be reverted more easily. The ESP32 and SdFat library support should be available in version 1.2.0, and the bugfix appears in version 1.2.1. This means users can downgrade bu still keep ESP32 support if an issue is found in the bugfix.

Thanks for contributing, Steve

toybox01 commented 5 years ago
bool IniFile::findSection(const char* section, char* buffer, size_t len,
                          IniFileState &state) const
{
    if (section == NULL) {
        _error = errorSectionNotFound;
        return true;
    }

    error_t err = IniFile::readLine(_file, buffer, len, state.readLinePosition);

    if (err != errorNoError && err != errorEndOfFile) {
        // Signal to caller to stop looking and any error value
        _error = err;
        return true;
    }

    char *cp = skipWhiteSpace(buffer);
    //if (isCommentChar(*cp))
    //return (done ? errorSectionNotFound : 0);
    if (isCommentChar(*cp)) {
        // return (err == errorEndOfFile ? errorSectionNotFound : errorNoError);
        if (err == errorSectionNotFound || err == errorEndOfFile) {
            _error = errorSectionNotFound;
            return true;
        }
        else
            return false; // Continue searching
    }
...

err == errorSectionNotFound || It is unnecessary

1st IniFile::readLine only have these return values errorFileNotOpen, errorBufferTooSmall, errorSeekError, errorEndOfFile, errorNoError

2nd

if (err != errorNoError && err != errorEndOfFile) {
    // Signal to caller to stop looking and any error value
    _error = err;
    return true;
}

I think it won't pass without errorNoError, errorEndOfFile

stevemarple commented 5 years ago

1st IniFile::readLine only have these return values errorFileNotOpen, errorBufferTooSmall, errorSeekError, errorEndOfFile, errorNoError

Agreed. I've committed a change for that.

2nd

if (err != errorNoError && err != errorEndOfFile) {
  // Signal to caller to stop looking and any error value
  _error = err;
  return true;
}

I think it won't pass without errorNoError, errorEndOfFile

Can you explain further please? I believe the code passes and stevemarple:master is the same as toybox01:master.

toybox01 commented 5 years ago

Sorry for my English >_<

if (err != errorNoError && err != errorEndOfFile) {
    // Signal to caller to stop looking and any error value
    _error = err;
    return true;
}

After these code err can only be errorNoError, errorEndOfFile It can't be errorSectionNotFound

Thank you for your patience