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

IDE OTA Upload changes #980

Merged
merged 8 commits into from Nov 10, 2015

Conversation

Projects
None yet
6 participants
@me-no-dev
Collaborator

me-no-dev commented Nov 7, 2015

  • added md5.h and MD5Builder with access to the ROM md5 functions
  • added DIGESTish_MD5 authentication to espota.py and ArduinoOTA
  • added MD5 check of the received image in Updater
  • added extra fields to mDNS to enable IDE OTA for when/if our changes are accepted by the Arduino team
  • add/changed recipe for network upload through the IDE for when/if our changes are accepted by the Arduino team
  • big overhaul to ArduinoOTA to facilitate all necessary authentication and sanity checks
  • fixed relevant example sketches

my Arduino IDE branch with IDE Upload included: https://github.com/me-no-dev/Arduino-1/tree/esp8266-ota

me-no-dev added some commits Nov 7, 2015

mDNS, platform and espota.py changes for IDE Upload
mDNS responds with more TXT properties
platform change to support OTA functions
espota.py added authentication parameter
IDE branch: https://github.com/me-no-dev/Arduino-1/tree/esp8266-ota

me-no-dev added some commits Nov 8, 2015

Let the socket to properly close
python was keeping the connection open and sending duplicate FINs until
the ESP came back online, because the ESP was resetting without giving
the network a chance to answer and close

igrr added a commit that referenced this pull request Nov 10, 2015

@igrr igrr merged commit 450718b into esp8266:master Nov 10, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@eliabieri

This comment has been minimized.

Show comment
Hide comment
@eliabieri

eliabieri Nov 10, 2015

did this fix the error, where the arduino IDE always wanted a password?

Elia Bieri
whateveryouwant@eliabieri.com

eliabieri commented Nov 10, 2015

did this fix the error, where the arduino IDE always wanted a password?

Elia Bieri
whateveryouwant@eliabieri.com

@me-no-dev

This comment has been minimized.

Show comment
Hide comment
@me-no-dev

me-no-dev Nov 10, 2015

Collaborator

no. This fixes everything else and adds security
when/if the IDE changes are accepted then upload will work as it is supposed to and ask for password only if you have secured OTA running.
The reason you get asked for password is because by default only Yuns use the network and they are SSH secured

Collaborator

me-no-dev commented Nov 10, 2015

no. This fixes everything else and adds security
when/if the IDE changes are accepted then upload will work as it is supposed to and ask for password only if you have secured OTA running.
The reason you get asked for password is because by default only Yuns use the network and they are SSH secured

@mangelajo

This comment has been minimized.

Show comment
Hide comment
@mangelajo

mangelajo Nov 10, 2015

Contributor

niiiice!! 👍

Contributor

mangelajo commented on cores/esp8266/Updater.cpp in a8976a0 Nov 10, 2015

niiiice!! 👍

@mangelajo

This comment has been minimized.

Show comment
Hide comment
@mangelajo

mangelajo Nov 10, 2015

Contributor

couldn't this buffer overflow the _md5 var?

Contributor

mangelajo commented on libraries/ArduinoOTA/ArduinoOTA.cpp in a8976a0 Nov 10, 2015

couldn't this buffer overflow the _md5 var?

This comment has been minimized.

Show comment
Hide comment
@me-no-dev

me-no-dev Nov 10, 2015

Collaborator

there should be more changes to this. Seems like my sanity checks are missing.. I'm sure I pushed them

Collaborator

me-no-dev replied Nov 10, 2015

there should be more changes to this. Seems like my sanity checks are missing.. I'm sure I pushed them

@mangelajo

This comment has been minimized.

Show comment
Hide comment
@mangelajo

mangelajo Nov 10, 2015

Contributor

cool! :)

Contributor

mangelajo commented on libraries/ArduinoOTA/ArduinoOTA.cpp in a8976a0 Nov 10, 2015

cool! :)

@mangelajo

This comment has been minimized.

Show comment
Hide comment
@mangelajo

mangelajo Nov 10, 2015

Contributor

we could make this a bit more stateless (re-entering the above block in case of auth never arrived the previous time) by including a header at the stat of the packet saying if it's initial request or auth.

Contributor

mangelajo commented on libraries/ArduinoOTA/ArduinoOTA.cpp in a8976a0 Nov 10, 2015

we could make this a bit more stateless (re-entering the above block in case of auth never arrived the previous time) by including a header at the stat of the packet saying if it's initial request or auth.

This comment has been minimized.

Show comment
Hide comment
@me-no-dev

me-no-dev Nov 10, 2015

Collaborator

see my comment above... same story already in the code

Collaborator

me-no-dev replied Nov 10, 2015

see my comment above... same story already in the code

@mangelajo

This comment has been minimized.

Show comment
Hide comment
@mangelajo

mangelajo Nov 10, 2015

Contributor

easier for the user!, nice too! ;)

easier for the user!, nice too! ;)

@mangelajo

This comment has been minimized.

Show comment
Hide comment
@mangelajo

mangelajo Nov 10, 2015

Contributor

why only two hex bytes?, hostname is cheap, let's add them all and avoid collisions.

Contributor

mangelajo commented on libraries/ArduinoOTA/ArduinoOTA.cpp in a8976a0 Nov 10, 2015

why only two hex bytes?, hostname is cheap, let's add them all and avoid collisions.

This comment has been minimized.

Show comment
Hide comment
@me-no-dev

me-no-dev Nov 10, 2015

Collaborator

typo :)

Collaborator

me-no-dev replied Nov 10, 2015

typo :)

@mangelajo

This comment has been minimized.

Show comment
Hide comment
@mangelajo

mangelajo Nov 10, 2015

Contributor

May be I'd increase that timeout, a packet loss with TCP could trip for a few seconds.

Contributor

mangelajo commented on libraries/ArduinoOTA/ArduinoOTA.cpp in a8976a0 Nov 10, 2015

May be I'd increase that timeout, a packet loss with TCP could trip for a few seconds.

This comment has been minimized.

Show comment
Hide comment
@me-no-dev

me-no-dev Nov 10, 2015

Collaborator

Sure, but let's test it first :) this is a few seconds as is

Collaborator

me-no-dev replied Nov 10, 2015

Sure, but let's test it first :) this is a few seconds as is

@mangelajo

This comment has been minimized.

Show comment
Hide comment
@mangelajo

mangelajo Nov 10, 2015

Contributor

Awesome work, some comments, but WOW! :)

This is real collaborative work! :)

Contributor

mangelajo commented on a8976a0 Nov 10, 2015

Awesome work, some comments, but WOW! :)

This is real collaborative work! :)

This comment has been minimized.

Show comment
Hide comment
@me-no-dev

me-no-dev Nov 10, 2015

Collaborator

yeah, look at the final code. Will have the sanity checks + new command for auth. Maybe you are commenting on a particular commit and not on the whole pull request?

Collaborator

me-no-dev replied Nov 10, 2015

yeah, look at the final code. Will have the sanity checks + new command for auth. Maybe you are commenting on a particular commit and not on the whole pull request?

This comment has been minimized.

Show comment
Hide comment
@hallard

hallard Nov 11, 2015

Contributor

great jobs guys,
Any chance to put the bargraph on espota.py before PR, this will avoid me to wait merge and do another PR ?
changes are really trivials
add this func anywhere you want

# update_progress() : Displays or updates a console progress bar
## Accepts a float between 0 and 1. Any int will be converted to a float.
## A value under 0 represents a 'halt'.
## A value at 1 or bigger represents 100%
def update_progress(progress):
    barLength = 60 # Modify this to change the length of the progress bar
    status = ""
    if isinstance(progress, int):
        progress = float(progress)
    if not isinstance(progress, float):
        progress = 0
        status = "error: progress var must be float\r\n"
    if progress < 0:
        progress = 0
        status = "Halt...\r\n"
    if progress >= 1:
        progress = 1
        status = "Done...\r\n"
    block = int(round(barLength*progress))
    text = "\rUploading: [{0}] {1}% {2}".format( "="*block + " "*(barLength-block), int(progress*100), status)
    sys.stdout.write(text)
    sys.stdout.flush()

then the main transfer loop becomes with calls to update_progress and offset var + that's all

try:
    f = open(filename, "rb")
    update_progress(0)
    #sys.stderr.write('Uploading')
    #sys.stderr.flush()
    offset = 0
    while True:
      chunk = f.read(1460)
      if not chunk: break
      offset += len(chunk)
      update_progress(offset/float(content_size))
      #sys.stderr.write('.')
      #sys.stderr.flush()
      connection.settimeout(10)
      try:
Contributor

hallard replied Nov 11, 2015

great jobs guys,
Any chance to put the bargraph on espota.py before PR, this will avoid me to wait merge and do another PR ?
changes are really trivials
add this func anywhere you want

# update_progress() : Displays or updates a console progress bar
## Accepts a float between 0 and 1. Any int will be converted to a float.
## A value under 0 represents a 'halt'.
## A value at 1 or bigger represents 100%
def update_progress(progress):
    barLength = 60 # Modify this to change the length of the progress bar
    status = ""
    if isinstance(progress, int):
        progress = float(progress)
    if not isinstance(progress, float):
        progress = 0
        status = "error: progress var must be float\r\n"
    if progress < 0:
        progress = 0
        status = "Halt...\r\n"
    if progress >= 1:
        progress = 1
        status = "Done...\r\n"
    block = int(round(barLength*progress))
    text = "\rUploading: [{0}] {1}% {2}".format( "="*block + " "*(barLength-block), int(progress*100), status)
    sys.stdout.write(text)
    sys.stdout.flush()

then the main transfer loop becomes with calls to update_progress and offset var + that's all

try:
    f = open(filename, "rb")
    update_progress(0)
    #sys.stderr.write('Uploading')
    #sys.stderr.flush()
    offset = 0
    while True:
      chunk = f.read(1460)
      if not chunk: break
      offset += len(chunk)
      update_progress(offset/float(content_size))
      #sys.stderr.write('.')
      #sys.stderr.flush()
      connection.settimeout(10)
      try:

This comment has been minimized.

Show comment
Hide comment

This comment has been minimized.

Show comment
Hide comment
@hallard

hallard Nov 11, 2015

Contributor

Excellent, thanks :-)

Contributor

hallard replied Nov 11, 2015

Excellent, thanks :-)

@sticilface

This comment has been minimized.

Show comment
Hide comment
@sticilface

sticilface Nov 11, 2015

Contributor

The new OTA, defines extern ArduinoOTAClass ArduinoOTA;
Im working on a settings manager that allowed you to start and stop the OTA with the flick of a switch. say you have lots of ESPs... This used new and delete to create the object dynamically. my knowledge of c++ is not good enough to know if 1) you can override that or 2) if i used

       ota_server = new ArduinoOTAClass; 

would that effectively mean i have two objects ota_server & ArduinoOTA in memory? i.e. a waste or resources?

Contributor

sticilface commented Nov 11, 2015

The new OTA, defines extern ArduinoOTAClass ArduinoOTA;
Im working on a settings manager that allowed you to start and stop the OTA with the flick of a switch. say you have lots of ESPs... This used new and delete to create the object dynamically. my knowledge of c++ is not good enough to know if 1) you can override that or 2) if i used

       ota_server = new ArduinoOTAClass; 

would that effectively mean i have two objects ota_server & ArduinoOTA in memory? i.e. a waste or resources?

@igrr

This comment has been minimized.

Show comment
Hide comment
@igrr

igrr Nov 11, 2015

Member

Today I was actually thinking of reverting this change — I don't think we
should have ArduinoOTA instance allocated by default.

The pattern of statically allocated instances works well when these
instances map to hardware resources (e.g Serial and SPI). For things such
as network services this pattern is less applicable, and this becomes
obvious when application complexity passes the level of "I only initialize
things once".

On Wed, Nov 11, 2015, 21:54 sticilface notifications@github.com wrote:

The new OTA, defines 'extern ArduinoOTAClass ArduinoOTA;'
Im working on a settings manager that allowed you to start and stop the
OTA with the flick of a switch. say you have lots of ESPs... This used new
and delete to create the object dynamically. my knowledge of c++ is not
good enough to know if 1) you can override that or 2) if i used

   ota_server = new ArduinoOTAClass;

would that effectively mean i have two objects ota_server & ArduinoOTA in
memory? i.e. a waste or resources?


Reply to this email directly or view it on GitHub
#980 (comment).

Member

igrr commented Nov 11, 2015

Today I was actually thinking of reverting this change — I don't think we
should have ArduinoOTA instance allocated by default.

The pattern of statically allocated instances works well when these
instances map to hardware resources (e.g Serial and SPI). For things such
as network services this pattern is less applicable, and this becomes
obvious when application complexity passes the level of "I only initialize
things once".

On Wed, Nov 11, 2015, 21:54 sticilface notifications@github.com wrote:

The new OTA, defines 'extern ArduinoOTAClass ArduinoOTA;'
Im working on a settings manager that allowed you to start and stop the
OTA with the flick of a switch. say you have lots of ESPs... This used new
and delete to create the object dynamically. my knowledge of c++ is not
good enough to know if 1) you can override that or 2) if i used

   ota_server = new ArduinoOTAClass;

would that effectively mean i have two objects ota_server & ArduinoOTA in
memory? i.e. a waste or resources?


Reply to this email directly or view it on GitHub
#980 (comment).

@sticilface

This comment has been minimized.

Show comment
Hide comment
@sticilface

sticilface Nov 11, 2015

Contributor

Ah ok. Good to know. thanks

For future reference having two objects, even if one is never used double the memory usage?

Contributor

sticilface commented Nov 11, 2015

Ah ok. Good to know. thanks

For future reference having two objects, even if one is never used double the memory usage?

@me-no-dev

This comment has been minimized.

Show comment
Hide comment
@me-no-dev

me-no-dev Nov 11, 2015

Collaborator

but looked at it differently, you can have only one ArduinoOTA and you allocate it when you include the header

Collaborator

me-no-dev commented Nov 11, 2015

but looked at it differently, you can have only one ArduinoOTA and you allocate it when you include the header

@me-no-dev

This comment has been minimized.

Show comment
Hide comment
@me-no-dev

me-no-dev Nov 11, 2015

Collaborator

So even though it's not directly "hardware" related, it still can exist only once. Any other situation will lead to unknown results. You can not have more than one UDP listener on a particular port and even if you could have more, you could not know which one will get the packet and so on. Therefore I strongly believe that in this case a global object should be used (I think I even made it so it does not init twice).
Surely a method to stop the OTA can be added, but what will you actually stop? MDNS, the UDP or?
This is the only reason I made it global: it can have only one instance running at any given time.

Collaborator

me-no-dev commented Nov 11, 2015

So even though it's not directly "hardware" related, it still can exist only once. Any other situation will lead to unknown results. You can not have more than one UDP listener on a particular port and even if you could have more, you could not know which one will get the packet and so on. Therefore I strongly believe that in this case a global object should be used (I think I even made it so it does not init twice).
Surely a method to stop the OTA can be added, but what will you actually stop? MDNS, the UDP or?
This is the only reason I made it global: it can have only one instance running at any given time.

@sticilface

This comment has been minimized.

Show comment
Hide comment
@sticilface

sticilface Nov 15, 2015

Contributor

I agree, only one instance running at a time. My two cents is that i've now got ~ 9 of these in the home, more if u count the ones not running the arduino code... so it means i've got a lot of OTA stuff going on, in that menu. My settings manager allowed me to turn off OTA by deleting a dynamically created OTA object. ( i also see your point looking at the MDNS stuff that there is no destructor , or remove service options, once it is added). This meant I can just have the unstable / projects I'm working on show up. means I'm less likely to go and flash the wrong device. I have to go enable it, then flash it.

so +1 for stop method. New lib is ace btw. much better!

Contributor

sticilface commented Nov 15, 2015

I agree, only one instance running at a time. My two cents is that i've now got ~ 9 of these in the home, more if u count the ones not running the arduino code... so it means i've got a lot of OTA stuff going on, in that menu. My settings manager allowed me to turn off OTA by deleting a dynamically created OTA object. ( i also see your point looking at the MDNS stuff that there is no destructor , or remove service options, once it is added). This meant I can just have the unstable / projects I'm working on show up. means I'm less likely to go and flash the wrong device. I have to go enable it, then flash it.

so +1 for stop method. New lib is ace btw. much better!

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