Go Down

Topic: Interesting discovery - Can you figure out what's wrong? (Read 1 time) previous topic - next topic

Protoneer

  String test(char A,int C, String R)
  {
    if(C > 10)
      return A + R;
    else if(C < -10)
      return A + "-" + R;
  }

Notice  anything wrong?

This code causes my controller lock-up...
Delta 3D Printer with Auto Bed Leveling - http://www.kickstarter.com/projects/ttstam/openbeam-kossel-pro-a-new-type-of-3d-printer/posts
http://blog.Protoneer.co.nz

holmes4

Quote
Notice  anything wrong?


Yes you are - read the sticky at the top of the forum.

Quote
This code causes my controller lock-up...


We deal with arduinos here not controllers/pics/pis/ ect.

Mark

Protoneer


We deal with arduinos here not controllers/pics/pis/ ect.


I see... So an Arduino's not a Micro controller then?

But thanks for the friendly greetings and advice. ;)

Just want to point out that its worth ensuring all your code paths return values. In this case, the second "if" did not return a value at all times causing my Arduino "Micro Controller" to lock up.
(The IDE also doesn't warn you of this issue)

Exception handling would be nice for this, but too resource intensive for a MCU I take it..
Delta 3D Printer with Auto Bed Leveling - http://www.kickstarter.com/projects/ttstam/openbeam-kossel-pro-a-new-type-of-3d-printer/posts
http://blog.Protoneer.co.nz

MarkT

Unfortunately warnings are disabled in the Arduino compiler (presumably because it
confuses newcomers to programming), so missing return statements are not reported.
C/C++ does not require compilers to error-out on such broken dataflow - presumably
because of all the existing poorly written C code out there in the real world.
[ I won't respond to messages, use the forum please ]

mstanley


Unfortunately warnings are disabled in the Arduino compiler

More like "disabled by default".  If you go to File->Preferences and check the box for compilation next to Show verbose output during: then hit verify you get the following compiler output:

Blink.ino: In function 'String test(char, int, String)':
Blink.ino:32: warning: control reaches end of non-void function

Quote
(presumably because it confuses newcomers to programming)world.

I like to think that it protects those of us who like to help from programmers and newcomers who might have O.C.D. ;)

PeterH


Exception handling would be nice for this, but too resource intensive for a MCU I take it..


The Arduino doesn't have any concept of virtual memory management so it will not give you 'segmentation fault' errors and so on if your program accesses an invalid location, as would happen in this case if your function ends without returning a value the caller would then use whatever value happened to be left in the stack return frame. You will get similar problems if you use uninitialised pointers, array index values etc. The compiler used by Arduino is capable of warning about many of these errors but for some strange reason those warnings have been switched off by default.

As an aside, the String class you use in your example can also be problematic since it exposes a heap corruption problem in versions up to 1.0.4 and introduces the risk of heap fragmentation in all versions. In a system with such limited heap space and such limited ability to manage the heap, using dynamic (heap) memory to store dynamic data is not a smart approach. I recommend that you use c-strings (null-terminated char arrays) instead. There are no particular advantages to using the String class and IMO you would be better off forgetting that it exists.
I only provide help via the forum - please do not contact me for private consultancy.

Protoneer

Thanks for all the info...

I turned on verbose output and it pointed out some useful stuff. Also cleared up a few things I wondered about.

Regarding the String class, I do agree that its not the best way to go. My first thought was actually that using String was
causing the lock-ups.  :D

Delta 3D Printer with Auto Bed Leveling - http://www.kickstarter.com/projects/ttstam/openbeam-kossel-pro-a-new-type-of-3d-printer/posts
http://blog.Protoneer.co.nz

Protoneer


As an aside, the String class you use in your example can also be problematic since it exposes a heap corruption problem in versions up to 1.0.4 and introduces the risk of heap fragmentation in all versions. In a system with such limited heap space and such limited ability to manage the heap, using dynamic (heap) memory to store dynamic data is not a smart approach. I recommend that you use c-strings (null-terminated char arrays) instead. There are no particular advantages to using the String class and IMO you would be better off forgetting that it exists.


Peter, String has a huge advantages... :) It prevents a huge amount of frustration.

The function I am working on will generate GCode commands.. I just want to build simple string but I am finding it hard to even just add a char to char *.  :smiley-eek-blue:
Delta 3D Printer with Auto Bed Leveling - http://www.kickstarter.com/projects/ttstam/openbeam-kossel-pro-a-new-type-of-3d-printer/posts
http://blog.Protoneer.co.nz

Arrch


Peter, String has a huge advantages... :) It prevents a huge amount of frustration.

It also causes plenty of frustration as well.
Quote

The function I am working on will generate GCode commands.. I just want to build simple string but I am finding it hard to even just add a char to char *.  :smiley-eek-blue:

sprintf(), strcat(), etc.

PeterH


I am finding it hard to even just add a char to char *.  :smiley-eek-blue:


Typically it could be done something like this:
Code: [Select]

// append c to the string held in buffer
buffer[len++] = c;

// put a null terminator at the new end of the string
buffer[len] = '\0';


You will see plenty of examples of doing this sort of thing if you look for example sketches that read from the serial port.
I only provide help via the forum - please do not contact me for private consultancy.

Protoneer

thanks...

Yes.. I noticed that  a lot of people was doing it like that.

If I declare a Char Temp[100] and I insert 50 characters followed by a null terminator. Does that reserve the full 100 in memory or only the bit up to the null terminator?

thanks again for all the help.
Delta 3D Printer with Auto Bed Leveling - http://www.kickstarter.com/projects/ttstam/openbeam-kossel-pro-a-new-type-of-3d-printer/posts
http://blog.Protoneer.co.nz

Sembazuru



As an aside, the String class you use in your example can also be problematic since it exposes a heap corruption problem in versions up to 1.0.4 and introduces the risk of heap fragmentation in all versions. In a system with such limited heap space and such limited ability to manage the heap, using dynamic (heap) memory to store dynamic data is not a smart approach. I recommend that you use c-strings (null-terminated char arrays) instead. There are no particular advantages to using the String class and IMO you would be better off forgetting that it exists.


Peter, String has a huge advantages... :) It prevents a huge amount of frustration.

The function I am working on will generate GCode commands.. I just want to build simple string but I am finding it hard to even just add a char to char *.  :smiley-eek-blue:


Well, adding a char variable to a char string can be trivial if you know exactly where in the string the char variable goes by index. (Either by having an index variable, or knowing a rigid character position structure.) That's because it's just an array that can be accessed just like any other array. (You as the programmer just need to make sure there is a null (0x00, '\0', 0, 00, etc.) at the index position right after the last character of interest before trying to handle the char string as a singular entity.) If you want to put 'r' at the 10th position of char string outputBuffer, it's as simple as outputBuffer[9] = 'r';. If you are continually adding a new character, simply keep track of where you are in the char string with an index variable. Assign the new char at the current index location, increment the index, and insert a null at the new current index location. Wash, rinse, repeat for each new char to be appended to the char string. (Ok, yes, you also need to watch to make sure you don't try to go past the bounds of your char string...)

The C string libraries just make this easier for you as long as you can remember the abbreviation construction of the function names, or have http://www.cplusplus.com/reference/cstring/ bookmarked. (It's the latter for me...)
http://www.catb.org/jargon/html/I/I-didn-t-change-anything-.html

Nick Gammon



If I declare a Char Temp[100] and I insert 50 characters followed by a null terminator. Does that reserve the full 100 in memory or only the bit up to the null terminator?


It reserves the amount you declare.
Please post technical questions on the forum, not by personal message. Thanks!

More info:
http://www.gammon.com.au/electronics

PaulS

Quote
Typically it could be done something like this

I'm not sure that it is a good idea to give an example that makes any assumptions about what
Quote
I am finding it hard to even just add a char to char *.

means.

I find that people that make statements like this assume that pointers and arrays are the same thing. While there are strong ties between pointers and arrays, that is ONLY true when the pointer points to allocated memory. Far too often, newbies create a pointer that points to nothing, and then assume that they can use it as an array that holds as much as they want.

Rather than suggesting how they might do what they suggest in the question, I think it's better to ask them to post some code, so that it can be determined that the pointer in question does indeed point to allocated memory.

Protoneer

Code: [Select]

  struct position {
    int x;
    int y;
    int z;
    position(){ 0; 0; 0; }
    position(int X,int Y, int Z){ x = X; y = Y; z = Z; }
  } CurrentPosition, DefaultPosition;

  void GRBLCommand()
  {
    int XChange = CurrentPosition.x - DefaultPosition.x;
    int YChange = CurrentPosition.y - DefaultPosition.y;
    int ZChange = CurrentPosition.z - DefaultPosition.z;
   
    if(
        (XChange > 10 || XChange < -10) ||
        (YChange > 10 || YChange < -10) ||
        (ZChange > 10 || ZChange < -10)
      )
    {
      Serial.print("G1 ");
   
      // This can be shortened  to a method that takes the Change,Axis letter and returned the string.
      if(XChange < -10) Serial.print("X-1000");
      if(XChange > 10)  Serial.print("X1000");
     
      if(YChange < -10) Serial.print("Y-1000");
      if(YChange > 10)  Serial.print("Y1000");
     
      if(ZChange < -10) Serial.print("Z-1000");
      if(ZChange > 10)  Serial.print("Z1000");
     
      Serial.println("F100");
    }
    else
    {
      Serial.print("!");
    }
  }


This is my working code. My original idea was to create method that returned a string that gets printed with Serial.print();
I could not get that going because String kept on locking up my Arduino. Because I can't get strings to work properly I ended up just doing the Serial.print inside the method. (It works but I don't like it cause I am sure the code can look much simpler.)

Any ideas on how I can optimize this method and removing the dependency on Serial?

Thanks for everyone's help up to now.


Delta 3D Printer with Auto Bed Leveling - http://www.kickstarter.com/projects/ttstam/openbeam-kossel-pro-a-new-type-of-3d-printer/posts
http://blog.Protoneer.co.nz

Go Up