Factory should retain a reference to protocol for future reference.

[yombo]

Inside the factory, their should be a reference to the protocol that was built. This can be used by the factory or the service that created the factory to reference the protocol easier.

[astrorafael]

In my "eat your own food dog" use, I keep the protocol reference in the Service itself, which gets called with the protocol as soon as one is produced (see the gotProtocol() method in my examples).

The problem with your suggested change as is is that the factory now is able to produce protocols for several simultaneous MQTT broker connections and you should know which is which by the addr parameter. So, instead of a single protocol reference, you should have a dictionary of protocols, indexed by addr. For my own use, this is an unnecessary complication, but I'm willing to incorporate yours if it is useful for you.

Greetings Rafael

[yombo]

I'm not implementing this as a service. So, I've had to make a few adjustments...unless you see a better way to implement this. I don't get calls to "serv.whenConnected()". Is there a simple way to get the gotProtocol() method when not running as a service?

See: https://github.com/yombo/yombo-gateway/blob/develop/yombo/lib/mqtt.py#L198

This version is working, but with the latest tweaks to your code base.

[astrorafael]

If there is any, I just don't know it :-(

Thank you for your submission.

Before merging your tweaks, I'd like to propose you the following:

instead of

self.protocol = MQTTProtocol(self, addr)
return protocol

how about

self.protocol[addr] = MQTTProtocol(self, addr)
return protocol[addr]

With the later, you can have two protocols for two different brokers (this could be interesting in certain scenarios such as publishing some data to a private broker and also publishing public data to test.mosquitto.org

In this case, you need to update the factory constructor to

self.protocol = {}

[yombo]

I'll try your suggestion. I will not have time to complete that for about 48 hours. I will keep you posted of my findings.

Currently, i'm isolating the factory and protocol for each connection. This allows me to connect to different servers with different configurations...as I'm storing this information in the factory as I think that configuration should be there (i'm not an expert) and out of the protocol. This leaves the protocol clean of configuration data and allows it to just be a protocol that pulls information from the factory.

Do you think it's appropriate to store this information in the protocol instead? Here's what i'm currently doing (un-commited). I set the data up in the factory after it's created, but before the connectTCP/connectSSL calls. Then, I adjust the protocol with this:

class MQTTYomboProtocol(MQTTProtocol):

def connectionMade(self):  # Empty through stack of twisted and MQTT library

    # call the mqtt_client.mqtt_connectin function once fully connected. This allows to send queued messages.
    self._onMqttConnectionMade = self.factory.mqtt_client.mqtt_connected

    self.connect("Yombo-%s-v1" % self.factory.mqtt_client.client_id, keepalive=self.factory.keepalive,
        willTopic=self.factory.will_topic, willMessage=self.factory.will_message,
        willQoS=self.factory.will_qos, willRetain=self.factory.will_retain,
        username=self.factory.username, password=self.factory.password,
        cleanStart=self.factory.clean_start)

---

As far as my other commits go, they should be fine as I think it's just bug fixes.

[yombo]

I played around with this. Your suggestion to add addr just makes a pure mess of things. :-)

[astrorafael]

OK, leave as it is. I won't use it anyway

By the way, I think the correct code should be:

self.protocol = MQTTProtocol(self, addr)
return self.protocol  

Could you fix it before I merge it ?

[astrorafael]

I'm thinking on changing the callback interface after my return from holidays

Now I have thse setters setDisconnecCallback and setPublishCallback and now you have added _onMqttConnectionMade

These setters comes from my C++ background, but they are not very pythonic

I think the proper way should be declaring this as public attributes onDisconnectCallback, onPublishCallback and onMqttConnectionMade. This is much simpler.

Should later I decided for some reason to have control access on these attributes, I could use Python properties.

This would break your code. Would it be too much inconvenient ?

[yombo]

Correct, the setters should be gone. Change anything you want, I keep a local copy and have to manually import any changes...I've been bitten before and learned. :-)

See this: https://github.com/yombo/yombo-gateway/blob/3dcf791bb5a466d1edb714d4e45e7b962bc37f59/yombo/lib/mqtt.py#L329

I think a lot of these features could be incorporated into your code, but up to you. I'm basically running my fork (pull requests to you) and changing only a copy additional items in the protocol and factories specific to my use case (i inherit and override your functions a couple times).

I also implemented another layer between my code and your code. It basically just handles connack items. My code can simply request a new connection and subscribe right away and the middle layer queues requests so my code doesn't have to wait for the connect-ack, etc. Fire and forget...This works in my use case as MQTT is low priority and dosn't break things as it's secondary to AMQP connections.

[astrorafael]

Ok. I'll have a look at it when I get back from my holidays

[astrorafael]

Ok. Ill merge the pull request, then I'll fix the return self.protocol. And also will change the callbacks. Regrading your library, I'll have a look a what it does. Maybe it could be incorporated a new module called mqtt.client.helper.py. Does it make sense to you ?

I would be very grateful if you could use the 'develop' branch for your PRs. I use the develop branch to accumulate tested changes before producing a new release. Then I merge to master to produce the release.

[astrorafael]

Hi Micth,I was wondering why you need an onMQttConnection Callback.

You perform a connection request, the library returns you a deferred and you should register your callback in the deferred object. I think this is the way it should work.

My library invokes the deferreds when the CONNACK packet is received is made, as you can see in this snippet. All asynchronous request/responses should be handled bu Deferreds, not by setting callback hooks, which is redundant with the Deferred mechanism.

I have only found two cases for use callbacks: 1) Receiving published payloads. Even in this case, you could do a "read published data" that returns a deferred, but the was much more convenent to me. 2) Notifying disconnection when no deferreds were given to client code.

# Changes state and execute deferreds
        log.debug("<== {packet:7} (code={code} session={flags})", packet="CONNACK", code=response.resultCode, flags=response.session)
        request = self.connReq
        request.alarm.cancel()
        if response.resultCode == 0:
            self.state = self.CONNECTED
            self.mqttConnectionMade()   # before the callbacks are executed ...
            if request.keepalive != 0:
                self._pingReq.keepalive = request.keepalive
                self._pingReq.timer     = task.LoopingCall(self.ping)
                self._pingReq.timer.start(request.keepalive)
            request.deferred.callback(response.session)
        else:
            self.state = self.IDLE
            msg = MQTT_CONNECT_CODES[response.resultCode]
            request.deferred.errback(MQTTStateError(response.resultCode, msg))
        self.connReq = None     # to be garbage-collected

My mqttConnectionMade was meant for internal bookeeping