Actuators receiving extra data

[AcidUK]

I have been trying to set up some basic sensors and outputs/actuators using mysensors. I am using the MQTT client gateway code from the dev branch, and using W5100 based ethernet shield on my gateway. My setup is essentially the same as http://www.makeuseof.com/tag/diy-smart-home-sensors-arduino-mysensors-openhab/

Everything works great, except now I have an LED I'm trying to send a value to (for dimming via PWM). The code works fine (I've used most of the code from http://www.mysensors.org/build/dimmer). A value is sent to MQTT, which prompts the gateway to send a message over the wireless chips to the node, and that converts it into an integer and maps that to the PWM output range. Of note this is using SoftSPI on the gateway.

Unfortunately the node is receiving more than it asked for. It is being sent, or is storing (I'm not sure which) some string data which is being appended to values. The values being sent are just Ints in the range of 0-100, with no zero padding.

At the start of the receive(const MyMessage &message) block I have added some debugging to print out the data being received (as I noticed the level values it was trying to set were randomly different).

  Serial.print("Received message, type: ");
  Serial.println(message.type);
  Serial.print("Message level: ");
  Serial.println(message.data );

For a message of '10' yields the values:

Received message, type: 3
Message level: 101mableLED

When the code runs atoi, it gets back a value of 101, which is >100 and so the value is set to 100.

For a value of '43' I get:

Message level: 401mableLED
Changing level to 100, from 100

For single digit values, I get the most recently set second digit before a 0 and 'mableLED'. Looking through the code, I see that this is part of the string '#define SN "DimmableLED"' which is set as part of the sketch info. If I comment this out, the string being padded on the end is instead '6.0-beta', and I'm not sure what that's coming from.

I'm not sure where to go next with troubleshooting, and I'd be grateful for anyone with more experience/insight. My guess is that the data being sent is not being null terminated, but it's only a guess.

[hek]

Might help if you have a look at the gateway log.

[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