-
Notifications
You must be signed in to change notification settings - Fork 787
Conversation
Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>
Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>
Fixes: #5197 Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>
@doandzhi What do you think about? |
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.
Very important changes that surely extend the use cases of MQTT lwt implementation of ESH. I assume that it is working fine. Good effort @maggu2810 , thanks for the contribution.
* Add a broker by a configuration. | ||
* | ||
* <p> | ||
* You need to provide at least a "name" and an "url". | ||
* Additional properties are "user","pwd","qos","retain","lwt","keepAlive","clientId", please read the |
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.
Maybe it would be useful to mention the new properties here ( lwtTopic, lwtMessage lwtQos lwtRetain)
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 implementation has been changed to use a config object that properties are "fixed". As all fields are nullable and so optional the documentation needs to state which ones are required.
I don't know if it make sense to sync the additional properties all the time.
Perhaps we could remove the "Additional properties ..." part at all if this PR will be accepted (and before it is merged).
Would you like to give it a try. |
* Additional properties are "user","pwd","qos","retain","lwt","keepAlive","clientId", please read the | ||
* service configuration documentation for a detailed description. | ||
* | ||
* @param brokerConnectionConfig The configuration key-value map. | ||
* @param cfg The configuration key-value map. |
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 not a key-value map anymore
Sure, I can do that! I will give feedback in a while. |
I can confirm that the proposed changes resolve issue #5197 . It is able to use JSON payload and no problems were faced. Thanks for the contribution @maggu2810 ! |
Add support to configure LWT topic, message, qos and retain flag explicit.
Fixes: #5197