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

fix _onPubcomp #111

Merged
merged 2 commits into from
Aug 31, 2023
Merged

fix _onPubcomp #111

merged 2 commits into from
Aug 31, 2023

Conversation

MichaelDvP
Copy link
Contributor

With receiving Pubcomp the communicaion is finished.

@bertmelis
Copy link
Owner

Another bug? 😢

@MichaelDvP
Copy link
Contributor Author

Yes, but mosquitto ignores it, only the ioBroker mqtt-broker throws a lot of errors because of the extra pubcomp.

@bertmelis
Copy link
Owner

Ouch, that's a stupid copy-paste error of me.

Obviously we don't need to send a pubcomp when receiving a pubcomp. 🤦

@MichaelDvP
Copy link
Contributor Author

BTW: is this:

, _timeout(10000)
fixed value for _timeout intended or should it be EMC_TX_TIMEOUT from the config?

@bertmelis
Copy link
Owner

BTW: is this:

, _timeout(10000)

fixed value for _timeout intended or should it be EMC_TX_TIMEOUT from the config?

Yes. I believe so. This compile time variable also isn't in the docs.

@bertmelis bertmelis merged commit acf3564 into bertmelis:main Aug 31, 2023
@bertmelis
Copy link
Owner

The code was actually tested for this scenario but Mosquitto is forgiving and doesn't disconnect.

See https://github.com/bertmelis/espMqttClient/blob/main/test/test_client_native/test_client_native.cpp#L226

Maybe I should add a delay in the test to give Mosquitto some time to realize something went wrong...???

@MichaelDvP
Copy link
Contributor Author

Maybe I should add a delay in the test to give Mosquitto some time to realize something went wrong...???

No, mosquitto just knows the communication is successfull and ignores the extra pubcomp. There is no errormessage, it just works.
Other brokers get confused by this packet. The ioBroker-mqtt i've used does not like it, i tested with mosquitto, it works, so i first thought it was an error in ioBroker,

@bertmelis
Copy link
Owner

That's it! I just test with Mosquitto (in Github actions but also in my home setup).

Thanks for the find!

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.

2 participants