Naguissa / uRTCLib

Really tiny library to basic RTC functionality on Arduino. DS1307, DS3231 and DS3232 RTCs are supported.
https://www.foroelectro.net/librerias-arduino-ide-f29/rtclib-arduino-libreria-simple-y-eficaz-para-rtc-y-t95.html
GNU Lesser General Public License v3.0
87 stars 24 forks source link

eeprom write or read int might behave weird #7

Closed HidingCherry closed 5 years ago

HidingCherry commented 5 years ago

Hey there, I am not an experienced programmer, so please don't be angry at me.

I have looked over my code a few times, rewrote it in a "simple" way, but I still get wrong eeprom_read results. I actually want to write the time to the eeprom when a button is pressed (and released), but in this case I use a fixed int value with a fixed address.

The code:

#include "Wire.h"
#include "uRTCLib.h"

// uRTCLib rtc;
uRTCLib rtc(0x68, 0x50);

int test;
unsigned int rec = 0;

void setup() {
  delay (2000);

  Serial.begin(9600);
  Serial.println("Serial OK");
  Wire.begin();

  pinMode(2, INPUT_PULLUP);
}

void record() { // function to write the eeprom value on button change
  Serial.print("counter: ");
  Serial.println(rec);
  test = 666;
   Serial.print("test value before eeprom write: ");
   Serial.println(test);

  //rtc.eeprom_write(rec, (unsigned int) rtc.day());
  if (!rtc.eeprom_write(0, test)) {
    Serial.println("Failed to store test");
  }else {
    Serial.println("test correctly stored");
  }
    rtc.eeprom_read(0, &test);
   Serial.print("test value after eeprom write: ");
   Serial.println(test);
  test = 0;
}

void loop() {
  if(digitalRead(2) == LOW) {
    rtc.refresh();
    record();
    rec++;
    while(digitalRead(2) == LOW);
    rtc.refresh();
    record();
    rec++;
  }
}

And this is what I get when I very quickly press the button a few times:

counter: 133
test value before eeprom write: 666
test correctly stored
test value after eeprom write: 666
counter: 134
test value before eeprom write: 666
test correctly stored
test value after eeprom write: 666
counter: 135
test value before eeprom write: 666
test correctly stored
test value after eeprom write: 666

This when I take some time in between pressing and releasing the button:

counter: 1
test value before eeprom write: 666
test correctly stored
test value after eeprom write: -1
counter: 2
test value before eeprom write: 666
test correctly stored
test value after eeprom write: -1
counter: 3
test value before eeprom write: 666
test correctly stored
test value after eeprom write: -1

I have no idea what might be wrong. Maybe the cheap TinyRTC module I bought or the lib?

Naguissa commented 5 years ago

Very strange.. Which board are you using?

HidingCherry commented 5 years ago

I use an (original) Arduino Uno R3, the TinyRTC was really cheap. I have five of those TinyRTCs, though I didn't test the others yet. I will report back after I checked the others. (In about 15h - it's night already here.)

At the end for me it is important to know if it is the library or my TinyRTCs. In the latter case I will use the Arduinos eeprom.

edit: The values from eeprom read are different with signed and unsigned int values. In this case it is "-1" in the other something like "65535" (from what I remember).

Naguissa commented 5 years ago

I'll test the codein my nano...

23:45h here... :)

Naguissa commented 5 years ago

I was able to reproduce it and fix it.

Should be fixed in this new release: https://github.com/Naguissa/uRTCLib/releases/tag/4.2.1

Please, confirm it's working as expected now.

Cheers!

Naguissa commented 5 years ago

(Oh! Should appear on library manager as update in few hours!)

HidingCherry commented 5 years ago

fix confirmed It works now as expected, thank you :)

Naguissa commented 5 years ago

You're welcome!

Naguissa commented 5 years ago

Oh! An explanation:

In write function it was known and delay was there, but not on read functions. Adding that delays solved the issue.

HidingCherry commented 5 years ago

Ah, ok, thank you for the explanation. I saw the delay changes (though, I noticed that the delay value was already defined before that commit, so I was a bit confused) and I understood a bit of it.

Though, I still don't know why the AVRs needs the delay and other MCUs do not. (I also own a ESP8266, so I might face some issues on differences like this later.)

Naguissa commented 5 years ago

May be an issue that is only reflected in some special race conditions. Or maybe AVRs have a Wire implementation slighty different to others (more efficiency-based), given the different CPU power.

I don't know, but I was not facing that problem in the example in any MCU, and other programs I have using this library were using other MCUs, so went undetected until now.

Naguissa commented 5 years ago

Oh! Another thing: I've added some RTC datasheets on extras folder, it may interest to you!

HidingCherry commented 5 years ago

Ok, guess I have to keep in mind that AVRs might behave a bit different than MCUs.

Yeah, I noticed the datasheets and I will surely look into them, thanks again :)

Naguissa commented 5 years ago

Caution: new release that breaks EEPROM data ordering, so new version cannot read old version written EEPROMs: https://github.com/Naguissa/uRTCLib/releases/tag/4.3.0

I suggest you to use new one, as improvements on EEPROM are huge.

HidingCherry commented 5 years ago

@Naguissa I have some weird behavoir (I already wondered why my code is so slow). I think this is a direct impact to your changes.

void read_eeprom(int position){
  long minitimer = millis();

  rtc.eeprom_read(position  , &bDay    );
  rtc.eeprom_read(position+1, &bMonth  );
  rtc.eeprom_read(position+2, &bDOW    );
  rtc.eeprom_read(position+3, &bHour   );
  rtc.eeprom_read(position+4, &bMinute );
  rtc.eeprom_read(position+5, &bSecond );

  Serial.print(millis() - minitimer,DEC);
  Serial.println("msec");
}

This "little" process takes about 7756msec (+- 1msec), repeatable. Is there a possibility for optimization?

PS: uRTCLib version 4.3.0 installed PPS: The data type of those variables is byte. PPPS: By the way, if I remember right, I could save up to 4096 variables in version 4.2.1. Now it is 256.

Naguissa commented 5 years ago

Yes, it's because needed delays to prevent malfunction.

BTW, I'm splitting both functionalities, creating an improved version focused on EEPROMs and this library will lack EEPROM support. I suggest you try new library: https://github.com/Naguissa/uEEPROMLib (also available at Ardiuno IDE Library Manager).

I'll apply some improvents to uRTCLib and release a new version WITHOUT EEPROM support, from now on.

HidingCherry commented 5 years ago

I kinda fixed the problem (though, just using your new library wasn't enough). I adapted your new read method:

void read_eeprom(int position){
  long minitimer = millis();

  bDay    = eeprom.eeprom_read(position  );
  bMonth  = eeprom.eeprom_read(position+1);
  bDOW    = eeprom.eeprom_read(position+2);
  bHour   = eeprom.eeprom_read(position+3);
  bMinute = eeprom.eeprom_read(position+4);
  bSecond = eeprom.eeprom_read(position+5);

  Serial.print(millis() - minitimer,DEC);
  Serial.println("msec");
}

It now takes about 94msec. I guess the old method which I used before is much slower.

(The old method took about 1.3 seconds per position, which resulted in 7.7 seconds for six positions.)

Naguissa commented 5 years ago

Even more, you can use:

byte data[6];

bool resultOK = eeprom.eeprom_read(position, &data[0], 6);

Proably the adaption implies removing delays or reducing the define to 0 or almos. Be careful about timings and compatibility issues.

Cheers!

HidingCherry commented 5 years ago

Tried that method, it takes about 1317msec (or 1.3 seconds) to read the whole block.

edit: But you are right about timing/compatibility issues. I already wondered why DoW is being printed wrong. (With my adaption above.)