Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config: add support for lastWill and keep alive #468

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AmarOk1412
Copy link

By default, MQTT keep-alive is 1200 seconds and AWS broker add a 1.5x factor, so it takes generally 15 min to discover that a device is disconnected (30 min in the worst case)

This patch allow a user to configure a last will and change the default timeout (30 seconds will be the minimum authorized by AWS)

New config will have for example:

"last-will-topic": "things/myDevice/shadow/name/demo-shadow/update",
    "last-will-message": "{\"state\":{\"reported\":{\"connected\":false}}}",
    "connect-timeout": 0,
    "keep-alive": 30,

Motivation

[- Please give a brief description for the background of this change.

Modifications

Change summary

This is a WIP I guess, I don't know if this feature is wanted

Revision diff summary

If there is more than one revision, please explain what has been changed since the last revision.

Testing

"last-will-topic": "things/myDevice/shadow/name/demo-shadow/update",
    "last-will-message": "{\"state\":{\"reported\":{\"connected\":false}}}",
    "connect-timeout": 0,
    "keep-alive": 30,

You may want a rule to republish to the shadow topic, but with this config, instead waiting 30 minutes to get a KEEP_ALIVE disconnection, you can wait 45 seconds.

By default, MQTT keep-alive is 1200 seconds and AWS broker add a
1.5x factor, so it takes generally 15 min to discover that a device
is disconnected (30 min in the worst case)

This patch allow a user to configure a last will and change the
default timeout (30 seconds will be the minimum authorized by AWS)

New config will have for example:

	"last-will-topic": "things/myDevice/shadow/name/demo-shadow/update",
        "last-will-message": "{\"state\":{\"reported\":{\"connected\":false}}}",
        "connect-timeout": 0,
        "keep-alive": 30,
@HarshGandhi-AWS
Copy link
Contributor

Hello @AmarOk1412 , if the objective of this feature is to know when the device is connected or disconnected, then you can also use a pubsub model to detect it. Instead of asking device to reply back, you can use pushsub feature which will send message to a certain topic A after receiving a message on a certain topic B. Using shadow feature with IoT rule looks like an overkill to me.

Comment on lines +387 to +389
LOG_INFO(TAG, "MQTT connection set will succeeded");
} else {
LOG_INFO(TAG, "MQTT connection set will failed");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have more meaning full logs pasted over here? Also DEBUG logs will make more sense over here?

@@ -381,6 +381,15 @@ int SharedCrtResourceManager::establishConnection(const PlainConfig &config)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add documentation around using this feature. I suspect user will also have to create an IoT Rule, add detailed documentation around setup required for this feature to help other users use it with ease. You will also have to update setup file and config file template to help other users set the config easily.

It is not very clear on what is the use case for this feature and why you would like to add this feature to know the device state on when it is disconnected. Can you share more details on that?

Maintaining a shadow for this feature alone is not a good idea since shadow feature is expensive than using MQTT Pub Sub messages to determine the device state.

Also consider using Device Defender feature which in timely manner uploads device side metrics to cloud. Check is you can use any DD metric to determine the device state instead?

@ig15
Copy link
Contributor

ig15 commented Nov 1, 2024

@AmarOk1412 Can you address the open comments and update the PR accordingly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants