C: Returning an array from a function into a variable

[noname4me]

Hi

I am trying to return an array from a function that is reading from a memory location in the flash memory.

The data is read as a uint32_t, but is chunked in uint8_t (it is storing some numbers).

I am not able to read the numbers from the flash, so I thought that reading it as a uint32_t, then making a union of that with a uint8_t and reading it off using that was a work around.

The function that reads the memory is working, as I can see the data in the uint8_t form.

I am now trying to pass that information back to my calling function:

Code: [Select]

uint8_t * read_data_8(uint16_t idx, uint16_t len)
{

union d8a_t temp;
uint8_t outfiles[32];
__disable_irq();
EE_Reads(idx, len, &temp.bigdata);
__enable_irq();

memcpy(outfiles, temp.chunks, (strlen(temp.chunks)));

return outfiles;
}
This appears to work as far as outfiles contains the numbers I expect.


My receiving code looks like this:

Code: [Select]

uint8_t * TESTMENOW;
TESTMENOW =  read_data_8(BT_NAME,3);

int t1 = TESTMENOW;
int t2 = TESTMENOW +1;
int t3 = TESTMENOW +2;

and TESTMENOW gives me a 0x0 in my debugger (Atollic).


t1 = 0
t1 = 1
and t2 = 2


I am sure I'm doing something stupid with the pointers, but I tried similar code on https://www.tutorialspoint.com/compile_c_online.php and I was able to return the array.

Code: [Select]

#include <stdio.h>

/* function to generate and return random numbers */
int * getRandom( ) {

static int  temp[10] = {0};
temp[1] = 'A';

temp[1] = 'A';

temp[2] = 'B';

temp[3] = 'C';

temp[4] = 'D';


   return temp;
}

/* main function to call above defined function */
int main () {

   /* a pointer to an int */
   int *p;
   int i;

   p = getRandom();

   for ( i = 0; i < 10; i++ ) {
      printf( "*(p + %d) : %d\n", i, *(p + i));
   }

   return 0;
}

Can anyone please give me a clue on what I'm doing wrong?

Thanks

[magic]

You are returning pointer to a local stack variable which ceases to exist when the function returns.

Technically, the bits still remain on the stack so you have some chance of getting them as shown by your second sample, but they can be erased at any time by the calling function making some new stack allocation or by an interrupt.

It seems that your debugger helpfully zeros out the stack after returning to catch exactly that kind of bugs.

You need to accept a pointer to a buffer allocated by the calling function, like EE_Reads does, or return those bytes by value.

[GromBeestje]

You are returning outfiles, which is a local declaration in the read_data_8. A pointer to this array is no longer valid when you the function goes out of scope. A possible solution is to make it static

static uint8_t outfiles[32];

This means, there will be a fixed memory block allocated for the array that will always be reserved for this function, so it won't get overwritten any time the function returns. Note that this memory will always be allocated, and that this also means that calling the multiple times, it will always return the same pointer, and it will contain the data of the last call.

[magic]

And if you store that pointer somewhere and call the function again, the value pointed by the old pointer will change. That's asking for some debugging adventure.

[noname4me]

Ahh I see...this makes sense

I am not on the PC now, but I wanted to ask:

If I cleared the outfiles array at the start of the function (by filling with NULL or NULL terminating each time data is entered into this array) then would this be a "safe" way to use this function?

Safe in the sense that I'm not likely to get hard faults...

I wi try this in a couple of hours when I can get to the PC.

Thanks

EDIT: I meant after setting it to static...

Sent from my SM-N960F using Tapatalk

[T3sl4co1l]

Right, it needs to be allocated elsewhere.

To do this in OOP+GC, you simply return the object and forget about it, no problem.  C is too simplistic for that method however.

The hidden steps of that operation, are: allocating the object on the heap (C's malloc), returning the pointer to the object, and later disposing of the object (free) when it falls out of scope.

If you ever forget to free the object, it gets stuck on the heap.  This is called a memory leak.  If you ever free an object then reuse its pointer, you'll be reading garbage, and randomly get anything from protection faults to possible code execution vulnerability (use-after-free).

So while you could malloc inside the function, return a pointer, and free once it's used, it's probably not a good idea.  Better not to have functions with side effects (namely, modifying the heap).

So the better way is to pass in a pointer to available memory.  In other languages, this is a pass-by-reference.  You'll need,

Code: [Select]

void read_data_8(uint16_t idx, uint16_t len, uint8_t * buf) { ... }

...

read_data_8(idx, len, &buf);

You could also break it into multiple accesses, which is ugly since those would be multiple different function calls and you might as well inline it or something.  But sometimes that's an okay way to go.

Tim

[Mechatrommer]

If I cleared the outfiles array at the start of the function (by filling with NULL or NULL terminating each time data is entered into this array) then would this be a "safe" way to use this function?
Safe in the sense that I'm not likely to get hard faults...

no, the point is outfiles is only temporary allocated during the life of read_data_8 function, once the program goes out (exit) of read_data_8, the outfiles will be technically terminated (invalid) you are lucky your compiler is clever enough to zero the returned value, otherwise as other said, debugging adventure because the why your program crashed on another part of your code, or that out of boundary memory exploit hack that everybody slag about C/C++. the only solution is to expand the context or lifetime of the returned pointer (outfiles), ie by declaring it global or static as above poster suggested, or use pointer returned by malloc (if your compiler ever support it) but then thats going to be fun tracing alive memory to manually terminate them.

[noname4me]

Code: [Select]

uint8_t TESTMENOW[32];
read_data_8(BT_NAME, TESTMENOW,3);

int t1 = TESTMENOW[0];
int t2 = TESTMENOW[1];
int t3 = TESTMENOW[2];


and

Code: [Select]

uint8_t * read_data_8(uint16_t idx, uint8_t outfiles[], uint16_t len)
{

union d8a_t temp;
//uint8_t outfiles[32];
__disable_irq();
EE_Reads(idx, len, &temp.bigdata);
__enable_irq();

memcpy(outfiles, temp.chunks, (strlen(temp.chunks)));

return outfiles;
}
now returns my numbers back to me

Thanks for all your help - very appreciated

[Mechatrommer]

whats the purpose of returning outfiles then on the latest code? if its already manipulated in the function which will already take effect when exiting function. its just a waste one line of code, remove that and make the function of type void to avoid further waste on stack allocation (in case compiler is not smart enough). there are few more discussions for improvement such as reusability, but since you think you fixed it, so i think this one is concluded, ymmv and have a nice day.

[noname4me]

Lol... sorry... it does nothing because I don't store it anywhere as a returned value.

It can be deleted and the code works the same

Btw: any tips you can give would be welcome,  I'm always up for improvement

Sent from my SM-N960F using Tapatalk

[NorthGuy]

Code: [Select]

uint8_t * read_data_8(uint16_t idx, uint8_t outfiles[], uint16_t len)
{

union d8a_t temp;
//uint8_t outfiles[32];
__disable_irq();
EE_Reads(idx, len, &temp.bigdata);
__enable_irq();

memcpy(outfiles, temp.chunks, (strlen(temp.chunks)));

return outfiles;
}

Why not simply:

Code: [Select]

uint8_t * read_data_8(uint16_t idx, uint8_t outfiles[], uint16_t len)
{
__disable_irq();
EE_Reads(idx, len, outfiles);
__enable_irq();
return outfiles;
}
?

[obiwanjacobi]

A general read pattern like this usually returns the number of bytes placed in the buffer (array) passed into the function. In scenarios where you read from a long stream for instance, you can transfer the data in chunks (buffers/arrays) of a certain length. When you reach the end of the stream, the passed in buffer may not be filled all the way, which can be detected by the number of bytes returned from the function.

(in your case source can be omitted)
int read([void* source], byte* destination, int destMaxLength, int numberOfBytesToRead)

[2c]

[T3sl4co1l]

Why not simply:

Code: [Select]

uint8_t * read_data_8(uint16_t idx, uint8_t outfiles[], uint16_t len)
{
__disable_irq();
EE_Reads(idx, len, outfiles);
__enable_irq();
return outfiles;
}
?

And for that matter, why not simply inline it, or use a #define as shortcut?

Code: [Select]

#define read_data_8(idx, outfiles, len) \
__disable_irq(); \
EE_Reads((idx), (len), (outfiles)); \
__enable_irq();

(Or to cover more pathological cases of syntax, maybe those statements should be comma-separated, inside of parenthesis, with a final void statement to remove a return value.)

Which goes back to what I mentioned earlier, "you might as well inline it".

If there's more to the function than what we see here -- this is just a small part for example purposes -- keeping it in a function would be justified of course.

By the way, at -O3 and -Os optimization, automatic function inlining will be attempted, and you don't have to actually write it inline yourself; the compiler will figure out the best way.

Tim

[Mechatrommer]

And for that matter, why not simply inline it, or use a #define as shortcut?

Code: [Select]

#define read_data_8(idx, outfiles, len) \
__disable_irq(); \
EE_Reads((idx), (len), (outfiles)); \
__enable_irq();


imho that is one archaic way of coding, harder to maintain even to re-type / edit. and some debugger will not bother to step into each line and function within the #define body making debugging work even harder. as you mentioned, -O3 and -Os option already can figure it out, or at least explicitly use "inline" keyword in the function if we want to highlight (to other programmers) that the function should be better inlined (even if compiler choose not to). but then we dont know how many times the OP will make calls to this function, too many calls will increase compiled code by few bytes, not good if space is critical, but otherwise can be a good thing for performance when space is abundant. i did like what you've suggested once, but it had very specific purpose, more like construct codes or shorcuts, but mere inlining and maintainability are not among the purposes.

[Nominal Animal]

Given that the data is actually an array of uint32_t's, you'll want to write the function as

Code: [Select]

static inline void read_data(uint16_t  idx,
                             uint32_t *data,
                             uint16_t  bytes)
{
    __disable_irq();
    EE_Reads(idx, bytes, (uint8_t *)data);
    __enable_irq();
}
so that to read 5 values from index idx to a buffer buffer[], you use

Code: [Select]

        uint32_t  buffer[5];

        read_data(idx, buffer, sizeof buffer);
If you wanted to read buffer[1] and buffer[2] only, use read_data(idx, buffer + 1, 2 * sizeof buffer[0]).

Here are the reasons:

• Functions marked static inline are embedded in their call sites (if possible), making them as fast as preprocessor macros.
  
• On most architectures, __disable_irq() and __enable_irq() compile to a single machine instruction.  As the function is just a call to another function, embedding (more properly, inlining) makes sense code size-wise too.
  
• Instead of a temporary buffer, you can just cast the pointer to the desired type, because the other type is effectively a pointer to unsigned char.  (While this is a form of type punning, that cast is actually used to refer to the storage representation of the data you're interested in.  If you wonder about the standards conformance of this, see why and how memcpy() can be used to copy the storage representation of any data type.)
  
• By using proper function parameter types, you help the programmer at compile time, as the types of the parameters are checked.
• When specifying arrays by their names, they will automatically decay to a pointer to the first element of the array.  This means that &(buffer[1]) and buffer+1 evaluate to the exact same thing, to a pointer to the second element in the buffer array.
  
• The Principle of Least Surprise.  This interface is similar to other read()-like functions, making it familiar to programmers; so, they're less likely to be surprised by its behaviour and accidentally write buggy code due to wrong assumptions.
  

[langwadt]

Why not simply:

Code: [Select]

uint8_t * read_data_8(uint16_t idx, uint8_t outfiles[], uint16_t len)
{
__disable_irq();
EE_Reads(idx, len, outfiles);
__enable_irq();
return outfiles;
}
?

And for that matter, why not simply inline it, or use a #define as shortcut?

Code: [Select]

#define read_data_8(idx, outfiles, len) \
__disable_irq(); \
EE_Reads((idx), (len), (outfiles)); \
__enable_irq();


just don't call it anywhere interrupts are not enabled

[golden_labels]

Actually: don’t call it at all. In some contexts it will introduce syntax error or change program flow in a non-obvious way — including executing code that should never been executed according to what a human sees in the code.

Macros are not functions. This is a literal text replacement engine, that has no idea about C syntax. The macro should be at least defined like that:

Code: [Select]

#define RAD_DATA_8(idx, outfiles, len) do {\
        __disable_irq();
EE_Reads((idx), (len), (outfiles));
__enable_irq();
    } while (false)However, while in this particular case it will work, beware: this is still a horrible trap in more complex situations. For example you are not allowed to ever reference a parameter more than once — because that may lead to multiple invocations of code with side effects. Unless it is documented as a name or a part of a name, which implies only referencing a variable or function. But that exception has exceptions to itself: for examle volatile variables (performance issues, which are not obvious from what the invocation suggests; inconsistent value of a variable within the macro expansion), names of functions with side effects or accessing volatile variables themselves (value inconsistencies; multiple side-effects of seemingly single invocation) etc. And I bet that this exception to exception has exceptions itself, perhaps with exception with exceptions to exceptions. And some additional gotchas.

Since a macro is just replacing text, the source code effectively contains multiple copies of similar pieces of code. The compiler may have hard time optimizing that, leading to performance or memory footprint problems.

tl;dr: just use proper C abstractions and avoid text replacement as much as possible. You will save yourself a lot of ugly problems. Unless you understand what are the actual outcomes of the changes you make — that includes at least ability to properly profile code read raw assembly.

[T3sl4co1l]

just don't call it anywhere interrupts are not enabled

Ah yes, quite true; true of the original function, too.

That's always been one annoying thing about atomic operations, on many platforms -- it's often a big annoyance to save the interrupt flag(s), especially when you just need some teeny operation.

Speaking of teeny, depending on how long EE_Reads() takes, you might not want to disable interrupts at all -- the greatly (and randomly) increased interrupt latency may cause problems elsewhere.  Or, find a better way of handling it altogether (say, an asynchronous model: request a read operation, when complete fire an interrupt and execute callback function).

Actually: don’t call it at all. In some contexts it will introduce syntax error or change program flow in a non-obvious way — including executing code that should never been executed according to what a human sees in the code.

Yes, quite, as I alluded to:

(Or to cover more pathological cases of syntax, maybe those statements should be comma-separated, inside of parenthesis, with a final void statement to remove a return value.)

Wrapping in a do-statement is the most common patch for this (which I forgot at the time).

Basically, avoid making pathological expressions and statements in general, and especially when using macros.  Easily avoided.

Long expressions aren't optimized either -- also worth keeping in mind.  My most recent example of this was a bit-shifting operation (specifically, converting from packed 5-6-5 RGB <> 24-bit RGB color): easy enough to write a one-liner of, but the result is basically unoptimized (>100 instructions I think it was).  Written in a more suggestive manner (using local variables in a loop structure) it was still pretty ugly, but significantly shorter and faster.

Tim

[IDEngineer]

It's terrible form to disable interrupts around a function call:

__disable_irq();
EE_Reads((idx), (len), (outfiles));
__enable_irq();

...for several reasons, not the least of which is that you're including all the function call overhead in the disable period. Even if it "works" today, the risk is high that someone will modify the interrupt system in the future, not realizing that interrupts can be off for a lengthy period of time. Yes, I know others have spoken of inlining functions, but it's almost certain you'd still be including code within the "interrupts off" periods that doesn't need that protection. You should work aggressively to minimize the clock cycles during which interrupts are turned off. Usually it's just one or two lines of code that really matter, and not everything that your (inlined) function does.

[SiliconWizard]

It's terrible form to disable interrupts around a function call:

__disable_irq();
EE_Reads((idx), (len), (outfiles));
__enable_irq();

...for several reasons, not the least of which is that you're including all the function call overhead in the disable period. Even if it "works" today, the risk is high that someone will modify the interrupt system in the future, not realizing that interrupts can be off for a lengthy period of time. Yes, I know others have spoken of inlining functions, but it's almost certain you'd still be including code within the "interrupts off" periods that doesn't need that protection. You should work aggressively to minimize the clock cycles during which interrupts are turned off. Usually it's just one or two lines of code that really matter, and not everything that your (inlined) function does.

True enough. And if you really have "no choice" doing that, I'd suggest at least adding a #warning directive (or similar), so that everytime you'd compile code calling that, the compiler would spit out a warning suggesting to be extra careful.

[mfro]

... To do this in OOP+GC, you simply return the object and forget about it, no problem.  C is too simplistic for that method however.

It's not.

Technically, you can do the exact same thing in C. Putting any efficiency concerns aside (returning a compound object from a function on the stack isn't exactly efficient), you just need to return a struct that wraps the array:

Code: [Select]

struct arr_struct {
    char array[20];
};

struct arr_struct return_an_array(void)
{
    struct arr_struct s = { .array = { 'a', 'b', 'c', '\0' } };

    return s;
}


[IDEngineer]

Technically, you can do the exact same thing in C. Putting any efficiency concerns aside (returning a compound object from a function on the stack isn't exactly efficient), you just need to return a struct that wraps the array....

Or, since it has to be a non-local object to pass between functions anyway, just use a pointer. Since the OP mentions "flash" this is likely an embedded environment, so minimizing stack space is a larger concern than it might otherwise be. And as you note, in any environment pushing/popping entire data structures is horribly inefficient compared to a pointer.

[Mechatrommer]

It's terrible form to disable interrupts around a function call:

__disable_irq();
EE_Reads((idx), (len), (outfiles));
__enable_irq();

then  where you suggest to put disable/enable interrupt calls? only in main() function? that will certainly put program in high constraint and hence difficult to expand anyway. if you extra taboo about it you may create your own protectied or guarded functions such as....

Code: [Select]

int my_disable_irq_count = 0; // global

my_disable_irq() {
if (!my_disable_irq_count) __disable_irq();
my_disable_irq_count++;
}

my_enable_irq() {
if (my_disable_irq_count == 1) __enable_irq();
if (my_disable_irq_count > 0) my_disable_irq_count--;
}
or vice versa, hence...

my_disable_irq();
EE_Reads((idx), (len), (outfiles));
my_enable_irq();

i heard some people said about performance penalty/overhead? well you asked for it, performance or safety, pick one. tell me any programming language that is as fast as C/C++/assembly yet as safe as ADA/Phyton/Java? there is no free lunch and dinner...

[NorthGuy]

tell me any programming language that is as fast as C/C++/assembly yet as safe as ADA/Phyton/Java? there is no free lunch and dinner...

As Benjamin Franklin said: "The one who agrees to trade Liberty for Safety, doesn't deserve neither Liberty nor Safety".

[langwadt]

It's terrible form to disable interrupts around a function call:

__disable_irq();
EE_Reads((idx), (len), (outfiles));
__enable_irq();

then  where you suggest to put disable/enable interrupt calls? only in main() function? that will certainly put program in high constraint and hence difficult to expand anyway. if you extra taboo about it you may create your own protectied or guarded functions such as....

inside the EE_Reads function around the lines that actually need it, no need to have disable during the
overhead for calling and returning from the function

[IDEngineer]

tell me any programming language that is as fast as C/C++/assembly yet as safe as ADA/Phyton/Java?

"Safety" is mostly a function {wink} of the person behind the keyboard, not the choice of language. Many may find this shocking, but it is actually possible to write "safe" code in Assembly, or C, or C++.

"Safe" languages are attempts to make programming errors less likely or less harmful. But they are most definitely NOT a requirement to achieve "safe" code. And by the way, they are definitely not a guarantee of "good" code either.

[IDEngineer]

inside the EE_Reads function around the lines that actually need it, no need to have disable during the overhead for calling and returning from the function

Exactly.

Also, it's likely the actual number of instructions that need interrupt protection is extremely small. For example, in the Microchip PIC family the only thing time sensitive about writing to on-chip EEPROM/flash is the "magic sequence" that enables writes. The writes themselves are not time sensitive, and in fact complete asynchronously from the code that causes them (so the CPU can go off and do other things if desired while the write finishes).

Reading from EEPROM/flash has no timing constraints at all. I wonder if the target hardware in the OP's system really, actually needs interrupt protection for READS from EEPROM/flash....

[mikerj]

inside the EE_Reads function around the lines that actually need it, no need to have disable during the overhead for calling and returning from the function

Exactly.

Also, it's likely the actual number of instructions that need interrupt protection is extremely small. For example, in the Microchip PIC family the only thing time sensitive about writing to on-chip EEPROM/flash is the "magic sequence" that enables writes. The writes themselves are not time sensitive, and in fact complete asynchronously from the code that causes them (so the CPU can go off and do other things if desired while the write finishes).

Reading from EEPROM/flash has no timing constraints at all. I wonder if the target hardware in the OP's system really, actually needs interrupt protection for READS from EEPROM/flash....

As always care is needed given the limited context provided.  It's possible the OP is attempting to call EE_Reads() from both an interrupt handler and the main loop, in which case moving the interrupt enable/disable inside the function invites problems.

[IDEngineer]

It's possible the OP is attempting to call EE_Reads() from both an interrupt handler and the main loop, in which case moving the interrupt enable/disable inside the function invites problems.

If he's already inside the ISR, disabling interrupts won't change anything. Unless he's working in a multi-priority interrupt system, which I strongly suspect he's not given the tone of his questions.

A much bigger potential problem, if he's doing what you suggest, is that his ISR could invoke the function while the foreground is already in it. Is his read function safely re-entrant?

If he's trying to use non-reentrant code both inside and outside his ISR, he has a lot more to worry about besides passing a (potentially local, and thus locally deallocated) array on his stack which was his original problem. I would humbly submit that he's in way over his head and needs to seriously reconsider his overall architecture. It's very, very seldom that an ISR should be sharing executable code with the foreground, or calling functions at all. That's a pretty dense minefield for someone who doesn't think to pass a pointer instead of an entire array.

[noname4me]

Just to add a little about the application:

The reading and writing to the EEPROM (actually flash emulating EEPROM) on an STM32F103C8T6 is to be done only at the beginning of the application for setup/calibration functions.

The USB will be used to communicate with the application to pass parameters to the application to store/read from the EEPROM.

Otherwise, the application reads from ADCs about once every 250ms (interrupt handled), outputs a PWM signal, and spits out the readings over UART.  There are other bits that are changed in order to alter the state of connected hardware.

The EEPROM is read/written only in polling mode, because there was no requirement to read/write the EEPROM after the initial start of the application.  I disabled the interrupts because I was fearful of having the flash "unlocked" while the interrupts were active, just in case something unintended happened.

I will take on board the suggestion to move the disabling of the interrupts for only those functions that need to have the interrupts disabled - this will probably mean moving it to the EE_Writes() function, and removing it from the EE_Reads() function, but I will test this later today, as I will need to ensure that I am not opening the flash up to random writes.

The reason why I didn't pass the pointer in the first place, and used a returned array was because this was a library that I had downloaded online and was using in my own code.

When I tested the code some time ago, this worked as it was, so I did not think to refactor it in any way (don't fix what ain't broke) - but you are correct that I am a beginner in terms of programming.  My main focus is to build R&D protoypes that allow me to test other aspect of the design, the firmware is ancillary to that function.

All the tips, and discussions around it are really helpful to me to understand how I should be thinking about the code/system architecture, so all is appreciated  - like I said, any tips will be welcomed.

[NorthGuy]

The EEPROM is read/written only in polling mode, because there was no requirement to read/write the EEPROM after the initial start of the application.  I disabled the interrupts because I was fearful of having the flash "unlocked" while the interrupts were active, just in case something unintended happened.

How about you finish all the initialization first, then enable interrupts?

There's no need for each initialization piece to worry about interrupts. It is better to let all  your initialization code run with interrupts are disabled. Most importantly, the initialization that comes after you have read EEPROM, can still enjoy disabled interrupts (which is not the case with your current code).

[noname4me]

Hi,

I just looked again at the code:

During development I have a function "processUSB()" running in a while(1) loop so that I can test this function - actually, this code will not run on those boards on which this setup has already run (there will be a "setup performed" flag that once written to, will signal the code to skip this part of the application.

Long story short: Yes, this code can be run before interrupts are even enabled in the first place.

Do you know if the HAL USB implementation uses interrupts?

That would be the only reason why I would need interrupts enabled at the point this code will run...