search

tinyfpga / TinyFPGA-Bootloader

An open source USB bootloader for FPGAs
Apache License 2.0
357 stars 94 forks source link

Make sure pretty_hex works under Python 2 and Python 3. #49

Closed mithro closed 5 years ago

mithro commented 5 years ago

Fixes #48.

mithro commented 5 years ago

Will merge if CI confirms the doctests are run and it goes green.

ewenmcneill commented 5 years ago

@mithro,

FYI, we never did decide that we'd release 1.0.24 as a production release. The various requests for testing (eg, https://discourse.tinyfpga.com/t/tinyprog-1-0-24-testing/886 and https://github.com/tinyfpga/TinyFPGA-Bootloader/issues/25#issuecomment-455375224) didn't really get much response. AFAICR the only 1.0.24 issue found was someone who had trouble with the 1.0.24devNN version string containing letters, but it's a bit inconclusive (eg, probably only litex-buildenv really does testing of 1.0.24 in practice).

Given the various issues people have had, possibly we should consider what would be required to be happy to make a 1.0.24 actual (non-dev) release, so more people get to use that version?

Also FWIW, my guess is that a bunch of these errors people are seeing -- https://github.com/tinyfpga/TinyFPGA-Bootloader/issues/48, https://github.com/tinyfpga/TinyFPGA-Bootloader/issues/18, https://github.com/tinyfpga/TinyFPGA-Bootloader/issues/42, https://github.com/tinyfpga/TinyFPGA-Bootloader/issues/43 -- are actually the result of shorter-than-expected reads, due to race conditions in the serial IO code, which seems to assume that a read of N bytes will either all happen or none of it will happen, but that's definitely not true on any unix/linux; unix/linux will happily supply 1-N bytes if it has some of them now. I suspect most of the issues reported are in the bootloader update simply because if it crashes out at the wrong point (stage 2 seems the worst), then it bricks the board to the point of needing a SPI programmer -- see https://discourse.tinyfpga.com/t/error-while-updating-bootloader-invalid-literal-for-int-with-base-10/484/3. But I still seem them periodically even with uploading regular code. (I don't think 1.0.24devNN is any worse for this than 1.0.23, perhaps better for it, hence this comment; but I do suspect that Python 3 might be somewhat worse for short reads than Python 2, possibly just due to being faster.)

Ewen

ewenmcneill commented 5 years ago

@mithro Re shorter than expected reads, see also https://github.com/tinyfpga/TinyFPGA-Bootloader/issues/42#issuecomment-467376884, where someone modified their own version of tinyprog to handle short reads. They just retry the command, but in practice I suspect the low level read() ought to be tried again until the right number of bytes have been received, as is usual C practice for using read().

Ewen

PS: Feel free to, eg, move these comments to a new issue if you'd prefer.

ewenmcneill commented 5 years ago

@mithro Also, in Python 3 bytearray and "" are pretty different things, but in Python 2 they're the equivalent. So https://github.com/tinyfpga/TinyFPGA-Bootloader/blob/99f87a5947fada082cc11606a2b3370f50e1f970/programmer/tinyprog/__init__.py#L159-L165 is probably non-portable. I wonder if that's the trigger for a bunch of these issues on Python 3? Maybe the length = 0 case should explicitly return an empty bytearray?

Ewen

ewenmcneill commented 5 years ago

@mithro pyserial read() is explicitly documented as "Read size bytes from the serial port. If a timeout is set it may return less characters as requested. With no timeout it will block until the requested number of bytes is read.". And the programming tool explicitly opens the port with a 1 second timeout, which means short reads quite possible (writes will raise an exception instead).

But AFAICT the code at https://github.com/tinyfpga/TinyFPGA-Bootloader/blob/99f87a5947fada082cc11606a2b3370f50e1f970/programmer/tinyprog/__init__.py#L339-L350 which calls https://github.com/tinyfpga/TinyFPGA-Bootloader/blob/99f87a5947fada082cc11606a2b3370f50e1f970/programmer/tinyprog/__init__.py#L297-L304 just assumes that it read everything if it didn't get an exception, and doesn't check for/retry short reads. Which probably leaves the serial stream out of sync, causing things to get worse from there (including these pretty print issues).

Ewen

PS: Sorry this all ended up in a merged pull request :-) It was how I found the references, and then started looking at the code and at the beginning it was just going to be a quick comment in passing....