MQTT support for multiple devices #138
Conversation
|
@jbaudoux , appreciate a code review if you have the opportunity. |
| } | ||
|
|
||
| // Note: If (username == NULL) then disable authentication | ||
| // If (username != NULL && password == NULL) then username is used without a password |
There was a problem hiding this comment.
No, you cannot do that. See https://github.com/eclipse/mosquitto/blob/master/lib/options.c#L69
Please revert this change
There was a problem hiding this comment.
Thanks for the review. This is getting interesting as it looks like the latest version of mosquitto does not match the documentation! But we should probably reference this link here as this is the version 1.3.4-1.4.10 which is available by apt for OSPi (i.e. much earlier). But lets dig into this as the code isnt operating as I would expect.
The motivation behind this change was to ensure we deal with three use cases: 1) username not provided (un-authenticated), 2) both username and password are provided (authenticated); and 3) username is provided but password is not (unprotected user login).
It is this third case that I was considering as according to the mosquitto documentation and code I linked above then we should be calling mosquitto_username_pw_set(mqtt_client, _username, NULL) rather than passing an empty string as _password.
Appreciate your thoughts on this as it feels like a decision between coding for the currently distributed version vs what might come down the road.
There was a problem hiding this comment.
I had a wrong reading of source code. Apparently, it does not change anything but documentation says to implement it as you did.
https://mosquitto.org/api/files/mosquitto-h.html#mosquitto_username_pw_set
So, I agree with this change ;-)
Thanks
There was a problem hiding this comment.
I also pondered on this and so far I've assumed that if user name is non-empty then password is also non-empty. Would anyone intentionally set only a user name but empty password in practice?
There was a problem hiding this comment.
I agree, in normal practice you would want to use either case 1 or 2 but the spec does allow for case 3 as well. I can imagine scenarios in a trusted network where you want to track usage but aren't too worried about rogue agents !
Having said that, if we want to restrict against this scenario then we should do it earlier in the process i.e. in the App UI or api response so we can provide user feedback.
There was a problem hiding this comment.
As documentation says to do it like this and you implemented it, let's merge this improvement
There was a problem hiding this comment.
Sorry PeteBa but I am strongly against this PR in its current state. The topic should never contain the MAC address, this is a bad design.
The MAC address can be related to external hardware and is subject to change. The topic should never change if you modify your ethernet connection. Add wifi dongle, change default route,..
The root topic currently hardcoded to "opensprinkler" must be configurable through the App. I checked other projects and this is the approach to follow.
However, then clientid used to identify the client connected to the broker should contain the MAC address. That is correct.
| DEBUG_LOGF("MQTT Topic (%s) is too large. Will be truncated\n", topic); | ||
| #endif | ||
|
|
||
| int len = snprintf(full_topic, MQTT_MAX_TOPIC_LEN, "%s/%s/%s", MQTT_ROOT_TOPIC, _id, topic); |
There was a problem hiding this comment.
The root topic should be configurable through the app (like host, username, password). The MAC address should not part of the topic !!!
There was a problem hiding this comment.
So again thanks for the feedback and let me try to explain some of my thinking by taking this in two parts.
Firstly, the use of "opensprinkler" as the root topic. This is currently how the code exists today and available in the master branch. So this hasn't changed, I have just moved the definition out of main.cpp and into mqtt.cpp in order to better compartmentalise the code. I agree that in the future this should be configurable via the App and this PR moves us a little bit in that direction.
The second part is to address the situation where there are two or more opensprinkler devices on the network. This is the case for me and others. The current master branch is problematic as a user could enable mqtt on multiple devices with conflicting messages causing unexpected behaviour. So this is a first step to guarding against that with the information readily at hand. Again, a future PR could make this configurable from the App and the structure that this PR puts in place would make that a bit easier.
Note that the load_hardware_mac() routine produces a pretty stable identifier. Only recent OS3.0 have an option to change network interfaces and on OSPi I search the interface list in a particular order to minimise risk of change. In the future, I think leveraging the existing Site Name that App uses would be a good choice to differentiate opensprinkler devices as people are already familiar with it. Unfortunately, at the moment, it isn't accessible over the rest api and would require some changes to make that possible. I explored this some time ago here and it looks to be a viable option.
Let me know your thoughts as I was keen to get something that makes the current master branch a bit safer while the bigger questions in #130 and #134 get worked through.
There was a problem hiding this comment.
For enabling mqtt, you also have to provide a host. So let's show and propose to change the root topic as we did for username and password. This will allow multiple devices. And if you want to put your mac address in your topic, you are free to configure it like this.
But don't always append the root topic with the MAC address. For me, it is a bad design, and should not be the default behavior.
So default root topic:
"opensprinkler"
can be changed in the config to any other value like for example:
"opensprinkler1"
"opensprinkler/1"
"opensprinkler/00:0a:95:9d:68:16"
There was a problem hiding this comment.
Two quite notes:
- We do plan to add a 'device name', mainly to make it easy for users (with multiple OS) to tell which controller it's coming from when using, say IFTTT notifications (i.e. the device name will be included in the message). It's probably not appropriate to use 'device name' as mqtt root topic, because the device name can have all sorts of special characters and I am not sure if that's a good fit for root topic.
- When I was working on integrating the mqtt branch, I did think about whether we should add an additional parameter - root topic - to give users the choice of deciding what topic to use. Related to my point 1 above -- what are the restrictions on the root topic name? For example, is it ok to have the 'space' character? What about other special characters?
There was a problem hiding this comment.
An mqtt topic is any utf-8, case-sensitive string. In many implementations the topic defaults to a slug of the device name or a readily available unique identifier (eg. mac).
Including a "device" leaf in the topic allows for multiple device support and having a "product" leaf in the topic allows for broadcast and discovery. So topics like "opensprinkler/device_id/endpoint" are quite common. However, there are alternative conventions and many sophisticated users will want to customise their own topics to their particular preference.
My intent with this PR is two fold: 1) safeguard multi-device owners so that devices don't clash on the same topic and cause unpredictable behaviour, and 2) do it in a way that doesn't constrain the design of the message syntax under discussion in #134. Hence this PR doesn't expose the topic structure to end-user customisation.
There was a problem hiding this comment.
many sophisticated users will want to customise their own topics to their particular preference.
Exactly. So expose the root topic to end-user customization :-)
There was a problem hiding this comment.
I am in favor of exposing the root topic to the user for customization. The topic can have an initial default value (it could either be 'opensprinkler' as we currently have, or it could be the device ID that @PeteBa proposed), and then user can change it to something else if they want.
This adds support for multiple opensprinkler devices using MAC address as the unique client identifier. The topic tree is extended to the form
opensprinkler/OS-MACADDRESS/...to allow for addressing specific units. I have retainedopensprinkleras root topic to avoid namespace conflict and to allow for group broadcast in future.There are some minor code corrections to allow for MQTT usernames without passwords.