Actuators receiving extra data

[TimO]

The problem occured to me too yesterday. I was using the example from the latest development version. I didn't find the time for further investigation yet.

[Michael M]

I'm getting the same behavior with latest dev (as of 27.12.2015). I'm using ESP8266 mqtt client gateway.

Gateway:

0;0;3;0;9;Message arrived on topic: mysensorsgwin/2/1/1/1/3
0;0;3;0;9;send: 0-0-2-2 s=1,c=1,t=3,pt=0,l=2,sg=0,st=ok:50
0;0;3;0;9;read: 2-2-0 s=1,c=1,t=3,pt=0,l=2,sg=0:50
0;0;3;0;9;Sending message on topic: mysensorsgwout/2/1/1/1/3
0;0;3;0;9;read: 2-2-0 s=0,c=1,t=2,pt=2,l=2,sg=0:1
0;0;3;0;9;Sending message on topic: mysensorsgwout/2/0/1/0/2
0;0;3;0;9;read: 2-2-0 s=0,c=1,t=3,pt=2,l=2,sg=0:100
0;0;3;0;9;Sending message on topic: mysensorsgwout/2/0/1/0/3

Sensor:

read: 0-0-2 s=1,c=1,t=3,pt=0,l=2,sg=0:50
send: 2-2-0-0 s=1,c=1,t=3,pt=0,l=2,sg=0,st=ok:50
501mableLED
Changing level to 100, from 0
send: 2-2-0-0 s=0,c=1,t=2,pt=2,l=2,sg=0,st=ok:1
send: 2-2-0-0 s=0,c=1,t=3,pt=2,l=2,sg=0,st=ok:100```

[Bolliebol]

For me here the same.

When using a sensor ( door, PIR, temp) no problem. When using a relay I get the same as above.

Log from my gateway ( connected via Pimatic):

debug [pimatic-mysensors]: <- MySensorSwitch { sender: 7, sensor: 1, type: 2, value: '0' }
22:01:52debug [pimatic-mysensors]: <- I_LOG_MESSAGE 0;0;3;0;9;read: 7-7-0 s=1,c=1,t=2,pt=0,l=1:0
22:01:52debug [pimatic-mysensors]: <- MySensorSwitch { sender: 7, sensor: 1, type: 2, value: '0' }
22:01:52debug [pimatic-mysensors]: <- I_LOG_MESSAGE 0;0;3;0;9;read: 7-7-0 s=1,c=1,t=2,pt=0,l=1:0
22:01:52debug [pimatic-mysensors]: <- MySensorSwitch { sender: 7, sensor: 1, type: 2, value: '0' }
22:01:52debug [pimatic-mysensors]: <- I_LOG_MESSAGE 0;0;3;0;9;read: 7-7-0 s=1,c=1,t=2,pt=0,l=1:0

etc... etc....

and from my Relay:

read: 0-1-7 s=1,c=1,t=2,pt=0,l=1,sg=0:0
send: 7-7-0-0 s=1,c=1,t=2,pt=0,l=1,sg=0,st=ok:0
read: 0-1-7 s=1,c=1,t=2,pt=0,l=1,sg=0:0
send: 7-7-0-0 s=1,c=1,t=2,pt=0,l=1,sg=0,st=ok:0
read: 0-1-7 s=1,c=1,t=2,pt=0,l=1,sg=0:0
send: 7-7-0-0 s=1,c=1,t=2,pt=0,l=1,sg=0,st=ok:0

I am a newbee, but what can I do to help ?

[AcidUK]

@hek I've done my best to try and do some data collection/troubleshooting as per your suggestion.

I tried to work out whether the problem was with the gateway or the client. I think I've established that it is the client receiving the message, but I've exhausted my limited c++/googling ability.

I threw in lots of debug statements all over the place. At first I was convinced there was a problem with the section of incomingMQTT in MyGatewayTransportMQTTClient that inserts a null byte at the end of the payload, however I have confirmed this is working using debug(PSTR messages.

I dumped the contents of the payload pointer at the start of that function and at the end. At the start the payload was equal to '6/+/+', however at the end, it was changed to '6' (this was the value I sent for debugging).

Instead I turned my attention to MyTransport.cpp, and verified that a) the correct length was being returned from mGetLength(message), and b) that the payload being sent was exactly the right length/right amount of data. I did this with debug commands in the transportSendWrite function.

Everything seems fine from the gateway point of view, however when I turned my attention to the client, I found that this seems to be where the issue is. I put some debug statements in the transportProcess() function in MyTransport, and could see that while the client logging that it had received the correct length message in mGetLength(_msg), the length of _msg.data is set to whatever the longest string that has been sent previously is. It doesn't seem like a new _msg object is being made for each message, and the value of _msg.data isn't being cleared/correctly assigned to be just the length of the incoming payload as per mGetLength(_message).

Adding the line reading 'Message payload...' in the MyTransport.cpp below

debug(PSTR("read: %d-%d-%d s=%d,c=%d,t=%d,pt=%d,l=%d,sg=%d:%s\n"),
					sender, _msg.last, destination, _msg.sensor, mGetCommand(_msg), type, mGetPayloadType(_msg), mGetLength(_msg), mGetSigned(_msg), _msg.getString(_convBuf));
debug(PSTR("Message payload in MyTransport.cpp: %s\n"), _msg.data);

I get the following output:

read: 0-0-3 s=0,c=1,t=3,pt=0,l=1,sg=0:6
Message payload in MyTransport.cpp: 6.6.0-beta
Received message, type: 3
Message level: 6.6.0-beta
Message length: 10
Changing level to 6, from 0

As you can see, the l=1, shows that the client recognises the intended length of the message, but the message.data on which atoi() is called on by the example code for the Dimmable LED Actuator example (and also called on by the MyMessage::getInt() code), is then failing unpredictably based on previous messages.

I can't go any further because I don't know

• What assigns the value to MyMessage data
• Whether there is supposed to be some sort of clearing of this value between messages
• Enough about how the code/library works to do any further debugging.

I hope this has been helpful.

[hek]

Ok, so the length is correct and type is string. Calling getInt on this message will end up here:
https://github.com/mysensors/Arduino/blob/development/libraries/MySensors/core/MyMessage.cpp#L162

My guess is that this will return a messed up value when atoi is called on the data buffer.

You should probably do something like :

char buf[10];
int level = atoi(msg.getString(buf))

to get a correctly null terminated string before converting it.

Or perhaps we could patch the MyMessage.cpp inserting a null terminator character before calling atoi() to fix this once and for all, like this.

int16_t MyMessage::getInt() const {
	if (miGetPayloadType() == P_INT16) { 
		return iValue;
	} else if (miGetPayloadType() == P_STRING) {
        data[miGetLength()] = 0;
		return atoi(data);
	} else {
		return 0;
	}
}

If you have time to do a PR (where all getters has the null-terminator code) after verifying, I'd be glad.

[AcidUK]

You've nailed the issue, but unfortunately I'm not smart enough to get your code working. I tried making the changes and unfortunately I got the following error:

C:\Program Files (x86)\Arduino\libraries\MySensors/core/MyMessage.cpp:162:23: error: assignment of read-only location '((const MyMessage*)this)->MyMessage::<anonymous>.MyMessage::<anonymous union>::data[(((int)(((const MyMessage*)this)->MyMessage::version_length >> 3)) & 255)]'

   data[miGetLength()] = 0;
                       ^
exit status 1
Error compiling.

Having managed to wrap my head around the code in https://github.com/mysensors/Arduino/blob/development/libraries/MySensors/core/MyGatewayTransportMQTTClient.cpp#L120 which seems to be doing what I think you're trying to do, I edited MyMessage::getInt() to instead look like this:

int16_t MyMessage::getInt() const {
	if (miGetPayloadType() == P_INT16) { 
		return iValue;
	} else if (miGetPayloadType() == P_STRING) {
		// Null terminate string according to message length to prevent reading old bytes from data
		char* ca;
		ca = (char *) data;
		ca += miGetLength();
		*ca = '\0';
		
		return atoi(data);
	} else {
		return 0;
	}
}

I can confirm this has fixed the problem. I don't know if what I've done is hackish or nasty, I'm afraid I'm not versed in C/C++.

If you give me the thumbs up I'll download sourcetree and work out how to submit a pull request with this code copied + pasted into the getters? Given my limited ability would it make more sense for me to create an issue with all the information summarised from here so that someone smarter can do it properly?

I worry that you're trying to instruct a monkey to write a haiku by letting me try and fix this! P.S. thanks for all the help!

[hek]

@AcidUK said:

I worry that you're trying to instruct a monkey to write a haiku by letting me try and fix this! P.S. thanks for all the help!

Haha.. :)

I'll do it then...

[Michael M]

@AcidUK Perhaps I'm an earlier generation of monkey then you are, because I can't get it to work properly :) Could you please tell me what steps did you take to get this working? I've changed MyMessage.cpp to this:

/*
int16_t MyMessage::getInt() const {
	if (miGetPayloadType() == P_INT16) { 
		return iValue;
	} else if (miGetPayloadType() == P_STRING) {
		return atoi(data);
	} else {
		return 0;
	}
}
*/

int16_t MyMessage::getInt() const {
    if (miGetPayloadType() == P_INT16) { 
        return iValue;
    } else if (miGetPayloadType() == P_STRING) {
        // Null terminate string according to message length to prevent reading old bytes from data
        char* ca;
        ca = (char *) data;
        ca += miGetLength();
        *ca = '\0';
        
        return atoi(data);
    } else {
        return 0;
    }
}

Then I reflashed gateway and sensor and the result is the same as before:

read: 0-0-2 s=0,c=0,t=3,pt=0,l=2,sg=0:35
351mableLED
Changing level to 100, from 0

Am I missing something besides my programming skills ? :)

[AcidUK]

The current example code for the dimmable LED is calling the MyMessage data value directly. Make sure you change this line: https://github.com/mysensors/Arduino/blob/development/libraries/MySensors/examples/DimmableLEDActuator/DimmableLEDActuator.ino#L88 to

int requestedLevel = message.getInt();

So that it is properly using the getter. I don't know if hek was going to make the solution more elegantly, but at the very least that example code will need to change no matter what he does to the library.

[Michael M]

@AcidUK said:

int requestedLevel = message.getInt();

It works like a charm now :) Thank you

[hek]

Fixed by @tekka now
https://github.com/mysensors/Arduino/pull/317