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 topic copying for chunked messages #645

Merged
merged 1 commit into from
Dec 19, 2019

Conversation

mkfrey
Copy link
Contributor

@mkfrey mkfrey commented Dec 12, 2019

Only copy the topic on the first message chunk. Otherwise the old copy will be
destroyed on subsequent calls to _onMqttMessage() for the same message, while
the copy is still being referenced by _mqttTopicLevels. This leads to undefined
behaviour.

Should solve #644

Thanks @nemidiy for investigating this issue!

Only copy the topic on the first message chunk. Otherwise the old copy will be
destroyed on subsequent calls to `_onMqttMessage()` for the same message, while
the copy is still being referenced by `_mqttTopicLevels`. This leads to undefined
behaviour.
@kleini
Copy link
Collaborator

kleini commented Dec 14, 2019

I tested this pull request on top of pull request #646. OTA partly seems to work. I see some firmware file transferred to my esp8266 devices, which is already a lot more compared to what was possible in the past, but after the firmware upload to the device I get 400 BAD_FIRMWARE.

I am using the firmware.bin file being created during pio run in the .pio/build/sonoff/firmware.bin file. Sometimes the firmware is not even transferred and the request is directly denied with the same error message.

@nemidiy
Copy link
Contributor

nemidiy commented Dec 15, 2019

I tested the @codefrey's PR on nodemcuv2 and nodemcu32 and works for both platforms.
The only difference is that for esp8266 the OTA updater script says that it failed (even though it did not) and for esp32 says it's successful, don't recall where but someone reported this same thing. Probably some bug in the python script.

@kleini when you applied the DNS commit as well and tested OTA, were us using dns1 and/or dns2 or did you just happen to apply the commit in #646 and tested without setting dns ? I am asking so that I can try it out myself. Thanks.

@kleini
Copy link
Collaborator

kleini commented Dec 16, 2019

@nemidiy I tested on Saturday the exactly the following version https://github.com/kleini/homie-esp8266.git#35419c27ada5af4f1fc44fb19f339b355149599f, which is #646 and this pull request on top.

@nemidiy
Copy link
Contributor

nemidiy commented Dec 17, 2019

Hi @kleini I pulled develop-v3 and then merged this on top. I tested both setting up DNS and without DNS. OTA worked for me in both cases (tested on nodemcuv2). maybe you can put the details of your set up in the OTA ticket and I can try and help you out ? thanks.

@luebbe luebbe mentioned this pull request Dec 17, 2019
@luebbe luebbe merged commit 104a09c into homieiot:develop-v3 Dec 19, 2019
@mkfrey mkfrey deleted the develop-v3-fix-topic-copy branch December 19, 2019 11:41
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