Code executes differently on Photon than on Core

I was thinking that either the Time.timeStr() function is behaving differently on Core vs Photon, with something going on with the null termination, or maybe concatenating onto a null-terminated string behaves differently?

I bet @dougal is correct. However my photon hasn’t arrived yet to test…

I’d go for the first one, since doing

        bufferReadout += "Z (epoch:";
	bufferReadout += dest[3];
	bufferReadout += "Z) ";
        bufferReadout += Time.timeStr(index);

which does work and still is concatenating onto a null-terminated string (but not a String to a string/String).

But since timeStr() returns a String instance, trying bufferReadout += Time.timeStr(index).c_str(); instead might reveal some issue with the String class - either the += operator overload or the (const char*) cast operator.

Or is it a right padded timeStr()?

@BDub, what do you think?

2 Likes

Yeah, it’s been too long since I dealt with the details of C. I thought that regular strings were null-terminated, but I couldn’t remember for sure. So yeah, likely a bug in how += is handled for the String class.

I’ve been using loosely-typed languages as a web developer so many years, I have to consult references when I’m trying to resolve variable casting and such in C/C++. :smile:

2 Likes

I tried the change suggested by BDUB. No joy - it is the same. I tried deleting the space before the Z (i.e. "Z (ep… vs " Z (ep… No joy - it is the same. I tired commenting out the string building lines right after bufferReadout += “Z (epoch:”; No joy - same result. I wanted to try some Serial.println() debugging but I found that my computer (Win 7) doesn’t have a device driver for the Photon and Windows can’t find one. Win device manager reports this as Other devices Photon with WiFi. I tried updating the driver both with a local search and an Internet search but cannot get a serial device driver into my computer - so No Joy here too. Finally, I tried ScruffR’s suggestion and guess what - IT WORKS!

ScruffR: what does this do and why do you think that it corrected the problem?
BDub: I’l love to call you. How do I get in touch with you directly?
All: Photon driver for Windows?

2 Likes

Updated Windows Driver for Core and Photon

@BobG, I guess you tried the suggestion to use c_str().

This function returns a const char* string instead of a String object.
While the += operator of the String class taking a const char* is proven to work as expected, the += operator taking a &String or &&String is not.
It’s a while since I checked the String implementation, but last time I looked the assignment and concat operators taking String where still marked “experimental”.

This is one of the reasons why I chose to stay away from String wherever possible (another is possible heap fragmentation ;-))

Thanks to everyone for their investigative inputs! We clearly have a regression from 0.3.4 to 0.4.0 - I’ll look into this today so we can roll out a fix for 0.4.1 if needed.

Hi @BobG

There are two bugs hiding in String right now that I have made Matt @mdma aware of. Here is some work-around code for you test–I am assuming dest[] is declared as int:

if(bufferReadout != "")  // skip empty log entries
{
	int index;

	// parse the comma delimited string into its substrings
	parser(bufferReadout);

	// format the sequence number and place into bufferReadout
	bufferReadout = "(S:";
	bufferReadout += (long)dest[1];
	bufferReadout += ")";

	// Determine message type
	if(dest[0] == "S")  	// sensor type message
	{

    	// format the sensor Name from the index
    	index = dest[2].toInt();
    	bufferReadout += sensorName[index];
    	bufferReadout += " tripped at ";
	}
	else    	// advisory type message
	{
    	bufferReadout += (long)dest[2];
    	bufferReadout += " detected at ";
	}

	// add in the timestamp

	index = dest[3].toInt();
        //work-around!
        String tmp = Time.timeStr(index);
	bufferReadout += tmp;
	bufferReadout += " Z (epoch:";
	bufferReadout += (long)dest[3];
	bufferReadout += "Z)";

}

bufferReadout.toCharArray(stringPtr, bufferReadout.length() + 1 );
stringPtr[bufferReadout.length() + 2] = '\0';

See if that works better and let us know.

Thanks all for the help. I am happy to hear that mdma will look into the regression issue, as the original code works fine for Core but not for Photon. I understand what ScruffR is saying; however, the String class concatenation function is supposed to work (it is documented as such) and thus it is best if it is treated as a bug and fixed (long term, I will use the “.c_str()” fix that ScruffR first suggested and works well for now).

C++ does not seem to have a Stringbuffer class, like e.g. Java, so I agree that this method of string building most likely causes heap fragmentation. Since C strings are mutable, I had once tried to initialize the String object with the longest possible string that my application would generate and then tried to overwrite this data with the real data. That didn’t work out, so I have accepted the potential for heap fragmentation for now. My current work is researching some application algorithms, so I am less concerned about neat and clean code at the moment, so long as it runs reliably (which it has been on Core). After this investigative stage is complete, I might take a pass back through the code and use low level C string functions as opposed to the String class to make this cleaner. But that will depend upon the ultimate outcome of the research.

Thanks again for all of your help and thanks, mdma, for treating this as a Particle bug.

@bko: thanks for making us aware of these bugs in the String class. However, this particular bug seems to be in the Time class (Time.timeStr() method, at least). ScruffR found this and his fix works, albeit it is a bug for the Particle folks to investigate since the original code works with on a Core.

Hi @BobG

When the integer epoch time is larger than 7 decimal digits, Photon will hard fault (SOS and one red flash). Why the Core does not do this is a mystery since the bug has been there for a while.

The anonymous String object concatenation is newly working after some compiler issues were worked out, but I think there is still a bug there.

@bko: strange that you encounter this. Here is a recent result from my Photon using the code discussed above:
(S:12)No movement detected at Wed Jun 3 02:51:09 2015 Z (epoch:1433299869Z)

The integer epoch time is 10 decimal digits long. I did not cast this into a long, although a long and an int are the same on a Core/Photon. But clearly there are some issues with the Time class that crept in when Core was ported to Photon.

What I did (effectively) is:

String timeStamp = "";
timeStamp += Time.now();

...... later on .....

int index;
index = timeStamp.toInt();
String message;
message = "";
message += Time.timeStr(index).c_str();  // ".c_str()" is bug fix per ScruffR
message += ........... // etc.

@ScruffR

I ran into the issue of the c_str() being required for string concatenation of values from Time.timeStr() or things get clipped.

I don’t know if this is a bug in the String class or something strange in the Time class.

Test code and results follow…

*#include "application.h"*


*/* executes once at startup */*
*void setup() {*
	  *Serial.begin(9600);*
*}*

*/* executes continuously after setup() runs */*
*void loop() {*

	 *String concatStr;*
	 *String preStr = "       Pre : ";*
	 *String midStr = "Mid";*
	 *String postStr = " : Post";*


	 *Serial.println("-------------------------------------------------------------");*

	 *Serial.println("Baseline");*
	 *concatStr = preStr + midStr + postStr;*
	 *Serial.println(concatStr);*

	 *Serial.println("timeStr Mid");*
	 *String timeStr = Time.timeStr();*
	 *concatStr = preStr + timeStr + postStr;*
	 *Serial.println(concatStr);*


	 *Serial.println("timeStr cloned Mid");*
	 *String timeStr2 = timeStr;*
	 *concatStr = preStr + timeStr2 + postStr;*
	 *Serial.println(concatStr);*

	 *Serial.println("timeStr Mid +=");*
	 *concatStr = preStr;*
	 *concatStr += timeStr;*
	 *concatStr += postStr;*
	 *Serial.println(concatStr);*

	 *Serial.println("post Mid +=");*
	 *concatStr = preStr;*
	 *concatStr += postStr;*
	 *concatStr += timeStr;*
	 *Serial.println(concatStr);*

	 *Serial.println("quoted date");*
	 *concatStr = preStr;*
	 *concatStr += "Sun Aug 23 03:22:42 2015";*
	 *concatStr += postStr;*
	 *Serial.println(concatStr);*


	  *char *ss=(char *)timeStr.c_str();*
	  *while(*ss!='\0')*
	   *{*
		  *char buf[256];*
	   *sprintf(buf,"%c --> %d\n",*ss,*ss);*
	   *Serial.println(buf);*
	   *ss++;*
	   *}*


	 *delay(1000);*
*}*

--------------------------------------------------------------------------------------------------

-------------------------------------------------------------
Baseline
       Pre : Mid : Post
timeStr Mid
       Pre : Sun Aug 23 03:35:26 2015
timeStr cloned Mid
       Pre : Sun Aug 23 03:35:26 2015
timeStr Mid +=
       Pre : Sun Aug 23 03:35:26 2015
post Mid +=
       Pre :  : PostSun Aug 23 03:35:26 2015
quoted date
       Pre : Sun Aug 23 03:22:42 2015 : Post
S --> 83

u --> 117

n --> 110

  --> 32

A --> 65

u --> 117

g --> 103

  --> 32

2 --> 50

3 --> 51

  --> 32

0 --> 48

3 --> 51

: --> 58

3 --> 51

5 --> 53

: --> 58

2 --> 50

6 --> 54

  --> 32

2 --> 50

0 --> 48

1 --> 49

5 --> 53

Hi @codiak

I tried to reformat your code and output sections, but I don’t get why you have all the *'s in the code. It makes it really hard to read since * is an operator in C. Maybe a better cut-and-paste would help us figure out what is going on.

The problem of concatenating String objects is fixed on Photon but not on Core yet.

@bko This code was built with 0.4.4 and tested on the photon.

Ignore he splats,ignore them if you want… It was a quick hack to print the timeStr character for character.

Bottom line bug statement is

If you start a string with the results for timeStr eveu after is truncatred.
If you put timeStr as the last concat it works
Again this is on the photon, 0.4.4 local…

Ive got a work around in my real code. What i provided is just to demo the issue

I can verify that the bug is still present in 0.4.4-rc3. Just to clarify – the string concatenation works fine when compiling for Core (always has); this has always been a Photon only issue and an issue specifically when trying to concatenate a substring onto the end of the string returned by Time.timeStr().

I think this might be related to the fact that we remove the trailing newline in the 0.4.x series of firmware? I’ll add a unit test to be sure.

Update: Sorry about this, but I’m pleased to say it’s now fixed. it was due to the way we were removing the unwanted newline in the asctime() function that timeStr() uses.

This is fixed in the PR linked to the issue. This will be merged into the develop branch soon and released to the compile server with the 0.4.5 release, planned Wednesday next week.

4 Likes

Is the 0.4.5 release available outside of dev ?