IniFile icon indicating copy to clipboard operation
IniFile copied to clipboard

Bug fix, Add support for ESP32 & SdFat library

Open toybox01 opened this issue 6 years ago • 7 comments

toybox01 avatar Apr 20 '19 05:04 toybox01

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?

stevemarple avatar Apr 20 '19 08:04 stevemarple

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 avatar Apr 20 '19 09:04 toybox01

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

toybox01 avatar Apr 20 '19 15:04 toybox01

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

stevemarple avatar Apr 20 '19 19:04 stevemarple

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

toybox01 avatar Apr 20 '19 23:04 toybox01

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.

stevemarple avatar Apr 21 '19 13:04 stevemarple

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

toybox01 avatar Apr 21 '19 13:04 toybox01