Hi sorry for putting a bug in this forum. I am not quite sure how to use GitHub well enough but I am trying to learn!
Anyway the bug is in the String constructor. There is no constructor for the case of
String()
When you try to print (String()) the core will crash. The problem is trival to fix.
Add a constructor as follows:
String() {
init();
}
That would fix the bug. Oops I noticed just now that you have a constructor with default arguments that sets the string to:
String(const char * = “”) the problem is if i return a String() object the code seems to crash with it.
I changed it to String(const char* );
I have some ideas to shrink the flash memory size of the code and ram usuage. One of them is making certain spark_wiring_xxx optional. For instance I am not using SPI, I2C, Tone, USARTSERIAL. If you could make defines that would then remove them from the firmware it saves prescious space and RAM! I did so already in my code but I think it would be a great feature to have in the offical code.
The next topic is very minor but it saves a few bytes in the file: spark_wiring.cpp
You repeat the following code 3 times:
// SPI safety check
   #ifdef SPARK_WIRING_SPI
   if (SPI.isEnabled() == true && (pin == SCK || pin == MOSI || pin == MISO))
   {
       return false;
   }
   #endif
   // I2C safety check
   #ifdef SPARK_WIRING_I2C
   if (Wire.isEnabled() == true && (pin == SCL || pin == SDA))
   {
       return false;
   }
   #endif
   // Serial1 safety check
   #ifdef SPARK_WIRING_USARTERIAL
   if (Serial1.isEnabled() == true && (pin == RX || pin == TX))
   {
       return false;
   }
   #endif
Please note the #ifdef are my own attempt at isolating the unused libraries and are not in the original firmware.
I put all the code in a single function that returns a bool and it saves a surprising amount of flash memory.
yes i thought that lately too, all of the spark_XXX files get compiled - not sure if they're linked or if there is already logic to stop that, but it does seem like overkill. arduino does the same thing but just doesn't link unused stuff into the hex.
@fearless40 have you checked that your changes actually shrink the firmware files?
I bet there are a lot of cases where this kind of thing can happen. Maybe it would be better since we have limited FLASH and RAM to just leave this up to the user to not write bad code. At least when the core crashes, you should get some kind of error code to give you some idea of what's going wrong. I realize it might not be very easy to find the source of the problem. Especially if you code up a whole bunch of stuff and THEN test it all at once. I like to code piece by piece and test along the way, building up the complexity.
Right because if it didn't the flash size would be insane! Maybe >256k ?
You are correct the linker won't link in un-used code. The problem is that the code is used! There is a lot of extern Class something or other. For example look at this header file: spark_wiring_i2c.h
The code is linked in because how application.h is used and plus they use it in the file spark_wiring.cpp
I know removing the code works because before I started I was getting out of heap errors before doing some massive parring down of the lib and changing overly large buffers in the firmware.
I agree not having bad code is good to enforce but you should be able to return a null string and send it to a print function without a crash. It makes functions that return strings much easier to write.
for example:
String DoSomethingWithThisValue(char c) {
switch (c) {
case 'a'
return "Adam";
case 'b'
return "Bob";
default:
return String();
}
}
I have written a reasonable implementation of a Telnet Server. ( I will release it as a library shortly )
To make the server as easy to implement or upgrade as possible I have seperated out the negotiation from the rest of the handling of the data.
There is code that goes something like this (I don’t have it on hand as it is at home):
String BasicTelnetServerHandler::Do( char * text )
{
// Ignore the input but must return something
return String();
// I used to have no return value but that was also causing the compiler some issues of returning un-initialized strings
}
class Server : public TCPServer{
OnCommand() {
switch (command) {
case DO:
print( BasicTelnetServerHandler::Do(char * text) );
// the print commands comes from TCPServer
}
}
Clearly this isn’t valid C++ code but it shows you want I am doing. I could work around the un-initialized string by adding some extra if/then statements but that is less elegant and adds code bloat.
print should do nothing on an empty string and not fail.
In case you were wondering why Telnet Server, because I wan’t to debug my core without having any wires attached
String BasicTelnetServerHandler::Do( char * text )
{
// Ignore the input but must return something
return String(NULL);
// I used to have no return value but that was also causing the compiler some issues of returning un-initialized strings
}
Then yes it will work so in the end String(NULL) will work and String() won’t work.
Plus String() actually does less work in the end avoiding 2/4 copies of a byte which is really nothing and means little. But why not
Your example still doesn’t make sense to me. What is the intended input/output from the Do() method? If one of the cases is ignoring the input (still not sure what this means) then I’m thinking it shouldn’t return a NULL string since you are just directly using the output of that method in your print() command without error checking.
Without knowing how it’s all supposed to work, a suggestion might be: return ""; or return String("");
With more data I can formulate a better course of action
OK, so I completely agree with @BDub that there must be a better way to do what you want to do. Using empty strings as sentinels seems wrong to me.
I think you are misreading the String constructor. Since the call to strlen() is inside the if test which tests the pointer (cstr) for NULL, then strlen() is never called. That is the purpose of the if statement and I why I suggested it. The net result is a constructor that just calls init(); like you wanted.
Oh you are completely correct I missed the if(cstr)! Thanks for pointing it out.
Option 1: What I have (this is again pseudo code not actual code from my classes)
String BasicTelnetServerHandler::Do( char text )
{
switch (text) {
case ECHO: return String("\xFF\xFD\x01"); // IAC DONT ECHO
default: return String();
}
}
class Server : public TCPServer{
OnCommand() {
switch (command) {
case DO:
print( BasicTelnetServerHandler::Do(text) );
// the print commands comes from TCPServer
}
}
Option 2: Error Checking version
String BasicTelnetServerHandler::Do( char text )
{
switch (text) {
case ECHO: return String("\xFF\xFD\x01"); // IAC DONT ECHO
default: return String();
}
}
class Server : public TCPServer{
OnCommand() {
switch (command) {
case DO:
String g= BasicTelnetServerHandler::Do(text)
if( g.length > 0 ) print(g);
return;
}
}
What is the reason for the extra error checking. Print works by looking at the String::len and if len == 0 will promptly exit. What would be the point in putting in extra if statements for no better purpose. It seems like a waste of time and code bloat and makes things less legible.
@BDub
Writing the following would all be equivalent:
String someFunction() {
return “” vs return String() vs return String(NULL) vs return String("")
}
However for whatever reason the following would cause a hard fault:
String weirdFunction() { return; } <-- Hard fault
String weirdFunction() { return String(); } <-- Hard fault
String weirdFunction() { return String(“Hello”); } <-- OK
TCPServer::print( weirdFunction() );
Now maybe it was my code in some weird way causing a memory overwrite somewhere and fault but un-commenting a single return value from return String() to return String(“somevalue”); stopped the hard fault. Yes I could probably change it to String(NULL), String("") <-- This is the default behavior btw but it would continue to crash.
Or it could be a bad flash and poor code but it continued to happen multiple times even after multiple flashes until I added the extra constructor and made a change to the default constructor of String(const char * chr = “” );
And going back to C++ conventions having a default empty constructor is usually a good idea. Take std::string. It has a constructor for string(); Look at std::basic_string().
If you really don’t want the default constructor then it should be specified as private.
Sure std::string and company are great, but this is the Arduino compatible String class and like I said, it does not appear to be doc’ed or written to work that way over there, so it hasn’t been considered over here.
I agree it can be improved here, but as @BDub points out, I think you are the first person to run into this because you have an unusual coding situation.
If this is crashing still, then you probably have some issue with print() ? This is why I was suggesting the error checking... You could also force a known error message return String("ERR"); and then test for that instead of a zero length string.
I'm not sure what the mechanism is, but the Core will hard fault when trying to access a NULL pointer.
I agree. I also found a weird bug in the compiler:
I had code that was just a stub but looked as follows:
String stubFunction() {
// To do: do something with a return value
}
The code was being called and would crash the core.
The other interesting thing was the the compiler did not Warn me that I was not returning a value. Kind of weird.
What is going on is an uninitiated string is returned. This also causes a crash of the core. It has the side effect of printing random memory.
So yeah their may be a bug in print or in String or in the compiler.
