Rewrite flashmem driver to cope with misaligned RAM buffers #1490
Conversation
mikee47
changed the title
Sming/system/flashmem.c
Outdated
|
|
||
| /** @brief determines if the given value is aligned to a word (4-byte) boundary */ | ||
| #undef IS_ALIGNED | ||
| #define IS_ALIGNED(_x) (((uint32_t)(_x)&0x00000003) == 0) |
Sming/system/flashmem.c
Outdated
|
|
||
| #else | ||
|
|
||
| static inline uint32_t _flashmem_write_internal(const void* from, uint32_t toaddr, uint32_t size) |
There was a problem hiding this comment.
_flashmem_write_internal -> flashmem_write_internal .
Sming/system/flashmem.c
Outdated
| } | ||
|
|
||
| bool flashmem_erase_sector( uint32_t sector_id ) | ||
| static inline uint32_t _flashmem_read_internal(void* to, uint32_t fromaddr, uint32_t size) |
There was a problem hiding this comment.
_flashmem_read_internal -> flashmem_read_internal. Make appropriate fixes If there are also other places in this PR where the name of the function ot the variables start or end with underscore.
There was a problem hiding this comment.
OK, but we'll have to leave _flash_code_end though
There was a problem hiding this comment.
OK, but we'll have to leave _flash_code_end though
Yes, that's correct.
|
|
||
| bool flashmem_erase_sector(uint32_t sector_id) | ||
| { | ||
| WDT_FEED(); |
There was a problem hiding this comment.
Is WDT_FEED() really needed? I am just curious if you have experienced any issues with flash sector erasing causing the software watchdog to kick in?
There was a problem hiding this comment.
This question applies also to the other new lines where WDT_FEED() is called.
There was a problem hiding this comment.
I haven't had any issues, but then this was always in the code as WRITE_PERI_REG(0x60000914, 0x73);. I had to dig around to find out what that actually did, turned out there was a useful definition for it.
There was a problem hiding this comment.
I think the main risk from watchdog timeouts is with spiffs_format. Erase/write operations for normal file access would normally be much shorter, unlikely enough to trip a watchdog. I timed some fileWrite() operations and chunks of about 14K take no more than 3ms. If SPIFFS had to run garbage collection that could extend the time considerably. I'll leave the timing code in place and see what pops up in normal usage.
There was a problem hiding this comment.
My previous statement about 3ms, ignore please, it's wrong! I spent a little more time on this today and put timing checks on fileWrite. Writes take an average of about 10us per byte, reads no more than 1us. Unlikely to write more than 10K in one operation (RAM limitations) so, with overheads, maybe 150ms for a fileWrite, 15ms for a fileRead.
Given these figures, the WDT seems un-necessary. If we wanted to remove them we'd need to take a closer look at spiffs_format and spiffs_check. Everything else is done with buffering and callbacks so should be fine. For now, probably easier to leave them in.
|
@mikee47 Is there any performance penalty coming with that PR? |
I've timed various fileRead() and fileWrite() operations using multiple read/writes of the same file, and averaged the values. There is no observable difference between the old and new code. Here's a simple test you can try to illustrate why the revisions are necessary. Try reading some data from the start of flash memory: Because of the misaligned memory buffer the old driver just hangs. |
Perfect, thanks. I will try to test it myself next week and if there is no performance drawback will merge the PR. |
…ar flashmem_read_internal and flashmem_write_internal calls. These don't really do much as the compiler will optimise the internal functions in release mode anyway.
spiffs_check just hangs without this. No exception raised.
Required to support random read/write. Note SPIFFS uses block accesses in normal operation so this problem wouldn't normally present.