Fixed uninitialized value for this->ip

[dguerizec]

When connecting with a domain instead of an ip (or an ip in a string like "192.168.1.2"), the member variable this->ip was not initialized.

Later on, calling connect() would try to connect with this uninitialized ip and ignoring domain, thus connection was failing.

[hirotakaster]

Hi,
If you want to use IP address, use like this.

uint8_t server[] = { 192, 168.2, 1};
MQTT client(server, 1883, callback);

ip member is "uint8_t *ip" can't use for string ("192.168.1.2").

[dguerizec]

Hello,

I know I can use {192, 168, 1, 2} but I want to use "192.168.1.2" for several reasons:

• it's more visual, it looks more like an IP address
• if I get it from another source (like Particle.variable), it's easier to get a string and I don't want to add code to parse and check if it's an IP or a domain
• and last reason, it works

The patch I submitted really doesn't change anything if {192, 168, 1, 2} is used, but allows using the form "192.168.1.2" also.

regards,
David

[hirotakaster]

Hi @dguerizec ,

Oh, I see now what you want.
That's right, you can use "192.168.1.2" string without no change on source code.

may be you call this function.

    MQTT(char* domain, uint16_t port, void (*callback)(char*,uint8_t*,unsigned int)

first parameter domain can use for IP address string.

[dguerizec]

Exactly, but it doesn't work since commit 18b6672

[hirotakaster]

Hi @dguerizec ,

Now I check and work fine.
My environment is Particle Photon, firmware is default 0.5.3, MQTT library is 0.4.2 on Particle WebIDE.

Please check your environment.

[dguerizec]

Well, an uninitialized variable can work fine depending on the environment, but I assure you it doesn't work with mine :)

I'm not using the Particle IDE, but my own build system with firmware version 0.5.2 and MQTT library from github master.

My compiler is:
arm-none-eabi-gcc (GNU Tools for ARM Embedded Processors) 4.9.3 20150529 (release) [ARM/embedded-4_9-branch revision 227977]

[dguerizec]

Maybe the main difference comes from the way I initialize my MQTT object.

I'm doing (here):
MQTT mqtt = MQTT((char *)host, port, cb);

While you do:
MQTT client((char *)host, port, cb);

I checked the code and there is one initialization of this->ip to NULL, but it's in the default constructor and I guess it's never called either way.

The initialization of this->ip should occur in initialize, even if ip is NULL. Otherwise this->ip could contain garbage and the test here will fail and try to connect to garbage IP.

[hirotakaster]

Okay, but if merge this code part, I think the bottom of a problem is other reason.

MQTT mqtt = MQTT client("192.168.10.10", 1883, callback);
...
// in initialize function , this part call.
    if (domain != NULL)
        this->domain = domain;
...
//  in MQTT.connect function, this TCP connection point is called.
        if (ip == NULL)
            result = _client->connect(this->domain.c_str(), this->port);

I think this point maybe problem. Following is on the source code.

[hirotakaster]

Try to change to the following.

MQTT mqtt = MQTT client("192.168.10.10", 1883, callback);
...
// in initialize function , this part call.
    if (domain != NULL)
        this->domain = String(domain);
...
//  in MQTT.connect function, this TCP connection point is called.
        if (ip == NULL)
            result = _client->connect(this->domain, this->port);

I checked like this now on the WebIDE lib environment, works well.

[dguerizec]

The problem I experienced is not on the domain part, but on the uninitialized ip member variable.

Another possibility for the fix could be to set ip to NULL in MQTT.h line 113:
uint8_t *ip = NULL;

[hirotakaster]

That's copy constructor is needed. But I don't define copy constructor.

[dguerizec]

OK, I think I see your point.

I'll check that when I get home.

[jotathebest]

Hello, I tested this on line 121 at MQTT.h file:

uint8_t *ip=NULL;

As on the connect() method you verify if the IP is NULL I think that it should be NULL by default at least that you set it properly on the constructor.

@hirotakaster Is it possible to close and merge this pr? What do you think?