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

fixed the rc parameter MQTT_ERR_CONN_LOST to be passed instead of 1 t… #441

Conversation

nawab36085
Copy link

…o user callback function on_disonncect

@nawab36085 nawab36085 mentioned this pull request Nov 5, 2019
@denravonska
Copy link

Is there a reason why this is still open, or why there has been no reaction to it for 4 months?

@karlp
Copy link

karlp commented Jun 23, 2021

The documentation states that it's "0 for explicit disconnect, non-zero for unexpected disconnect" you're not meant to be trying to decode it further.

@denravonska
Copy link

The documentation states that it's "0 for explicit disconnect, non-zero for unexpected disconnect" you're not meant to be trying to decode it further.

In that case I'd argue that both the documentation and the implementation are wrong.

@karlp
Copy link

karlp commented Jun 23, 2021

The documentation matches the implementation. Should the implementation be made more like other functions where rc can be decoded with mosq_errstr()? sure, that sounds perfectly reasonable, but that's an enhancement.

@denravonska
Copy link

denravonska commented Jun 24, 2021

Depending on if it's a conscious decision or not to use a hard coded 1 here you can view it as either a bug or a change request. Either way, it would be handy if the return codes are used consistently, especially when the function in question does use the provided define for positive outcomes.

ralight added a commit that referenced this pull request Jul 7, 2021
The `rc` parameter in the `on_disconnect` callback now has meaningful values
in the case of an error. Closes #441. Thanks to Nawab.
@ralight
Copy link
Contributor

ralight commented Jul 7, 2021

This is now fixed - I've done it in a slightly different and more comprehensive way than this PR, so I'm going to close this. Thanks for the request.

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.

4 participants