Handle unaligned address in EspClass::flashWrite u8 overload #8605
Conversation
|
I guessed that this issue could be solved with a good refactor of the whole spi_flash_write_ methods. Sure I will vote for your pull request, my intention was proposing a punctual fix, modifying as little as possible, that the overall refactor is the way to go! |
cores/esp8266/Esp.cpp
Outdated
| if (!flashReplaceBlock(address + offset, data + offset, sizeLeft)) { | ||
| return false; | ||
| while (size > 0) { | ||
| auto len = std::min(size, sizeof(buf)); |
There was a problem hiding this comment.
Maybe have a double check on what is returned by sizeof(buf) here?
I always find it tricky to see for myself whether the sizeof is returning the size of the array or the size of a pointer.
In this instance, it should be FLASH_PAGE_SIZE
There was a problem hiding this comment.
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 be std::size(buf); instead
There was a problem hiding this comment.
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.
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.
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.
puya supports looks fine as well?
There was a problem hiding this comment.
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?
|
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 |
|
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.
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...)