Pointer confusion C

[Jester]

What I'm trying to accomplish:
I wrote some functions to write and retrieve data from a SPI based EEPROM.
The functions work but.....

The problem:
During initialization I need to read three pages of data from the EEPROM and maintain that data for use in the program.

The details
The eepromReadPage() function returns a pointer to a page of data (8 x Unit16 per page) and the function appears to work just fine
I declare a pointer for each page as follows:
    static Uint16 *p0;
    static Uint16 *p1;  // etc

Later I call the read page function and use the pointers to point to the data as follows:
    p0 = eepromReadPage(0);
    p1 = eepromReadPage(1);

The problem:
If I stop the program just after the first (p0) read (I'm using CodeComposerStudio and use a breakpoint), I can see the correct data from page 0, that I wrote previously it gets located at 0xBCC0 - 0xBCC7
If I now stop after the second (p1) read, P1 is also pointing to 0xBCC0, and the data from the second page of EEPROM has overwritten the first page data!.

p0 is located at 0xBBC6, and p1 is located at 0xBBC8, both show 0xBCC0 as the actual pointer address

The eepromReadPage()

Code: [Select]


Uint16 * eepromReadPage(Uint16 pageNum)
{
    static Uint16 *ebuf;        // Perhaps this does not need to be static???

    CS_EE_SELECT;
    eepromSendReadCommand(pageNum);
    ebuf =  eepromReceivePage(EEPROM_PAGE_SIZE);  // Jly 13 try this
    CS_EE_RELEASE;
    return ebuf;
}



[Ian.M]

Your eepromReadPage() doesn't allocate storage for the buffer so where do you think its putting the words it reads?  (hint: nowhere good!).  Also every call to it returns a copy of the *SAME* pointer, which doesn't get changed, so of course they are at the same address.

The only sane fix is to allocate storage for the page data either globally or in the scope you want to use it in, than pass a pointer to that storage to eepromReadPage() for it to fill.  Whether you use an array definition or malloc() depends on your environment - many MCU toolchains don't support malloc().

There's also probably a similar flaw in eepromReceivePage() - its returning a pointer, but isn't told where to put the page it reads, so repeated calls will almost certainly overwrite the previous call's page data.  You'll need to fix that, either by modifying that function to also take a buffer pointer, or, after verifying it has allocated static storage for its buffer, memcpy() its result buffer to the one you allocated.

Edit: NominalAnimal has a good point below about wrapping arrays in structures so assignment will copy their contents, not duplicate the pointer to them.

[Nominal Animal]

A pointer is only a reference, it does not copy the data.

You did not show the implementation of eepromReceivePage(), but my guess is it uses static storage for it, and always returns the same pointer.  Thus, consecutive calls just overwrite the same memory region, and your p0 == p1.

It appears that your microcontroller does not have the hardware facility to map SPI EEPROM directly into memory, so instead it has to be transferred (to a local buffer).

If you wanted a copy of the data, then it would be easiest to use a structure to hold the data:
    typedef struct {
        union {
            uint64_t  u64[2];
            uint32_t  u32[4];
            uint16_t  u16[8];
            uint8_t    u8[16];
        };
    } eeprom_page;

and copy the EEPROM data to an user-supplied buffer, say:

    eeprom_page  eepromGetPage(uint16_t page) {
        eeprom_page  result;
        CS_EE_SELECT;
        eepromSendReadCommand(pageNum);
        result = *(eeprom_page *)(eepromReceivePage(EEPROM_PAGE_SIZE));
        CS_EE_RELEASE;
        return result;
    }

Just remember that each static copy of eeprom_page you use, takes 16 bytes of RAM.  Then you can do things like

    eeprom_page  p0, p1;
    p0 = eepromGetPage(0);
    p1 = eepromGetPage(1);

[Doctorandus_P]

One of the problems is using self defined data types such as "Uint16". Anyone can guess what this is supposed to be, but nobody can be certain without looking it up in the code and verifying. Such things are a nuisance for portability and maintainability. If you find portability or maintainability important, or even want to state your code actually does on a forum without "us" second guessing, then use the standardized data types, just like nominal animal shows in his example.

[Jester]

NominalAnimal
Thank you, your example works perfectly, I learned something and you saved me a lot of time and grief. :-)


Ian.M

Your eepromReadPage() doesn't allocate storage for the buffer so where do you think its putting the words it reads?  (hint: nowhere good!). 

There's also probably a similar flaw in eepromReceivePage() - its returning a pointer, but isn't told where to put the page it reads, so repeated calls will almost certainly overwrite the previous call's page data.  You'll need to fix that, either by modifying that function to also take a buffer pointer, or, after verifying it has allocated static storage for its buffer, memcpy() its result buffer to the one you allocated.

As mentioned seems to be working fine using Nominal Animal technique ( I now have all the data and it's not overwritten), but I realize that my eepromReceivePage() might be less than  ideal.

I would imagine that while eepromReceivePage is running ebuf is used to temporarily store the 8 values, and then they get assigned to result, and result(s) is ultimately the original declared structure(s) eeprom_page

I don't think overwriting the data in ebuf is of any consequence because the data has already been transfered to the eeprom_page structure

Does that make sense?

Existing eepromReceivePage()

Code: [Select]

Uint16 * eepromReceivePage(Uint16 pageSize){
       Uint16 ebuf[EEPROM_PAGE_SIZE];    // was static,

      for( Uint16 i=0; i < pageSize; i++ ) {
          ebuf[i] = eepromReceiveWord(i);
      }
     CS_EE_RELEASE;
     return ebuf;
}



[Jeroen3]

It looks like you have not fully embraced the possibilities pointer provide, and are trying your way "around" them.

Why make the reading of eeprom dependent on anything the calling code has for chunks?
Why adding a unnecessary copy?
Why make the function non-reentrant with static?
Why invent your own Uint16 as Docterandus_P rightfully mentions, stdint.h is a thing!

Take inspiration how proven code does things, like fread

Code: [Select]

size_t fread(void *restrict ptr, size_t size, size_t nitems, FILE *restrict stream);Obviously they go far beyond what you would need, doing restrict and void*. The the intent should be clear, right?
The function knows nothing of what your data type, size or where it should go.

You can make either one of these:

Code: [Select]

int eeprom_read(uint16_t address, uint8_t* page, size_t size)
int eeprom_readPage(uint16_t pageIndex, uint8_t* page, size_t size)

When you're reading:
- you need to specify where in the eeprom you want to read. Either an address or a page index, up to you.
- you will have a place to store the data, why not store it there immediately?
- you need to ensure not to overflow the place you pointed to, size should limit how many byte are written, or dictate how many bytes should be read, up to you.
Then return the number of bytes read so you can know if it failed. 0 would be a problem, negative would be a specific error code.

Technical:
Try to use standard types where possible to make code most portable, only use fixed width where explicitly required.
Using uint8_t* guarantees you will access the data byte-wide inside the function. (no alignment constraints)
size_t communicates explicit intent for that variable, the use of sizeof().

eg:

Code: [Select]

eeprom_page  p0, p1;
if(0 >= eeprom_read(0, (uint8_t*)&p0, sizeof(p0))){
  printf("reading failed");
}
[code]

Limitations:
struct packing may introduce portability issues, (de)serialization across platforms should be done in a safer way.



[Nominal Animal]

As mentioned seems to be working fine ( I now have all the data and it's not overwritten), but I realize that my eepromReceivePage() might be less than  ideal.
I would imagine that while it's running ebuf is used to temporarily store the 8 values, and then they get assigned to result and result(s) is ultimately the original declared structure(s) eeprom_page

What actually happens, is that it uses a static (global, always allocated) ebuf to store the result, and then returns a pointer to it.

A better way to implement it would be e.g.

Code: [Select]

void eepromReceivePage(uint16_t words, uint16_t data[words], uint16_t page)
{
    CS_EE_SELECT;
    eepromSendReadCommand(page);
    for (uint16_t i = 0; i < words; i++)
        data[i] = eepromReceiveWord(i);
    CS_EE_RELEASE;
}
The first parameter is the page size, the second parameter is an array of 16-bit unsigned integers of that size, and the last parameter is the page number.

(In C, arrays decay to a pointer to the first element of that element type, so the middle parameter could have been declared as uint16_t *data instead, a pointer to the data.  However, with the above form, if you enable warnings in your C compiler, it will warn you if you try to read into a too small variable – something that we call buffer overrun bug.)

Then, if you want the first two pages into cache, you could use
    #define  EEPROM_PAGE_WORDS  8
near the top of your C source file, so you don't have magic numbers in your code, and then
    uint16_t  cache[2][EEPROM_PAGE_WORDS];  /* cache[page][word] */
    eepromReceivePage(EEPROM_PAGE_WORDS, cache[0], 0);
    eepromReceivePage(EEPROM_PAGE_WORDS, cache[1], 1);

Note that because cache is a two-dimensional array, specifying cache[n] refers to the one-dimensional sub-array.  The first element of the first page would be cache[0][0], and the second element of the first page cache[0][1].  (The fastest-changing index in memory order is always the rightmost one.)

If you use the structure in my previous post, and have eeprom_page p0, you can use
    eepromReceivePage(EEPROM_PAGE_WORDS, p0.u16, pageNum);
(All the arrays within the union occupy the same memory; .u16 is the 16-bit unsigned integers one.  It is useful for type-punning, reinterpreting the storage bits as a variable of a different type.)

If the EEPROM SPI interface can support transfers of any number of words (that begin at a page boundary), then you can use
    eepromReceivePage(sizeof p0.u16 / sizeof p0.u16[0], p0.u16, pageNum);
or read into any structure variable (that is aligned to at least uint16_t, and an integer number of uint16_t in size) named foo using
    eepromReceivePage(sizeof foo / sizeof (uint16_t), (uint16_t *)&foo, pageNum);

[nctnico]

Personally I like to pass the size of a buffer to a function so the buffer size can be checked runtime (and lets the function fail if the buffer is too small). The layers above can then deal with the problem in some way but at least make the device not work as expected in a defined way (blinking a led, showing a warning, resetting, etc) instead of overwriting memory and do undefined behaviour.

[SiliconWizard]

Personally I like to pass the size of a buffer to a function so the buffer size can be checked runtime (and lets the function fail if the buffer is too small). The layers above can then deal with the problem in some way but at least make the device not work as expected in a defined way (blinking a led, showing a warning, resetting, etc) instead of overwriting memory and do undefined behaviour.

Yes. Passing a pointer to a buffer with no associated size is pretty bad. Just don't do it.
If you do, what it conveys is: "I'm making lots of assumptions". Assumptions are a programmer's worst ennemies.
Of course C has (at least a long time ago, and it has lingered on) set a precedent with zero-terminated strings. It has fixed its "mistakes" with the 'n' versions of their string functions, for instance, but old habits are hard to change.

If you are writing a function that only deals with a predefined buffer size in all use cases and you don't want to pass the size around for some reason, do not use pointer arguments, use array arguments (with a defined size, not just '[]'). That documents the intent much, much better and gives the compiler an opportunity to do some static checking.

Small example:

Code: [Select]

void Foo(char Buffer[ 256])
{
    Buffer[0] = 1;
    Buffer[256] = 0;
}

void Bar(char Buffer[128])
{
    Foo(Buffer);
}
Yields the following warnings:

Code: [Select]

<source>: In function 'Bar':
<source>:11:5: warning: 'Foo' accessing 256 bytes in a region of size 128 [-Wstringop-overflow=]
   11 |     Foo(Buffer);
      |     ^~~~~~~~~~~
<source>:11:5: note: referencing argument 1 of type 'char[256]'
<source>:3:6: note: in a call to function 'Foo'
    3 | void Foo(char Buffer[ 256])
      |      ^~~
<source>: In function 'Foo':
<source>:6:11: warning: array subscript 256 is outside array bounds of 'char[256]' [-Warray-bounds=]
    6 |     Buffer[256] = 0;
      |     ~~~~~~^~~~~
<source>:3:15: note: at offset 256 into object 'Buffer' of size [0, 256]
    3 | void Foo(char Buffer[ 256])
      |          ~~~~~^~~~~~~~~~~~
<source>: In function 'Bar':
<source>:6:17: warning: array subscript 256 is outside array bounds of 'char[128]' [-Warray-bounds=]
    6 |     Buffer[256] = 0;
      |     ~~~~~~~~~~~~^~~
<source>:9:15: note: at offset 256 into object 'Buffer' of size [0, 128]
    9 | void Bar(char Buffer[128])
      |          ~~~~~^~~~~~~~~~~

[cv007]

Code: [Select]

Uint16 * eepromReceivePage(Uint16 pageSize){
       Uint16 ebuf[EEPROM_PAGE_SIZE];    // was static,

      for( Uint16 i=0; i < pageSize; i++ ) {
          ebuf[i] = eepromReceiveWord(i);
      }
     CS_EE_RELEASE;
     return ebuf;
}Maybe you do not realize what is happening with ebuf in this case- ebuf gets allocated on the stack when entering the function (compiler moves stack pointer to make room), is populated with data, the function returns the address of the ebuf stack address, the function returns with the stack position restored. You now have data you want to access that is on the stack in an area that is now free to be used again (like another function call or an interrupt), so this data can easily be overwritten. In other words, do not return any stack data (local vars) by reference/pointer because it will be invalid after the return. It may happen to work some times, but is guaranteed to fail (data overwritten), most likely quite soon.

Also in case you do not know, ebuf is an array, and an array cannot be passed/returned by value so it 'decays' to a pointer and the 'return ebuf;' essentially turns into 'return &ebuf[0];' .

You can now see why these kind of functions typically have the caller provide the buffer (via pointer in C or reference in C++). The caller is responsible for providing the buffer space (and a good idea to provide its size in C, or use of a template in C++ to 'automatically' provide size), the callee can verify the buffer size is adequate and provide a mechanism to return ok/fail information (bool, or int/enum if there is need for more than ok/fail).

[Jester]

Code: [Select]

Uint16 * eepromReceivePage(Uint16 pageSize){
       Uint16 ebuf[EEPROM_PAGE_SIZE];    // was static,

      for( Uint16 i=0; i < pageSize; i++ ) {
          ebuf[i] = eepromReceiveWord(i);
      }
     CS_EE_RELEASE;
     return ebuf;
}Maybe you do not realize what is happening with ebuf in this case- ebuf gets allocated on the stack when entering the function (compiler moves stack pointer to make room), is populated with data, the function returns the address of the ebuf stack address, the function returns with the stack position restored. You now have data you want to access that is on the stack in an area that is now free to be used again (like another function call or an interrupt), so this data can easily be overwritten. In other words, do not return any stack data (local vars) by reference/pointer because it will be invalid after the return. It may happen to work some times, but is guaranteed to fail (data overwritten), most likely quite soon.

Also in case you do not know, ebuf is an array, and an array cannot be passed/returned by value so it 'decays' to a pointer and the 'return ebuf;' essentially turns into 'return &ebuf[0];' .

You can now see why these kind of functions typically have the caller provide the buffer (via pointer in C or reference in C++). The caller is responsible for providing the buffer space (and a good idea to provide its size in C, or use of a template in C++ to 'automatically' provide size), the callee can verify the buffer size is adequate and provide a mechanism to return ok/fail information (bool, or int/enum if there is need for more than ok/fail).

Thanks to everyone for your help, I'm not a programmer (just fumble my way through as required when necessary) so this kind of explanation goes a long way. Mental note have the caller allocate the buffer. To that end I used Nominal Animals latest suggestion for eepromReceivePage()

Code: [Select]


void eepromReceivePage(uint16_t words, uint16_t data[words], uint16_t page)
{
    CS_EE_SELECT;
    eepromSendReadCommand(page);
    for (uint16_t i = 0; i < words; i++)
        data[i] = eepromReceiveWord(i);
    CS_EE_RELEASE;
}

Code is now cleaner, saner and seems to work, I'm a happy camper.. Have a great day!

[DavidAlfa]

Seems an odd way to declare the buffer, I'd use uint16_t *data instead uint16_t data[words], but let's see what the programming army say 

[Nominal Animal]

Seems an odd way to declare the buffer, I'd use uint16_t *data instead uint16_t data[words], but let's see what the programming army say 

It is functionally equivalent, because the array "decays" to a pointer to its first element.

What makes this form useful is that it tells the C compiler about the expected access range to the array, both in the caller (from the prototype), and within the function itself.
Depending on your compiler and compiler options, it can help detect silly buffer overrun errors.  (Not all related warnings have been implemented in GCC and Clang yet.)

When using the pointer form, there is no way to tell the compiler to associate the "size" or "words" parameter with the array length, which makes it unable to detect buffer overrun bugs.  Off-by-one are the most common.  This "odd" form, as you call it, does exactly that with zero runtime overhead.

For such reasons, the form "(size_t len, TYPE array[len])" should be preferred over "(size_t len, TYPE *array)", even though both yield the exact same runtime machine code.  The former helps the compiler detect issues, the latter makes sure the compiler is not able to.

[Jeroen3]

I recall reading something in the new features for C23 about improved checks against out of bounds arrays using this. But I can't find it right now.

[Nominal Animal]

There are also quite a few requests-for-comments flying about, including clang __counted_by(varname) annotation for pointers, and gcc and clang __element_count(varname) attribute for flexible array members, and various other stuff related to array bounds checking at both compile time and run time –– I prefer the ones that help at compile time, and add zero overhead at run time.

This is definitely something compiler developers and users are thinking a lot about.  It just surprises me that the variably modified parameter form is often "forgotten" (it not being new, already defined in C99).

One annoying thing about this form is that the size must precede the array.  Most existing C interfaces (like memset()) specify the pointer first, size after, which makes it weird to switch to the size(s) first, then data in array form.

I'm not ashamed (well, maybe a little!) to admit it this better form does not come naturally to me; I do have to consciously choose it.  If I don't think at all, I'll just type the pointer form instead, with size after the pointer.

If only the compiler developers were to allow, as an extension, a variably modified parameter to specify a later parameter in the parameter list, we could just change the prototypes of most functions, without affecting the generated machine code at all.  Right now, we would need to change the parameter order, but otherwise the generated machine code is not affected.


A big reason why current versions of C compilers do not catch obvious errors, like accessing the n'th element of an n-element array, i.e. just past the end of the array, is that they internally only check arrays of compile-time integer constant length.  However, the C99 standard explicitly allows arithmetic operations in the array length, it does not need to be a single parameter.  For example,
    void foo(size_t bytes, uint32_t data[bytes / sizeof (uint32_t)]);
which would allow you to do stuff like
    uint32_t  my_buffer[32];
    foo(sizeof my_buffer, my_buffer);

To properly support variably modified array parameter bounds checks at compile time, they need to be able to express the array size as an arithmetic expression based on one or more parameters or variables, and compare to access indices involving a similar expression.  Right now, the compilers only deal with literal limits known at compile time.

For example, the array count expression above is (bytes/sizeof(uint32_t)), simplifying to (bytes/4).  An array index expression could be (bytes/5 + 1).  The task to add to compilers, is to be able to compare such expressions, and tell if the latter is equal to or greater than the former.  It is not impossible by any means, but quite a bit of work!

(In case you wonder, the array index expression is out of bounds when bytes<=20, and valid when bytes>20.  I don't expect the compiler to tell me that, but a warning that "depending on the bytes value, the access may be out of bounds" would be nice.  I needed to solve the inequality (bytes/5+1) >= (bytes/4) to find out, which is what the compilers would also need to implement, to fully support compile-time bounds checking; noting that the expressions may contain more than one variable.)

That is why I posted this message, hopefully explaining why the support for such array bounds checking is still in the works, and may not be implemented at all in all C compilers.

[DavidAlfa]

It is functionally equivalent, because the array "decays" to a pointer to its first element.
What makes this form useful is that it tells the C compiler about the expected access range to the array, both in the caller (from the prototype), and within the function itself.

I actually expected the compìler complaining about "words" not being a constant.

void eepromReceivePage(uint16_t words, uint16_t data[words], uint16_t page)

[SiliconWizard]

It is functionally equivalent, because the array "decays" to a pointer to its first element.
What makes this form useful is that it tells the C compiler about the expected access range to the array, both in the caller (from the prototype), and within the function itself.

I actually expected the compìler complaining about "words" not being a constant.

void eepromReceivePage(uint16_t words, uint16_t data[words], uint16_t page)

It would complain if it wasn't compliant with at least C99.
This is a VLA, and while many frown upon VLAs in the context of local variables, the function argument form of VLAs is useful and without any potential nasty issues.

Note that one can additionally add the 'static' keyword in front of the size for the VLA as a function argument, like so:

Code: [Select]

void eepromReceivePage(uint16_t words, uint16_t data[static words], uint16_t page)
What it means is that the 'data' parameter passed to the function should not be NULL. It's usually checked at compile-time when it can be determined statically. It usually doesn't yield any run-time check, although I don't think the C standard mandates anything in particular regarding this.