espressif / esp-mqtt

ESP32 mqtt component
Apache License 2.0
610 stars 257 forks source link

FIX: mqtt_get_id && handle msg without a payload (IDFGH-1025) #114

Closed homeit-hq closed 5 years ago

homeit-hq commented 5 years ago

Found 2 problems with the current idf branch.

This PR solves booth issues.

david-cermak commented 5 years ago

@homeit-hq Thanks for sharing your fixes!

The first commit https://github.com/espressif/esp-mqtt/pull/114/commits/4ba94a0a69f0cbbd563cf6573c81d40de8732971 looks good, thanks for fixing this OBOE bug.

The other one is missing support for longer data than buffer size, As far as I can see your implementation returns on the first data event, the original deliver_publish loops while longer payload is present and splits data to multiple data events.

homeit-hq commented 5 years ago

@david-cermak I'm glad to help.

About the 2nd one, yes, I only found that out after reading another non-related issue. Was that behavior written somewhere?

Right now I don't have time to add that to the PR, but if I manage to get some time I will try to add that.

My ideia was that if the msg is fragmented ( but its full size is less then the input buffer) it should only fire once the event.
If the msg (HEADER+data) is bigger then the buffer it should span multiple events...

david-cermak commented 5 years ago

@homeit-hq I'm afraid that's not documented properly, will add a doxy-comment to the header at least. Yes currently even shorter messages could fire multiple data events if fragmented. This works fine for publish messages, but could cause trouble if other than publish message gets fragmented (as indicated here https://github.com/espressif/esp-mqtt/issues/113, so shorter publish messages won't need to be split after implementing fix)

There's also another PR #110 related to empty messages which is already approved and could be merged soon. So your first commit https://github.com/espressif/esp-mqtt/commit/4ba94a0a69f0cbbd563cf6573c81d40de8732971 could be cherrypicked afterwards to correctly receive message ids for QoS 1 and 2.

homeit-hq commented 5 years ago

@david-cermak When I wrote this PR the one you mentioned didn't fix the PR objective as you mention in the PR comments. That was the reason behind this PR.
As that one is fixed and it's complying with the lib logic you can drop mine, and cherrypick the fix for the msg id.

Thank you for your attention.

david-cermak commented 5 years ago

Hi @homeit-hq

OK, understand. I'm picking this commit https://github.com/espressif/esp-mqtt/pull/114/commits/4ba94a0a69f0cbbd563cf6573c81d40de8732971 and would close this PR once it's merged and pushed to github. Thanks!

david-cermak commented 5 years ago

Cherry-picked as https://github.com/espressif/esp-mqtt/commit/db71c753aad7de09af1a667d1db0d5017c048f6a Thanks!