Go Down

Topic: Function fails with passed in variable, but works if set manually. (Read 2152 times) previous topic - next topic

Apr 10, 2013, 07:07 pm Last Edit: Apr 10, 2013, 07:13 pm by allenhuffman Reason: 1
Greetings. I have an array of Flash strings, and I can show them directly like this:

Code: [Select]

Serial.println((_FlashStringHelper*)optCmd[0]);


I can also do that in a function:

Code: [Select]

void thisWorks(byte i)
{
 i = 0; // notice this override
 Serial.println((_FlashStringHelper*)optCmd[i]);
}


...but if I pass in a 0, and even verify it by printing it inside, it prints garbage:


Code: [Select]

void thisFails(byte i)
{
 // i = 0;
 Serial.println((_FlashStringHelper*)optCmd[i]);
}


I assume the preprocessor phase is doing something weird, but I have pruned this down to very simple code and cannot figure it out.

Anyone know what is going on?
Embedded Software Engineer
UNO | Leonardo | Due | Teensy | BASIC Stamp
http://www.subethasoftware.com

Erdin

What happens if you use the PGMT macro, http://arduino.cc/forum/index.php/topic,158375.msg1187237.html

I can't try a sketch, if I don't know what kind of strings. Both strings and array of pointers with PROGMEM and also with prog_uchar ?

Here is a full example. The main for loop works and properly prints the strings. But, if you uncomment the "//Serial.println()" in thisFails(), it starts printing garbage.

The duplicate print in "thisFails()" is what I was trying to do, but I have yet to get it to work. Simply having a Serial.println() causing it to not work clues me in that it may just be another oddity with the compiler preprocessor.

Code: [Select]

#include <avr/pgmspace.h>

const char string1[] PROGMEM = "This is string #1";
const char string2[] PROGMEM = "This is string #2";
const char string3[] PROGMEM = "This is string #3";

const char *flashArray[] PROGMEM = { string1, string2, string3 };

void thisFails(int i);

void setup()
{
  int i;

  Serial.begin(9600);
  while(!Serial);

  for (i=0; i<3; i++) {
    Serial.println((__FlashStringHelper*)flashArray[i]);
    thisFails(i);
  }

  showFreeRam();
}

void loop()
{
}

void thisFails(int i)
{
  // Uncomment out the next line, and it stops working.
  //Serial.println("Comment this line out and it works.");

  // This is the line I want to work, but it always fails. BUT,
  // if you manually pass a number in to the flashArray[0], it
  // works, or if you set "i=0;" before it, it works. Weird.
  //Serial.println((__FlashStringHelper*)flashArray[i]);
}

// End of FlashTest
Embedded Software Engineer
UNO | Leonardo | Due | Teensy | BASIC Stamp
http://www.subethasoftware.com

Erdin

Why do it wrong, if you can do it right ?
Did you take a look at the other thread, and did you try that PGMT() macro ?

Yes, I actually found that thread before when first researching PROGMEM. It is a very good one.

But this is actually a compiler question, not a PROGMEM question. My question here is what is the compiler doing to change things just by calling a Serial.println(). It's the third issue I have ran in to, with the last two being bugs requiring workarounds, and I am wondering if this is another one.

Looking at the source in Print.cpp, I see it has one that expects a "const __FlashSTringHelper *" and I am specifically casting to that. It looks like something may be trashing the stack or, perhaps, the preprocessor is misreading the source and changing the casting based on what it sees in the source code when that is there. (I am leaning to this second explanation, based on previous issues I have run in to.)
Embedded Software Engineer
UNO | Leonardo | Due | Teensy | BASIC Stamp
http://www.subethasoftware.com

SurferTim

If you remove the PROGMEM from this, it works.
Code: [Select]
const char *flashArray[] = { string1, string2, string3 };

Yep, looks like a compiler preprocessor thing.

Works:
Code: [Select]

Serial.println((__FlashStringHelper*)flashArray[i]);


Does not work:

Code: [Select]

Serial.println((__FlashStringHelper*)flashArray[i]);
delay(1);


I will have to dig in to the compiler code at some point and see if I can figure out what is going on there.
Embedded Software Engineer
UNO | Leonardo | Due | Teensy | BASIC Stamp
http://www.subethasoftware.com

Thanks, SurferTim. I actually have all of this working fine in my production code. I just noticed some very odd things and created this small example to replicate the problem.

With that array in PROGMEM, I have 1839 bytes free on my test program. With PROGMEM removed, it goes to 1833 bytes. The 6 bytes different is because each of those is a 2-byte pointer, so that makes sense. In my production code, I have a much much larger array of strings, and I am down to about 200 bytes free, so every little bit counts, which is what has me converting everything over to Flash storage right now.

It's just a weird thing. Everything works just fine, until code is added around it, then it breaks. I thought it had to do with the function() I was calling, but after I eliminated that I could break it without anything -- just by adding a "delay(1);" or similar around it.

I should probably learn how to read reported bugs, and how to submit ones I can't find already submitted.
Embedded Software Engineer
UNO | Leonardo | Due | Teensy | BASIC Stamp
http://www.subethasoftware.com

Erdin

I apologize, that I spoke too soon. It is a bug/problem of the gcc compiler.

It has to do with the PROGMEM.
http://www.arduino.cc/en/Reference/PROGMEM
It says: "However experiments have indicated that, in various versions of Arduino (having to do with GCC version), PROGMEM may work in one location and not in another".

So I narrowed it down to this:
Code: [Select]

// Arduino 1.5.2
// Arduino Uno

// This line makes it work, PROGMEM in front of the variable !
const char * PROGMEM flashArray[] = { string1, string2, string3 };

// This line makes if fail, PROGMEM after the variable !
// const char * flashArray[] PROGMEM = { string1, string2, string3 };


And this is the full sketch, tested on Arduino Uno.
Code: [Select]

// Arduino 1.5.2
// Arduino Uno

// No include of avr/pgmspace.h. Not needed.

#define PGMT( pgm_ptr ) ( reinterpret_cast< const __FlashStringHelper * >( pgm_ptr ) )

const char string1[] PROGMEM = "This is string #1";
const char string2[] PROGMEM = "This is string #2";
const char string3[] PROGMEM = "This is string #3";

// This line makes it work, PROGMEM in front of the variable !
const char * PROGMEM flashArray[] = { string1, string2, string3 };

// This line makes if fail, PROGMEM after the variable !
// const char * flashArray[] PROGMEM = { string1, string2, string3 };

// No function prototyping. Not needed.

void setup()
{
 Serial.begin(9600);
 while(!Serial);     // For Leonardo only.

 for (int i=0; i<3; i++) {
   Serial.println((__FlashStringHelper*)flashArray[i]);
   thisFails(i);
 }
}

void loop()
{
}

void thisFails(int i)
{
 Serial.println((__FlashStringHelper*)flashArray[i]);
}

Very good find! Thanks.

But, I just did some checking, and moving that keyword changes the memory usage by 6 bytes, which is the size of the pointers (three 2 byte pointers). I think placing it there is just placing those pointers in RAM, like not having PROGMEM at all, which explains why it works. (I just did a test. Same free RAM by not using PROGMEM.)

But I do believe this is a compiler thing. In my working code, I just retrieve the address using pgm_read_word() and pass that in:

Code: [Select]

Serial.println((__FlashStringHelper*)pgm_read_word(flashArray+i));


But, if this one thing gets fixed to work consistently, I want to go to doing something like this:

Code: [Select]

#ifdef USE_FLASH
#define FLASHSTR PROGMEM
#define FLASHCAST const __FileStringHelper*
#else
#define FLASHSTR
#define FLASHCAST char *
#endif


It's not quite flexible enough yet, but it allows me to have code that conditionally compiles to either put the strings in RAM (using FLASHSTR where we use PROGMEM now) or in Flash just by adding "#define USE_FLASH".

If I have the RAM to spare, I'd rather use it and save some clock cycles. But, if I get to the point where RAM is more important, then I can enable that #define and everything magically switches. I am doing that now, but I had to put more code in and #ifdef things that *should* work with the casting alone, so they either use char* or pgm_read_work(). It works, but is a bit more to maintain.

Thanks for that thread -- I will bookmark it.

And some other tidbit... The prototypes are sometimes required to work around what may be another bug. I had a specific problem with an error saying a function of mine was being redeclared differently, even though it was only used once, and matched parameters exactly with the declaration in the same file. Adding the prototype allowed it to compile. I'm pulling my hair out over these things :)

But ... it's great stuff, and the tools are free, and I am the last person to complain about free!

Thanks so much for the pointers!
Embedded Software Engineer
UNO | Leonardo | Due | Teensy | BASIC Stamp
http://www.subethasoftware.com

Erdin

Thanks for noticing that. I only checked the flash size and didn't check the ram size.
I tried a few more tests, and did some "avr-objdump -d" on the object file, but that didn't make things clear for me.

westfw

I have a hunch.  Try:
Code: [Select]
Serial.println((_FlashStringHelper*)  (  optCmd[i])  ) ;
I'd have to dig into docs to figure out if the extra parens are necessary, and think WAY to hard to figure out why it worked for the case with the constant 0 index, but I think the behavior is consistent with getting the cast vs array index order of operations wrong.

pYro_65

Code: [Select]
Serial.println((__FlashStringHelper*)flashArray[i]);

needs to be: ( your pointers to the strings are also in flash, get them first ):

Code: [Select]
Serial.println((__FlashStringHelper*) pgm_read_word( &flashArray[i] ) );

Also keep your vars const for fun ( pointer and var ):

Code: [Select]
const char * const flashArray[] PROGMEM = { string1, string2, string3 };

Finally if all else fails, replace PROGMEM to '__attribute__((section(".progmem.data.mysection")))'
volatile data can mess the sections up if mixed in the same file.

westfw, no change on the parens. I really believe it's a compiler issue. It works fine, 100% of the time, with just the call, but place something else around it, and it stops working. When I got down to just doing a "delay(1);" and it broke, I pretty much gave up.

I developed my own routines for handling these items when I ported an old program that expected 21,000 bytes of RAM to work on the Arduino's 2K (just an impractical amount of array storage to be useful), but I really thought I could simplify things once I found that all the Print:: functions were already handling __FlashStringStorage* (so why do it twice).

If I find anything new, I will share it here.
Embedded Software Engineer
UNO | Leonardo | Due | Teensy | BASIC Stamp
http://www.subethasoftware.com

pYro_65


I really believe it's a compiler issue.

Nope,

Because your flasharray is an array of progmem pointers, not data. You must use pgm_read_word to retrieve the pointer.

Code: [Select]
Serial.println((__FlashStringHelper*)flashArray[i]);

This is reading the i'th value in sram, you placed the data in PROGMEM. I can see that it works on occasion for you ( and my test ), but it is still wrong.

Use this:
Code: [Select]
Serial.println((__FlashStringHelper*) pgm_read_word( &flashArray[i] ) );

Or change your flasharray to this ( no PROGMEM ):

Code: [Select]
const char * flashArray[] = { string1, string2, string3 };

Go Up