MidiBoot doesn't handle large firmware image correctly

[antisvin]

I've ran into this issue on bootloader fork for Daisy, but it's identical to upstream in this regards. Here's what happens:

1. When first sysex buffer is processed, this code works correctly and returns error: https://github.com/pingdynasty/OpenWare/blob/develop/MidiBoot/Src/MidiBoot.cpp#L160-L175

2. However, this check exits too early, which means that buffer pointer is NOT set to correct location - https://github.com/pingdynasty/OpenWare/blob/master/Source/FirmwareLoader.hpp#L81-L82 . This should be among things that needs fixing

3. While we set error on first received buffer, remaining data will still be received and stored. Which causes incoming data to reach invalid address eventually on Daisy, probably would be the same on Owl.

So the fix would be something like - always set buffer pointer (even on error) and stop processing data if stored data offset is >= MAX_FIRMWARE_SIZE

I can probably make a fix for this myself unless someone won't let me know he's doing it out of shame ;-)

[pingdynasty]

The idea is that clearing the package index makes all subsequent messages invalid, but I can see that is not necessarily happening. How about beginFirmwareUpload sets packageIndexto 1, and the condition is changed to

if(packageIndex++ != idx)
      return setError("SysEx package out of sequence"); // out of sequence package

In other words, packageIndex holds the expected value of the next package, instead of the last received one.

[antisvin]

Yeah this sounds good, it looks like that would let us distinguish packages that have successfully returned from upload begin procedure. I can test and make a PR for this.

IIRC, the issue was that I was getting a hardfault from sending a large sysex file, will try to reproduce this first.

[pingdynasty]

The only minor problem I see is that instead of just a single 'too large' error, there will be a bunch of 'out of sequence' errors reported.

[antisvin]

Maybe we can change handleFirmwareUpload to ignore intermediate packages silently. I.e. we could have the error message about corrupt sysex stream to be generated only when last package (that would call finishFirmwareUpload normally) is received. This would help us avoid errors if there's some junk data sent by OS before we send the actual sysex data.

[pingdynasty]

I think we can do that simply by suppressing the 'out of sequence' error message. The RUN, STORE, FLASH, SAVE commands all check that loader is ready (ie last package received and crc valid). So we'll get 'Invalid STORE command', or 'No program to run'.