2

I am having a problem when using a switch statement to check for something inside an array. I am using an int as my switch, then I compare the character arrays using strstr. So it reads like this:

void function(char *buffer, uint32_t size){
  char *p=(char*)malloc(compareSize); //compareSize is the size of the text to be compared
  #define TXTCOMPARE1 (char*)"Sample Text 1"
  #define TXTCOMPARE2 (char*)"Sample Text 2"
  #define TXTCOMPARE3 (char*)"Sample Text 3"
  switch(intCompare){
    case COMPARE1 :
      p = strstr(buffer,TXTCOMPARE1); break;
    case COMPARE2 :
      p = strstr(buffer,TXTCOMPARE2); break;
    case COMPARE3 :
      p = strstr(buffer,TXTCOMPARE3); break;
  }
  if(p!=NULL) { Serial.println("Success!"); }
  else { Serial.println("No match"); }
    memset(&p,0,sizeof(p);
}

This method does not work and p is always NULL. However, when I write it out without the switch() it compares just fine:

//using the same p declaration as above
p = strstr(buffer,TXTCOMPARE1);
if(p != NULL) { Serial.print("Success:"); }
else { Serial.print("No Match"); }

Why would the switch statement give me different results for what is basically the same function call?

Edit: Actually it seems to break everywhere when I set more than one call to p = strstr(...). When there is only one strstr it works, when two or more are used, it does not...

  • Have you defined COMPARE1, COMPARE2 and COMPARE3 ? Where is your value assigning statement for intCompare ? – Kugan Kumar Sep 10 '16 at 4:52
  • Yes, I appologize for leaving that out. But that is almost exactly what I'm doing, except that intCompare is being passed into function(char *buffer, uint32_t size, int intCompare). But when I have multiple cases, p is always NULL – Andrew Sep 10 '16 at 6:40
  • I don't understand three things in this code: 1) Why are you doing a malloc() into p, and then overwriting p before doing a free()? That is not what strstr() needs to work. 2) Why do the TXTCOMPARE strings have (char *) before them? This shouldn't be necessary. 3) Why are you doing a memset(&p,0,sizeof(p));? This is an extraordinarily long-winded way of saying p = NULL; - and then p goes out of scope anyway... – John Burger Sep 10 '16 at 8:30
  • Thanks @JohnBurger. 1) I'm trying to put in an expected size for p from the comparison text I am expecting. 2) The compiler was giving me warnings for converting the strings into char* when defining them, so I cast them as char*. Is there a better way to do this? 3)I thought I was trying to clear the p pointer. – Andrew Sep 11 '16 at 2:13
2

One big problem is that you forgot the break statements for those cases. So if it compares in the first one and finds it, you'll still get null because it is going to go look for the third one afterwards.

| improve this answer | |
0

I solved this problem, there were a few problems with the code that I found, so thanks for your comments. Basically, it lies in the #define# statements. First, in Arduino it's not such great practice to use #define for strings/chars, only for int. This is because the compiler applies the text every time they appear in the code, so it uses up a lot more memory. The problem here is solved by using static char TXTCOMPARE1 = {"Sample Text 1"};

Another problem was that there was no NULL terminated character at the end of the #define statement, so the program kept looking to the end of the stack when using strstr() and finding no termination, I think I was just getting lucky that it worked occasionally at all.

Furthermore, there is no need to clear p using memset. It needs to be declared as NULL and then reset after the switch by if(p){p = NULL;}.

And now it works!

| improve this answer | |
  • Good that you found it - and thank you for the caution about #define and strings. Note that you should have used static const char TXTCOMPARE1[] = "Sample Text 1";, and also that if (p) { p =NULL; } wastes an if. Just do p = NULL; - it's smaller and faster. Oh, and completely unnecessary. p is about to disappear anyway when it goes out of scope: no need to zero it out. – John Burger Sep 11 '16 at 5:51
  • 1
    Another problem was that there was no NULL terminated character at the end of the #define statement - that's not true. It's hard to tell from your snippet but a #define just does a textual replacement. – Nick Gammon Oct 12 '16 at 1:01
  • 2
    There is no need to malloc memory for p, just declare it as a char *p = NULL; strstr then returns a pointer to the existing strings. All the malloc line is doing is leaking memory. – Andrew Jan 10 '17 at 9:32
  • The statement "This is because the compiler applies the text every time they appear in the code, so it uses up a lot more memory" is wrong. No matter what declaration you use (#define, const char), the memory usage is the same. The C compiler take care of reusing the string when it appears for second time. Wrote a little test program and see how much RAM/program memory it use. – user31481 Aug 8 '17 at 19:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.