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

Add persistent storage for MQTT state #311

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

vishwamartur
Copy link

Related to #271

Add support for storing and restoring MQTT state in persistent memory to handle QoS2 messages after a device reboot.

  • Add function prototypes for MQTT_SetOutgoingPublishRecord, MQTT_GetOutgoingPublishRecord, and MQTT_GetFailedPacketId in source/include/core_mqtt_state.h.
  • Implement MQTT_SetOutgoingPublishRecord, MQTT_GetOutgoingPublishRecord, and MQTT_GetFailedPacketId functions in source/core_mqtt_state.c.
  • Update README.md to include instructions on using the new setter and getter functions.
  • Add unit tests for MQTT_SetOutgoingPublishRecord, MQTT_GetOutgoingPublishRecord, and MQTT_GetFailedPacketId functions in test/unit-test/core_mqtt_state_utest.c.

Related to FreeRTOS#271

Add support for storing and restoring MQTT state in persistent memory to handle QoS2 messages after a device reboot.

* Add function prototypes for `MQTT_SetOutgoingPublishRecord`, `MQTT_GetOutgoingPublishRecord`, and `MQTT_GetFailedPacketId` in `source/include/core_mqtt_state.h`.
* Implement `MQTT_SetOutgoingPublishRecord`, `MQTT_GetOutgoingPublishRecord`, and `MQTT_GetFailedPacketId` functions in `source/core_mqtt_state.c`.
* Update `README.md` to include instructions on using the new setter and getter functions.
* Add unit tests for `MQTT_SetOutgoingPublishRecord`, `MQTT_GetOutgoingPublishRecord`, and `MQTT_GetFailedPacketId` functions in `test/unit-test/core_mqtt_state_utest.c`.
@archigup
Copy link
Member

archigup commented Nov 4, 2024

Hey, thanks for opening a PR!

We recently landed having the library handle QoS1/2 re-transmits for the user to the main branch; this seems like its related. We should consider how these are used together

The device could set its packet storage callbacks to use flash, though when initializing the library need to restore the in-transmit state as you have here.

@archigup archigup added the enhancement New feature or request label Nov 4, 2024
@AniruddhaKanhere
Copy link
Member

Hello @vishwamartur!

Thank you for raising this PR. It seems quite useful.

Let us take a look. We recently have added functionality for the library to be able to re-send publishes which were not ACKed before a disconnect and reconnect of MQTT connection with an unclean session. We will see whether this might be somehow linked with that feature.

Thanks again for taking the time to create the PR!

- Aniruddha


### Setter Function

The `MQTT_SetOutgoingPublishRecord` function allows the application to set an outgoing publish record in the MQTT context. It can be used to restore the state of the MQTT context after a device reboot.
Copy link
Member

Choose a reason for hiding this comment

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

I wonder whether a callback might be more suitable here?

Here is what i am thinking: The method described here will work perfectly fine when the reboot is planned. But what if the reboot is unplanned such as a power failure or link to the internet goes down.

In such cases, application might not have enough time to ping the library repeatedly for new packets. I understand that there might be race condition even in callback. But it is less pronounced.

When the application calls the API described above, it can either do that very frequently leading to higher CPU consumption, or it can do that slowly leading to a higher chance of a missed QoS2 publish when a reboot happens.

Copy link
Member

Choose a reason for hiding this comment

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

I thought more about it and I would like to fix my error in the above statement.

In case of unplanned power loss/reboot, the coreMQTT library cannot be held responsible for keeping the actual state and the stored state in NVM in sync. Because regardless of whatever the library does, there will always be a race condition in the actual and stored state of the system.

Thus, I think the library should only support planned reboots. In case of unplanned reboots, I think the application knows/understands the system best and it should try to fix any issues with the state of the system.

I will think more on this. But let me know if you have any suggestions or concerns @vishwamartur.

Thanks

@vishwamartur
Copy link
Author

Hi @AniruddhaKanhere and @archigup,

Thank you for the feedback! I'm glad you see the potential of this feature.

Regarding the callback suggestion, that's an interesting point. A callback approach would indeed reduce the risk of missing QoS2 messages, particularly in the case of unexpected reboots or network issues, where the application might not have enough time to poll for unacknowledged packets.

I’ll explore implementing a callback mechanism to handle persistent storage updates, as this might provide a more robust solution for unplanned reboots. Additionally, I could look into ways to make the polling frequency adjustable to balance CPU usage and the likelihood of packet loss. Let me know if you have any specific requirements or additional insights on this.

Thank you again for the thorough review and suggestions!

Best regards,
Vishwa

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

Successfully merging this pull request may close these issues.

3 participants