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

Modify a check to make sure that keep alive is sent even when data is in the buffer #223

Merged
merged 5 commits into from
Sep 29, 2022

Conversation

AniruddhaKanhere
Copy link
Member

@AniruddhaKanhere AniruddhaKanhere commented Sep 29, 2022

The coreMQTT library is responsible for sending the keep alive messages to the broker when there is no transaction of data between the broker and the client.

The check ( if( status == MQTTNoDataAvailable ) ) for that condition is not correct anymore (since PR #218) because this condition is only set when there is no data in the socket AND none in the receive buffer. Thus, if there is any data in the receive buffer, it would prevent the library from sending a ping.

But for the matter of keep alive, the server does not care whether there is data in the buffer or not. It only cares when was the last TCP transaction with the client which can be depicted by the data in the socket.

This PR updates the if condition such that the sending of keep alive is only based on the reception of data or lack thereof from the socket. The change is as follows:

-   if( status == MQTTNoDataAvailable ) // This status value is set only when there is no data in the buffer and not in the socket.
+   if( recvBytes == 0 ) // This value shows that no more data is available in the socket.

archigup
archigup previously approved these changes Sep 29, 2022
paulbartell
paulbartell previously approved these changes Sep 29, 2022
Copy link
Contributor

@paulbartell paulbartell left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@AniruddhaKanhere AniruddhaKanhere merged commit 10d85cb into FreeRTOS:main Sep 29, 2022
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