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

[OTA] Device no longer found by Arduino IDE 2.5.0 (Beta 1) #5487

Closed
6 tasks done
Swiftnesses opened this issue Dec 12, 2018 · 24 comments
Closed
6 tasks done

[OTA] Device no longer found by Arduino IDE 2.5.0 (Beta 1) #5487

Swiftnesses opened this issue Dec 12, 2018 · 24 comments

Comments

@Swiftnesses
Copy link

Swiftnesses commented Dec 12, 2018

Basic Infos

  • This issue complies with the issue POLICY doc.
  • I have read the documentation at readthedocs and the issue is not addressed there.
  • I have tested that the issue is present in current master branch (aka latest git).
  • I have searched the issue tracker for a similar issue.
  • If there is a stack dump, I have decoded it.
  • I have filled out all fields below.

Platform

  • Hardware: [NodeMCU V1.0]
  • Core Version: [Latest Arduino IDE build]
  • Development Env: [Arduino IDE]
  • Operating System: [MacOS]

Settings in IDE

  • Module: [NodeMCU V1.0]
  • Flash Mode: [qio|dio|other]
  • Flash Size: [4MB (No SPIFFS)]
  • lwip Variant: [v2 Higher Bandwidth]
  • CPU Frequency: [80Mhz|160MHz]
  • Upload Using: [OTA]

Problem Description

Following the update to 2.5.0 (Beta 1), devices are no longer found by Arduino IDE (1.8.9).

Restarted IDE, no change. Reverting back to 2.4.2 (full flash erase via OTA), devices show within 10 seconds. Note: the valve is remote to the house, so I have not tried a serial flash (I will, once the weather improves!).

Sketch contains the following ESP parameters:

  // Prevent wifi sleeping
  WiFi.setSleepMode(WIFI_NONE_SLEEP);
  // 802.11n
  WiFi.setPhyMode(WIFI_PHY_MODE_11N);
  // Station mode
  WiFi.mode(WIFI_STA);
@Swiftnesses Swiftnesses changed the title [OTA] Device no longer found by Arduino IDE (2.5.0 Beta 1) [OTA] Device no longer found by Arduino IDE 2.5.0 (Beta 1) Dec 12, 2018
@pfeerick
Copy link
Contributor

I can reproduce this with a Wemos D1 mini on Windows using Arduino IDE 1.8.7 and the latest 2.5.0 Beta1 ESP8266 core. OTA on 2.4.2 was working perfectly, but the device is no longer visible with 2.5.0 Beta1. This is with lwIP 1.4 Higher Bandwidth. Re-installing 2.4.2 and re-compiling/re-uploading the code (erasing only the sketch) resulted in the device being visible for OTA within a few seconds.

@devyte
Copy link
Collaborator

devyte commented Dec 12, 2018

@Swiftnesses @pfeerick
ArduinoOTA relies on MDNS internally. In order for MDNS to work, you have to call MDNS.update() in the loop. This was a requirement in the old implementation as well, but in that case it was commonly omitted, and things got weird in some cases depending on network traffic. Or not.
Please let me know if calling MDNS.update() in the loop fixes this.

@devyte devyte added the waiting for feedback Waiting on additional info. If it's not received, the issue may be closed. label Dec 12, 2018
@pfeerick
Copy link
Contributor

pfeerick commented Dec 13, 2018

I was going to say that this didn't make any difference for me - but some more testing has shown that adding the MDNS.update() call does make the device start advertising again. Core v2.4.2 OTA was always visible and working regardless of the presence of a MDNS.update() call in the `loop(). When Core 2.5.0 Beta1 didn't advertise,as long as I didn't close the Arduino IDE, it seemed to be sticky and still responded to OTA updates.

Two observations - a quick grep of the ESP8266 libriries doesn't show a single call to MDNS.update in even the MDNS examples except for the newLEAmDNS examples that have been added post 2.4.2. Also, MDNS isn't highlighted as a keyword anymore with the 2.5.0 Beta1 core??? If going forward MDNS.update() is needed, all the OTA examples etc need updating, as they don't use it. And if it was technically needed before, that's a bit... unsettling... as the built-in examples weren't indicating/documenting that!

Edit: For consistency, here is the code I used to test this - it's a modified version of the BasicOTA sketch with a few extra serial outputs, etc. I just commented/uncommented line 79 to test 2.4.2/2.5.0Beta1 OTA functionality. https://gist.github.com/pfeerick/bef13f437b16e11b427bbb27416e4194

@devyte
Copy link
Collaborator

devyte commented Dec 13, 2018

@pfeerick MDNS.update() has always been needed. It's just that, given certain conditions that have nothing to do with the ESP, the legacy implementation would still work without it.
Now, MDNS.update() is mandatory, or it won't work. No in-between half-sometimes-maybe working state.
About the examples, you're correct, and art of the problem was that nobody knew the need of calling update() and that the examples weren't quite correct.
I'll keep this open to track fixing them.

@devyte devyte added type: bug component: libraries component: examples component: OTA and removed waiting for feedback Waiting on additional info. If it's not received, the issue may be closed. labels Dec 13, 2018
@devyte devyte added this to the 2.5.0 milestone Dec 13, 2018
@Swiftnesses
Copy link
Author

@devyte Thanks for helping us understand this.

What I don't understand is, none of my OTA sketches used to include (even though they're part of the examples):

#include <ESP8266mDNS.h>
#include <WiFiUdp.h>

They still worked without issue (very well in fact), how is this possible? Trying to get my head around why it worked before and why I need to add 2 new libraries...?

@devyte
Copy link
Collaborator

devyte commented Dec 13, 2018

The MDNSResponder in our core has been rewritten from scratch.
Arduino OTA relies on MDNS (I.e.: uses the global object internally) to advertise itself to the IDE.
In the legacy implementation, MDNS would call update() every time it received a packet. With enough network traffic, calling update in the loop was not needed, because packet reception would drive the functionality. This has the advantage of not needing that one call, but the disadvantage that if you changed the network conditions (move the esp to an isolated network, use softAP instead of sta, remove enough of the other mdns devices on the network, etc) the responder would start working intermittently or not at all. Worst part is: the cause was not obvious.
With the new implementation, you have to call update() in the loop it doesn't work. No in between state.
Yes, it used to work without calling update() before, but it was luck when it worked, and for other users it didn't work.
Yes, the examples are wrong before, and are still wrong now. They need to be fixed to include the call to update() in the loop.
If you used ArduinoOTA before, you also implicitly included MDNS. MDNS is still included now, it's just a bit more explicit instead of hidden.

@Swiftnesses
Copy link
Author

@devyte Thank you for the explanation, appreciate it.

I've now included <ESP8266mDNS.h> in my sketches.

I assume that <WiFiUdp.h> is not actually required, why is it included?

@dav1901
Copy link
Contributor

dav1901 commented Dec 13, 2018

Hey @Swiftnesses , WiFiUdp.h is included in ESP8266mDNS.h so I guess that's not really necessary.

@Swiftnesses
Copy link
Author

@dav1901 thanks for the confirmation ;-)

@devyte
Copy link
Collaborator

devyte commented Dec 13, 2018

I just realized that in the case of ArduinoOTA I can include the call to MDNS.update() inside ArduinoOTA::handle(). In that case, calling MDNS.update() in the loop is not needed when ArduinoOTA is used.

@Swiftnesses
Copy link
Author

Excuse my lack of knowledge (I'm not much of a coder...)

But all my sketches include <ArduinoOTA.h>, I still needed to add MDNS.update() for 2.5.0 Beta 1 to work, perhaps I'm misunderstanding your comment :-/

On another note, even with the update in the loop, my distant sensors no longer present themselves for OTA update, likely related to the low connection speed and slow ping that I reported. I planned to test reverting the change you suggested, but upon OTA flash, the sensor never came back (even though I can ping it!). Something's not right, but it's freezing outside and the sensors are at the bottom of the garden, bugger that.

@kerryeven
Copy link

Wemos D1 mini also lost OTA with beta update. Reverted and was discovered. Did notice
ESP8266mDNS.h was black and not red in the include and turned red when I downgraded.

@pfeerick
Copy link
Contributor

pfeerick commented Dec 14, 2018

@kerryeven Even after adding the MDNS.update() and required includes?

@devyte Thanks for the explanation... that makes perfect sense now. It would be great if you can include the MDNS.update() as part of ArduinoOTA.update()... then nothing needs to be changed in existing sketches. What about also including ESP8266mDNS.h as part of ArduinoOTA? It already includes ESP8266Wifi and WiFiUdp as it is, might as well make it so you can just '#include <ArduinoOTA.h> and everything is automagically included... ;) btw, I have three ESP8266 modules - 2xDigistump Oaks and 1xWeMos D1 Mini running the new core, as well as MODEM_SLEEPing, and they are still running stable after 24 hours.

@kerryeven
Copy link

@pfeerick All I know is I got a notice that some boards had updates. Went to board manager, searched on updateable, and saw the ESP8266 beta 5.0 so I installed it. Was working on a duct fan controller for a remote attic ductfan. Made a minor change and uploaded. Never to be seen again until I downgraded back to 2.4 whatever and it started OTA'ing again. Didn't see anything about any required includes or MDNS notifications when I updated. Did notice that color was black with the upgraded board include and red (orange) with the old rev. Directory problem or something...I don't know.

@devyte
Copy link
Collaborator

devyte commented Dec 14, 2018

@kerryeven the color of words in the IDE has exactly zero impact on functionality. It is just syntax highlighting, based on configuration found in file library.properties. In other words, it's just eye candy.
I'll take a look if I can find something obvious.

@pfeerick
Copy link
Contributor

pfeerick commented Dec 14, 2018

@devyte Just spotted it... keywords.txt is missing has been moved - both it and the readme are in the ESP8266mDNS/src directory. Part of commit 58a044b

@kerryeven
Copy link

Would that cause the OTA port to stay invisible. Port did not show with upgrade. Did show, same sketch with downgrade.

@pfeerick
Copy link
Contributor

NO. As per the last few comments, it is due to an internal change to how mDNS works, which is how the devices running OTA-enabled firmware advertise themselves on your network. As the code stands right now, if you add #include <ESP8266mDNS.h> to the top of your sketch, and MDNS.update(); above or below where you have ArduinoOTA.handle(); and it should work fine again. The missing keywords.txt file is just the reason the cosmetic highlighting has gone MIA.

@kerryeven
Copy link

Is there a way to let peeps know they need to add to their basic ota sketches before upgrading. Kind of a bummer to have to crawl around the attic. Thanks for the explanation..clear now.

@pfeerick
Copy link
Contributor

pfeerick commented Dec 14, 2018

If devyte makes the change hinted about above from six hours ago, it won't be needed, as the previous code behaviour will be kept (but with the more newer and more reliable mDNS). Otherwise, it's a matter of waiting to see the documentation updates when 2.5.0 is released. But generally, if it says beta (i.e. 2.5.0-beta1), expect something to break!

@kerryeven
Copy link

Definately does work...added MDNS.update(); immediately after loop() and before ArduinoOTA.handle(). Thanks

@devyte
Copy link
Collaborator

devyte commented Dec 14, 2018

Oops, keywords.txt not library.properties.
@pfeerick what do you mean missing? I see the file in git.

@pfeerick
Copy link
Contributor

@devyte keywords.txt and readme.rst are in the \src folder, not in the library root (https://github.com/esp8266/Arduino/tree/master/libraries/ESP8266mDNS) ... they got moved with the rest of the source files in that commit. ;)

@devyte
Copy link
Collaborator

devyte commented Dec 14, 2018

Closing via #5494 .
ArduinoOTA.handle() now calls MDNS.update(), so in that case there's no need to call from the loop.
Examples updated to call MDNS.update() in the loop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants