- Notifications
You must be signed in to change notification settings - Fork13.3k
Handle unaligned address in EspClass::flashWrite u8 overload#8605
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
fabianoriccardi commentedJun 17, 2022
I guessed that this issue could be solved with a good refactor of the wholespi_flash_write_ methods. Sure I will vote for your pull request, my intention was proposing apunctual fix, modifying as little as possible, that the overall refactor is the way to go! |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
cores/esp8266/Esp.cpp Outdated
| if (!flashReplaceBlock(address + offset, data + offset, sizeLeft)) { | ||
| returnfalse; | ||
| while (size >0) { | ||
| auto len =std::min(size,sizeof(buf)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe have a double check on what is returned bysizeof(buf) here?
I always find it tricky to see for myself whether thesizeof is returning the size of the array or the size of a pointer.
In this instance, it should beFLASH_PAGE_SIZE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No point guessing when IDE / language server in any editor will display this b/c it's a compile time value :)
But I suppose this could bestd::size(buf); instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Well my "code review IDE" (also called "brain") is lacking this feature. ;) It is only highlighting code which looks like code I have spent numerous hours on to fix in the past where I missed such a mistake.
Apart from that you still need to hover your mouse on those parts of the code in an IDE that does show you what it will do and that was my suggestion, to actually do that ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Which is why I mentioned the IDE as a tool to help with guesses, since we don't have this issue here. Review with a real test case might've helped too, would you mind re-checking with some ESPeasy builds if it's possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I did just now test ESPEasy with the code changes of this PR.
It does seem to work fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
puya supports looks fine as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
puya supports looks fine as well?
I don't have any Puya device here at hand, so have not tested it.
If you like, I can have another look at the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please do.
Uh oh!
There was an error while loading.Please reload this page.
mcspr commentedJun 27, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
re. does it work with u8 data, we do have these test cases for various offsets PR adds an additional case, which fails with the current master |
d-a-v left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
device test started & passed
fabianoriccardi commentedJul 2, 2022
I've tested the new commits and they solve#8372. Hoping they will be integrated soon in the main line. |
esp8266#8605)Separate page handling logic and the actual writing. Make sure we place both unaligned src and dest into a buffer.Fixes edge-case introduced for SPIFFS that exclusively works through unaligned flash write function.This copies the behaviour of official RTOS port, but does not change the original spi_flash_write.
Uh oh!
There was an error while loading.Please reload this page.
another approach at fixing#8372
resolves#8588@fabianoriccardi
instead of changing the branch, just break the flow on a lower level when actually writing things
both unaligned src and dest are placed into buffer. this copies behaviour of RTOS functions that I mentioned in the issue
example sketch seems to work. I just hope I did not break something else while fixing spiffs
(...and geez, how much unaligned writes it does. no wonder it's slow...)