Pages: [1] 2   Go Down
Author Topic: Function fails with passed in variable, but works if set manually.  (Read 1706 times)
0 Members and 1 Guest are viewing this topic.
Des Moines, Iowa, USA
Offline Offline
Jr. Member
**
Karma: 0
Posts: 52
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

Greetings. I have an array of Flash strings, and I can show them directly like this:

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

I can also do that in a function:

Code:
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:
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?
« Last Edit: April 10, 2013, 12:13:33 pm by allenhuffman » Logged

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

Offline Offline
Edison Member
*
Karma: 58
Posts: 2078
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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 ?
Logged

Des Moines, Iowa, USA
Offline Offline
Jr. Member
**
Karma: 0
Posts: 52
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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:
#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
Logged

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

Offline Offline
Edison Member
*
Karma: 58
Posts: 2078
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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 ?
Logged

Des Moines, Iowa, USA
Offline Offline
Jr. Member
**
Karma: 0
Posts: 52
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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.)
Logged

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

Miramar Beach, Florida
Offline Offline
Faraday Member
**
Karma: 147
Posts: 6038
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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

Des Moines, Iowa, USA
Offline Offline
Jr. Member
**
Karma: 0
Posts: 52
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

Yep, looks like a compiler preprocessor thing.

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

Does not work:

Code:
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.
Logged

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

Des Moines, Iowa, USA
Offline Offline
Jr. Member
**
Karma: 0
Posts: 52
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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.
Logged

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

Offline Offline
Edison Member
*
Karma: 58
Posts: 2078
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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:
// 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:
// 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]);
}
« Last Edit: April 10, 2013, 05:10:33 pm by Erdin » Logged

Des Moines, Iowa, USA
Offline Offline
Jr. Member
**
Karma: 0
Posts: 52
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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:
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:
#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 smiley

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!
Logged

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

Offline Offline
Edison Member
*
Karma: 58
Posts: 2078
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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.
Logged

SF Bay Area (USA)
Offline Offline
Tesla Member
***
Karma: 134
Posts: 6762
Strongly opinionated, but not official!
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

I have a hunch.  Try:
Code:
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.
Logged

North Queensland, Australia
Offline Offline
Edison Member
*
Karma: 70
Posts: 2171
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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

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

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

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

Code:
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.
Logged


Des Moines, Iowa, USA
Offline Offline
Jr. Member
**
Karma: 0
Posts: 52
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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.
Logged

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

North Queensland, Australia
Offline Offline
Edison Member
*
Karma: 70
Posts: 2171
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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:
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:
Serial.println((__FlashStringHelper*) pgm_read_word( &flashArray[i] ) );

Or change your flasharray to this ( no PROGMEM ):

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


Pages: [1] 2   Go Up
Jump to: