-
Notifications
You must be signed in to change notification settings - Fork 121
Conversation
* MQTT Transport Implementation : - TLS Implementation - Username/Password based authentication - QoS 0, 1, 2 support - Adding gateway identity layer for encapsulating gateway details * Removing typo loop_forever and adding check for qos_details * Adding default parameters * MQTT Transport Example - Example for streaming data from device to IoTCC data centre component using MQTT channel - Adding device comm for MQTT - Updates in sampleProp.conf for MqttChannel which needs to be subscribed * Updated MQTT Example - Print statements removed * - Modifying gateway name as edge system - Description for Paho network loop method in MQTT example
edge_system_identity = Identity(config['cacert'], config['certfile'], config['keyfile'], config['mqtt_username'], | ||
config['mqtt_password']) | ||
|
||
# Encapsulate TLS parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactoring change: Move the mqtt channel subscription into a separate method mqtt_subscribe() and call it from the main just for better understanding and clarity that calls are separate from Project ICE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unclear on this. Could you please explain ?
- Should we add mqtt_subscribe() in mqtt device and DCC comms ?
2). What about publish() then ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to change the implementation take out the mqtt specific code into a separate method. Let`s discuss.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
|
||
|
||
except RegistrationFailure: | ||
print "Registration to IOTCC failed" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Format the Python code using PEP 8. Give a blank line at the end of all files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
#### [MQTT] #### | ||
MqttChannel = "mqtt-channel" | ||
MqttSubChannel1 = "mqtt-subchannel1" | ||
BrokerIP = "127.0.0.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep it general
BrokerIP = mqtt-broker
BrokerPort = mqtt-broker-port
I'll suggest to make it a different config file as samplePropMqtt.conf rather then keeping all this parameters in the main sampleProp.conf file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ship It.
Some minor comments, please fix them.
I'll wait for one more approval before merging the changes as per the review process.
# ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF # | ||
# THE POSSIBILITY OF SUCH DAMAGE. # | ||
# ----------------------------------------------------------------------------# | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add some comments about the class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add some comments about the class.
# ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF # | ||
# THE POSSIBILITY OF SUCH DAMAGE. # | ||
# ----------------------------------------------------------------------------# | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add some comments about the class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add some comments about the class.
@@ -0,0 +1,178 @@ | |||
# -*- coding: utf-8 -*- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename file to iotcc_simulated_mqtt.py
cacert = "/etc/broker-name/ca_certificates/ca.crt" | ||
certfile = "/etc/certs/client.crt" | ||
keyfile = "/etc/certs/client.key" | ||
mqtt_username = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please try to add example using username and password based mqtt approach as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example covers both username-password and certificate based Mqtt approach. i.e Remote Mqtt broker expects certificate and password.
Missed to add dummy username and password in prop file. Will add it :)
User can chose to use either certificate based approach or username password authentication by simply switching flags. So, should we add separate examples for certificate based and username-password ?
from liota.entities.metrics.metric import Metric | ||
from liota.dccs.dcc import RegistrationFailure | ||
|
||
import Queue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import order seems incorrect from pep8 convention. https://www.python.org/dev/peps/pep-0008/
You can use autopep8 for formatting as per pep8 convention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
BrokerIP = "mqtt-broker" | ||
BrokerPort = "mqtt-broker-port" | ||
keepalive = 60 | ||
cacert = "/etc/broker-name/ca_certificates/ca.crt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be set in /etc/liota/conf/ or /etc/liota/mqtt/conf/ instead of /etc/ directly?
def subscribe(self, topic, qos): | ||
self.mqtt_client.subscribe(topic, qos) | ||
|
||
def send(self, message): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for extracting out common DCCComms module is that all DCCComms should provide a common interface for common functionalities like connect, disconnect, send and receive. The developers should be able to change just the DCCComms declarations (or minimum code) and should be able to change the communication channel. If we provide completely independent methods for implementing publsih/subscribe for MQTT and send/receive for others, the commonality is lost. We should try to achieve the functionality from a common interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
@KohliDev and I discussed mqtt topic generation from liota's entity hierarchy. Topics will be generated at Dcc.
Here are the possible topic structure for a metric in liota:
- gw-uuid/device-uuid/metric-name
- gw-uuid/metric-name
So, Dcc will pass all args required by mqtt.publish(topic, message, qos, retain)
in a dict to send(message)
in MqttDccComms. Is that okay ? By this publish functionality can be achieved from common interface.
def subscribe(self, topic, qos): | ||
self.mqtt_client.subscribe(topic, qos) | ||
|
||
def send(self, message): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
log = logging.getLogger(__name__) | ||
|
||
|
||
class MqttDccComms(DCCComms): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used anywhere as of now? I guess we need to add an MQTT Broker as DCC and an example of how to use it. Should we do MQTT DCC, MQTT DCC Comms and MQTT as DCC example as part of a separate pull request. Here it is unused code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is part of the implementation and it`ll be leveraged in AWS example. I think @Venkat2811 is going to update that pull request.
Can we also add one separate basic example to publish data to ThingsWorx using MQTT DCC comms ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For publishing we'll need a DCC. It could be either a generic MQTT Broker DCC or specific DCCs (AWS, ThingWorx etc.,) that leverages MqttDccComms. We've used Mqtt Transports and Mqtt broker previously in car parking example for thingworx. Connecting to AWS should also be possible.
So, a generic MQTTBroker could be implemented and we could provide examples to publish data to AWS and ThingWorx.
- Signature change `subscribe(topic, qos, callback)` - Added `loop_start` and `loop_stop` in transports - Username/Password based authentication - QoS 0, 1, 2 support - Adding gateway identity layer for encapsulating gateway details * Removing typo loop_forever and adding check for qos_details * Adding default parameters * MQTT Transport Example - Example for streaming data from device to IoTCC data centre component using MQTT channel - Adding device comm for MQTT - Updates in sampleProp.conf for MqttChannel which needs to be subscribed * Updated MQTT Example - Print statements removed * - Modifying gateway name as edge system - Description for Paho network loop method in MQTT example * Changes in example code: - Created function for MQTT connection setup - Seperate conf file for MQTT example - Code refactoring * Change in method name * - Renamed MQTT example - Added comments - Change in configuration file * Updated MQTT example configuration file * - Adding network loop methods inside MQTT transport - Subscribe method change : Adding callback as a parameter - Changes in MQTT example script according to new changes - Optimised import statements * Rolling back mistakenly placed extra line
mess_attr -> msg_attr
log.error("Connection error with result code : {0} : {1} ". | ||
format(str(self._connect_result_code), paho.connack_string(self._connect_result_code))) | ||
self._paho_client.loop_stop() | ||
sys.exit(0) # successful termination |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove sys.exit as we discussed just keep the error getting logged so that MQTT loopback works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
|
||
import Queue | ||
|
||
import pint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use linux_metrics, it is recently updated in the master branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's instead of psutil
right.. I'll update it
BrokerIP = "mqtt-broker" | ||
BrokerPort = "mqtt-broker-port" | ||
keep_alive = 60 | ||
ca_cert = "/etc/liota/mqtt/conf/ca.crt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to:
gw_ca_cert
gw_cert_file
gw_key_file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will use edge_system_
instead of gw_
# ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF # | ||
# THE POSSIBILITY OF SUCH DAMAGE. # | ||
# ----------------------------------------------------------------------------# | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add some comments about the class.
# ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF # | ||
# THE POSSIBILITY OF SUCH DAMAGE. # | ||
# ----------------------------------------------------------------------------# | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add some comments about the class.
@@ -1,4 +1,5 @@ | |||
websocket-client==0.37.0 | |||
psutil==4.3.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update this file as per latest code check-in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
@@ -0,0 +1,172 @@ | |||
# -*- coding: utf-8 -*- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to add the README.md in the next pull request on the usage of MQTT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea. README should describe usage using GenericMqttDcc
and IoTCC
cases
# Validate CA certificate path | ||
if self.edge_system_identity.ca_cert: | ||
if not(os.path.exists(self.edge_system_identity.ca_cert)): | ||
raise ValueError("Error : Wrong CA certificate path.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add log.error statements where-ever applicable (ValueError & TypeError), don`t just raise the ValueError on the console
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
elif isinstance(mqtt_msg_attr, MqttMessagingAttributes): | ||
self.msg_attr = mqtt_msg_attr | ||
else: | ||
raise TypeError("mqtt_mess_attr should either be None or of type MqttMessagingAttributes") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add log.error statement
|
||
if cert_file and key_file: | ||
if not ca_cert: | ||
raise ValueError("CA certificate path is required, when certification based auth is used") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add log.error statement
if self.tls_details: | ||
|
||
# Validate CA certificate path | ||
if self.edge_system_identity.ca_cert: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Root_ca_cert come from the server is different from the gw_ca_cert which is generated locally. Use DCC_Identity class to hold username, password & dcc_ca_cert(root_ca_cert). Edge_system_identity class should only hold client certificates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea. Agreed. Will have separate classes DccIdentity
& EdgeSystemIdentity
@@ -47,7 +47,7 @@ class DccIdentity: | |||
def __init__(self, root_ca_cert, username, password): | |||
|
|||
""" | |||
:param root_ca_cert: Root CA certificate path | |||
:param root_ca_cert: Root CA certificate path or Self-signed certificate path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-signed server certificate path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the commits in this pull request should be merged and committed as one single commit. It will be easy to track in future this specific pull request.
Fix for #54.
EdgeSystemIdentity
andRemoteSystemIdentity
classes.DccComms.send(message)
toDccComms.send(message, mess_attr)
MQTT Transport Implementation :
MQTTMessagingAttributes