Fix flasher::sync

[pavlus]

• connection::read now loops until all data have been read
• connection is flushed between Command::Sync sent and response is read
• Fixes espflash does not wait between writing a sync command and reading the response #116

connection::read changes are not required, adding flush() is enough, but buffering with shared state looks unnecessary anyway.

Should I rollback it, or this is fine?

[bugadani]

What will happen if I try to connect, but there is no ESP32 attached to my usb-serial adapter? Do we just hang in the infinite loop? If yes, doing that inside a retry-100-times loop seems a bit risky. Other than that, I like how much simpler this patch is!

[MabezDev]

Thanks! #117 worked nearly perfectly, but would sometimes get into a unrecoverable state. I've tried with this PR over 30 times and its working perfectly every time.

@bugadani is correct though, we shouldn't retry in an infinite loop. We should probably time out after X milliseconds, I'm not sure one what a good value of X is, perhaps we could look at the esptool.py source or just eyeball it.

[jessebraham]

From esptool.py:

DEFAULT_TIMEOUT = 3                   # timeout for most flash operations
START_FLASH_TIMEOUT = 20              # timeout for starting flash (may perform erase)
CHIP_ERASE_TIMEOUT = 120              # timeout for full chip erase
MAX_TIMEOUT = CHIP_ERASE_TIMEOUT * 2  # longest any command can run
SYNC_TIMEOUT = 0.1                    # timeout for syncing with bootloader

I would imagine DEFAULT_TIMEOUT is probably adequate, probably worth giving a shot.

[bugadani]

I guess my real question is: is the flush a sufficient fix? Because if it is, we can keep the VecDeque buffering and call it a day, the outer retry loops that already exist will catch any idling.

[pavlus]

@bugadani I've added logging of read size earlier, it was reading in x12 chunks on my ESP32 devkit (12 and 24 bytes at a time), so looks like it should be enough for its current task.

If there is no device on the other end, we will receive timeout error and it will be returned via ? (I assume that, because that's what happened before I've added flush(), so device was there but didn't bother to answer, because it hasn't received a command yet).

[pavlus]

The other situation I can imagine is when device responds slowly, then it may loop in iterations just shy of timeout, then Connection::read call may take longer than a timeout, because it buffers slowly, but not to the point of failure. Still, it will end eventually, when len bytes are read.

[bugadani]

You're right, it times out. I wasn't sure about it (I had to try & see, sorry :) ). Well then, thanks for fixing what I couldn't :)

[jessebraham]

Given that this seems to improve the success rate of flashing, I'm just going to go ahead and merge it as is. I do prefer not maintaining internal buffer state if it's not necessary.

There has been a fair amount of discussion regarding this, so I will leave the associated issue for the time being. Please move any further discussion to #116.