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

Xmc flash 2 #7317

Merged
merged 5 commits into from
May 27, 2020
Merged

Xmc flash 2 #7317

merged 5 commits into from
May 27, 2020

Conversation

ChocolateFrogsNuts
Copy link
Contributor

Remove the unnecessary XMC chip support from eboot and verify the flash contents after copying new firmware.

Testing shows there should be no issue with the XMC chips in eboot because the flash access speed is always 20MHz, however verifying the content written will help identify any issues if they do occur.
The additional code is minimal, and as it only reads, the verify is very quick.

eboot is always run with the flash access speed set to 20MHz, so
there is no need for special treatment of XMC chips.
If the data written to flash is as expected, the line cmp:0 will be displayed
after the usual @cp:0 from eboot.
@Tech-TX
Copy link
Contributor

Tech-TX commented May 19, 2020

That works with both failing cases for me (OTA upload and ESP.restart() at 160MHz),
#7307
#7267

I've already merged #7277 into my local fork, so I'm not sure if #7267 is needed in addition to this PR.

@devyte
Copy link
Collaborator

devyte commented May 19, 2020

@Tech-TX #7277 was a rollback of the first implementation. This should be the fixed final solution.

For some reason this was an issue during the reboot after an OTA update.
Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code wise it looks good.

Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'm not sure what the point of checking is. The way the code is written, you'll get an error, print a failure code, clear the command buffer and then SWRESET. At that time it'll just LOAD_APP anyway, same as if it passed verification.

That said, I can't give a much better idea of what to do in that case, anyway. Don't want to continually overwrite (i.e. don't clear the cmd so next reboot will try writing again) as you could end up in a hard reboot-write-reboot-write...loop and destroy the flash.

@devyte devyte merged commit 51daecc into esp8266:master May 27, 2020
@devyte
Copy link
Collaborator

devyte commented May 27, 2020

The verify is supposed to cover the case when the flash write seems to succeed, i. e. no error, but in truth it didn't take at hw level due to whatever reason.

@devyte devyte added this to the 2.7.2 milestone May 27, 2020
@ChocolateFrogsNuts
Copy link
Contributor Author

@earlephilhower the check is relatively cheap (in terms of code size and execution time) and if we do have problems with OTA updates to XMC chips will make it easy to identify if it is a problem with writing to the chip.
Theoretically we could store ACTION_COPY_FAILED in the RTC and have that cause printing the error and a while(true){}
That way if an OTA update copy failed, the chip would halt, then you could connect to the USB and press reset to get the reason dumped to monitor. If the eboot command is moved to a non-volatile location, it would even work over power cycles (although you would need to clear that the eboot command as part of a USB update).

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

Successfully merging this pull request may close these issues.

4 participants