Possible bug at eeprom_write(const unsigned int, void *, const uint32_t)

[dmelo]

https://github.com/Naguissa/uRTCLib/blob/d441af57a09dc94aed14064204bf6cf54b580264/src/uRTCLib.cpp#L335

This line seems wrong, or at best, misleading. It seems to be dereferencing data (as uint32_t) and shifting right the value. I think I don't understand the purpose of it. Considering the equivalent eeprom_read, I was expecting eeprom_write to be something like this:

bool uRTCLib::eeprom_write(const unsigned int address, void *data, const uint32_t n) {
        bool r = true;
        uint32_t i;
        for (i = 0; i < n; i++) {
                byte *b = ((byte *) data) + (n - i - 1);
                r &= _eeprom_write(address + i, *b);
        }
        return r;
}

Placing the original snippet on a simple program:

#include<stdio.h>
#include<stdlib.h>

int main()
{
    char data[30];
    int i = 0, n = 10;
    sprintf(data, "abcdefghijklmnopqrstuvwxyz");

    printf("%s\n", data);
    for (i = 0; i < n; i++) {
        printf("%c\n", (*((u_int32_t*) data)) >> ((n - i - 1)*8) & 0xFF);
    }

    return 0;
}

I get the following output:

abcdefghijklmnopqrstuvwxyz
c
b
a
d
c
b
a
d
c
b

I was expecting to see the letters "jihgfedcba".

[Naguissa]

Hello,

Maybe I should clarify that function a little bit: It's used to write an element of arbitrary data length.

It assumes big-endian data (also on read, to maintain data).

It writes an arbitrary data (void *data) based on its length (n).

The conversions and shifts are needed to avoid problems with automatic casts, that gave problem on some microcontrollers and numerical types (I'm not sure, but I recall problems with STM32 and float data type).

It woks getting the data in 1-byte chunks and writing it to EEPROM in big endian mode.

The "cba" result it's the correct one for that char array if you specify a 3-byte length (c-b-a are the 3 initial bytes in reverse order).

Thanks for the comment!

[dmelo]

Hi @Naguissa, thank you for clarifying the need for casting. About the c-b-a, the length specified is 10, which is why I expected to see chars from a to j.

[Naguissa]

Oh! I see the issue!!!

No, the problem is that thius function is intended to basic data types, not arrays on complex types.

If you check the casts it uses uint32_t, which is the longest data type I found in all microcontrollers (4 bytes). And this is the maximum length this function support.

You can check it this way:

include<stdio.h>
#include<stdlib.h>

int main()
{
    char data[30];
    int i = 0, n = 4;
    sprintf(data, "abcdefghijklmnopqrstuvwxyz");

    printf("%s\n", data);

    for (i = 0; i < n; i++) {
        printf("%c\n", (char) ((*((u_int32_t*) data)) >> ((n - i - 1)*8) & 0xFF));
    }

    return 0;
}

Result is:

$ nano -w issue.c
$ gcc issue.c -o issue
$ ./issue 
abcdefghijklmnopqrstuvwxyz
d
c
b
a
$

n cannot be longer than 4 (uint32_t length).

[Naguissa]

P.S.: To write arrays, it's better to just use write inside a loop....

[Naguissa]

(this means, I'll have to change function definition and change n to unsigned char.... :) )

[Naguissa]

I think it could be an add-on; something like white_array and read_array.

4rd parameter could be array size.

Also, a shortcut with 3 parameters where size is automatically calculated as sizeof(array[0]).

[Naguissa]

Oh! If you can do it in same function could be excellent. But be aware about issues with numerical data types....

[dmelo]

OK. I will think of a nice way of implementing this and then I will create a PR.

[Naguissa]

Thanks!

[Naguissa]

Done!

https://github.com/Naguissa/uRTCLib/releases/tag/4.3.0

Caution: new version changes data ordering, so it will not read old version written EEPROMs correctly.