Bug in PubSubClient::connected

[securelpb]

The code for PubSubClient::connected has a logic error related to mixing purposes of the return value.

The code below is taken from the 2.8 release. The method is supposed to return a boolean. If the client is NULL will return false. If the client is not NULL and (int)_client->connected() returns a non-zero value (which means it is connected), then it will go to the else clause and return MQTT_CONNECTED which is defined as a 0.

false and 0 are equivents. Non-zero is true. So MQTT_CONNECTED is essentially defined as false, so when you use boolean PubSubClient::connected() to determine if you are connected you will always get a false.

boolean PubSubClient::connected() {
    boolean rc;
    if (_client == NULL ) {
        rc = false;
    } else {
        rc = (int)_client->connected();
        if (!rc) {
            if (this->_state == MQTT_CONNECTED) {
                this->_state = MQTT_CONNECTION_LOST;
                _client->flush();
                _client->stop();
            }
        } else {
            return this->_state == MQTT_CONNECTED;
        }
    }
    return rc;
}

The quick hack would be to change the defines so that MQTT_CONNECTED is not ZERO, but that would just perpetuate the back coding practice of mixing ints and bools. A more straight forward solution would be to change the contract of this method and have it return an int or add new method called connectoinStatus that returns values from this list:

// Possible values for client.state()
#define MQTT_CONNECTION_TIMEOUT     -4
#define MQTT_CONNECTION_LOST        -3
#define MQTT_CONNECT_FAILED         -2
#define MQTT_DISCONNECTED           -1
#define MQTT_CONNECTED               0
#define MQTT_CONNECT_BAD_PROTOCOL    1
#define MQTT_CONNECT_BAD_CLIENT_ID   2
#define MQTT_CONNECT_UNAVAILABLE     3
#define MQTT_CONNECT_BAD_CREDENTIALS 4
#define MQTT_CONNECT_UNAUTHORIZED    

[securelpb]

Oh the shame. I misread return this->_state == MQTT_CONNECTED; which would return a true. I'll see myself out.