-
-
Notifications
You must be signed in to change notification settings - Fork 345
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
Rewrite flashmem driver to cope with misaligned RAM buffers #1490
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,102 +1,179 @@ | ||
#include "flashmem.h" | ||
#include <stdlib.h> | ||
/**** | ||
* Sming Framework Project - Open Source framework for high efficiency native ESP8266 development. | ||
* Created 2015 by Skurydin Alexey | ||
* http://github.com/SmingHub/Sming | ||
* All files of the Sming Core are provided under the LGPL v3 license. | ||
* | ||
* Based on NodeMCU platform_flash | ||
* https://github.com/nodemcu/nodemcu-firmware | ||
* | ||
* @author: 27/8/2018 - Mikee47 <[email protected]> | ||
* | ||
* Rewritten to deal with misaligned RAM buffers. | ||
* | ||
****/ | ||
|
||
// Based on NodeMCU platform_flash | ||
// https://github.com/nodemcu/nodemcu-firmware | ||
#include "flashmem.h" | ||
#include "espinc/peri.h" | ||
|
||
extern char _flash_code_end[]; | ||
|
||
uint32_t flashmem_write( const void *from, uint32_t toaddr, uint32_t size ) | ||
/* | ||
* To ensure memory alignment a temporary buffer is used by flashmem_read and flashmem_write functions. | ||
* | ||
* The buffer must be an integer multiple of INTERNAL_FLASH_WRITE_UNIT_SIZE. | ||
*/ | ||
#define FLASH_BUFFERS 32 | ||
|
||
/** @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) | ||
|
||
// Buffers need to be word aligned for flash access | ||
#define __aligned __attribute__((aligned(4))) | ||
|
||
// If this module were in C++ we could use std::min | ||
static inline uint32_t min(uint32_t a, uint32_t b) | ||
{ | ||
uint32_t temp, rest, ssize = size; | ||
unsigned i; | ||
char tmpdata[ INTERNAL_FLASH_WRITE_UNIT_SIZE ]; | ||
const uint8_t *pfrom = ( const uint8_t* )from; | ||
const uint32_t blksize = INTERNAL_FLASH_WRITE_UNIT_SIZE; | ||
const uint32_t blkmask = INTERNAL_FLASH_WRITE_UNIT_SIZE - 1; | ||
|
||
// Align the start | ||
if( toaddr & blkmask ) | ||
{ | ||
rest = toaddr & blkmask; | ||
temp = toaddr & ~blkmask; // this is the actual aligned address | ||
flashmem_read_internal( tmpdata, temp, blksize ); | ||
for( i = rest; size && ( i < blksize ); i ++, size --, pfrom ++ ) | ||
tmpdata[ i ] = *pfrom; | ||
flashmem_write_internal( tmpdata, temp, blksize ); | ||
if( size == 0 ) | ||
return ssize; | ||
toaddr = temp + blksize; | ||
} | ||
// The start address is now a multiple of blksize | ||
// Compute how many bytes we can write as multiples of blksize | ||
rest = size & blkmask; | ||
temp = size & ~blkmask; | ||
// Program the blocks now | ||
if( temp ) | ||
{ | ||
flashmem_write_internal( pfrom, toaddr, temp ); | ||
toaddr += temp; | ||
pfrom += temp; | ||
} | ||
// And the final part of a block if needed | ||
if( rest ) | ||
{ | ||
flashmem_read_internal( tmpdata, toaddr, blksize ); | ||
for( i = 0; size && ( i < rest ); i ++, size --, pfrom ++ ) | ||
tmpdata[ i ] = *pfrom; | ||
flashmem_write_internal( tmpdata, toaddr, blksize ); | ||
} | ||
return ssize; | ||
return (a < b) ? a : b; | ||
} | ||
|
||
uint32_t flashmem_read( void *to, uint32_t fromaddr, uint32_t size ) | ||
uint32_t flashmem_write(const void* from, uint32_t toaddr, uint32_t size) | ||
{ | ||
uint32_t temp, rest, ssize = size; | ||
unsigned i; | ||
char tmpdata[ INTERNAL_FLASH_READ_UNIT_SIZE ]; | ||
uint8_t *pto = ( uint8_t* )to; | ||
const uint32_t blksize = INTERNAL_FLASH_READ_UNIT_SIZE; | ||
const uint32_t blkmask = INTERNAL_FLASH_READ_UNIT_SIZE - 1; | ||
|
||
// Align the start | ||
if( fromaddr & blkmask ) | ||
{ | ||
rest = fromaddr & blkmask; | ||
temp = fromaddr & ~blkmask; // this is the actual aligned address | ||
flashmem_read_internal( tmpdata, temp, blksize ); | ||
for( i = rest; size && ( i < blksize ); i ++, size --, pto ++ ) | ||
*pto = tmpdata[ i ]; | ||
|
||
if( size == 0 ) | ||
return ssize; | ||
fromaddr = temp + blksize; | ||
} | ||
// The start address is now a multiple of blksize | ||
// Compute how many bytes we can read as multiples of blksize | ||
rest = size & blkmask; | ||
temp = size & ~blkmask; | ||
// Program the blocks now | ||
if( temp ) | ||
{ | ||
flashmem_read_internal( pto, fromaddr, temp ); | ||
fromaddr += temp; | ||
pto += temp; | ||
} | ||
// And the final part of a block if needed | ||
if( rest ) | ||
{ | ||
flashmem_read_internal( tmpdata, fromaddr, blksize ); | ||
for( i = 0; size && ( i < rest ); i ++, size --, pto ++ ) | ||
*pto = tmpdata[ i ]; | ||
} | ||
return ssize; | ||
if(IS_ALIGNED(from) && IS_ALIGNED(toaddr) && IS_ALIGNED(size)) | ||
return flashmem_write_internal(from, toaddr, size); | ||
|
||
const uint32_t blksize = INTERNAL_FLASH_WRITE_UNIT_SIZE; | ||
const uint32_t blkmask = INTERNAL_FLASH_WRITE_UNIT_SIZE - 1; | ||
|
||
__aligned char tmpdata[FLASH_BUFFERS * INTERNAL_FLASH_WRITE_UNIT_SIZE]; | ||
const uint8_t* pfrom = (const uint8_t*)from; | ||
uint32_t remain = size; | ||
|
||
// Align the start | ||
if(toaddr & blkmask) | ||
{ | ||
uint32_t rest = toaddr & blkmask; | ||
uint32_t addr_aligned = toaddr & ~blkmask; // this is the actual aligned address | ||
|
||
// Read existing unit and overlay with new data | ||
if(flashmem_read_internal(tmpdata, addr_aligned, blksize) != blksize) | ||
return 0; | ||
while(remain && rest < blksize) | ||
{ | ||
tmpdata[rest++] = *pfrom++; | ||
--remain; | ||
} | ||
|
||
// Write the unit | ||
uint32_t written = flashmem_write_internal(tmpdata, addr_aligned, blksize); | ||
if(written != blksize) | ||
return written; | ||
|
||
if (remain == 0) | ||
return size; | ||
|
||
toaddr = addr_aligned + blksize; | ||
} | ||
|
||
// The start address is now a multiple of blksize | ||
// Compute how many bytes we can write as multiples of blksize | ||
uint32_t rest = remain & blkmask; | ||
remain &= ~blkmask; | ||
// Program the blocks now | ||
while(remain) | ||
{ | ||
unsigned count = min(remain, sizeof(tmpdata)); | ||
memcpy(tmpdata, pfrom, count); | ||
uint32_t written = flashmem_write_internal(tmpdata, toaddr, count); | ||
remain -= written; | ||
if(written != count) | ||
return size - remain; | ||
toaddr += count; | ||
pfrom += count; | ||
} | ||
|
||
// And the final part of a block if needed | ||
if(rest) | ||
{ | ||
if(flashmem_read_internal(tmpdata, toaddr, blksize) != blksize) | ||
return size - remain; | ||
unsigned i; | ||
for(i = 0; i < rest; ++i) | ||
tmpdata[i] = *pfrom++; | ||
uint32_t written = flashmem_write_internal(tmpdata, toaddr, blksize); | ||
remain -= written; | ||
if(written != blksize) | ||
return size - remain; | ||
} | ||
|
||
return size; | ||
} | ||
|
||
uint32_t flashmem_read(void* to, uint32_t fromaddr, uint32_t size) | ||
{ | ||
if(IS_ALIGNED(to) && IS_ALIGNED(fromaddr) && IS_ALIGNED(size)) | ||
return flashmem_read_internal(to, fromaddr, size); | ||
|
||
const uint32_t blksize = INTERNAL_FLASH_READ_UNIT_SIZE; | ||
const uint32_t blkmask = INTERNAL_FLASH_READ_UNIT_SIZE - 1; | ||
|
||
__aligned char tmpdata[FLASH_BUFFERS * INTERNAL_FLASH_READ_UNIT_SIZE]; | ||
uint8_t* pto = (uint8_t*)to; | ||
uint32_t remain = size; | ||
|
||
// Align the start | ||
if(fromaddr & blkmask) | ||
{ | ||
uint32_t rest = fromaddr & blkmask; | ||
uint32_t addr_aligned = fromaddr & ~blkmask; // this is the actual aligned address | ||
if (flashmem_read_internal(tmpdata, addr_aligned, blksize) != blksize) | ||
return 0; | ||
// memcpy(pto, &tmpdata[rest], std::min(blksize - rest, remain)) | ||
while(remain && rest < blksize) | ||
{ | ||
*pto++ = tmpdata[rest++]; | ||
--remain; | ||
} | ||
if (remain == 0) | ||
return size; | ||
fromaddr = addr_aligned + blksize; | ||
} | ||
|
||
// The start address is now a multiple of blksize | ||
// Compute how many bytes we can read as multiples of blksize | ||
uint32_t rest = remain & blkmask; | ||
remain &= ~blkmask; | ||
// Read the blocks now | ||
while(remain) | ||
{ | ||
unsigned count = min(remain, sizeof(tmpdata)); | ||
uint32_t read = flashmem_read_internal(tmpdata, fromaddr, count); | ||
memcpy(pto, tmpdata, read); | ||
remain -= read; | ||
if(read != count) | ||
return size - remain; | ||
fromaddr += count; | ||
pto += count; | ||
} | ||
|
||
// And the final part of a block if needed | ||
if(rest) | ||
{ | ||
if(flashmem_read_internal(tmpdata, fromaddr, blksize) != blksize) | ||
return size - remain; | ||
unsigned i; | ||
for(i = 0; i < rest; ++i) | ||
*pto++ = tmpdata[i]; | ||
} | ||
|
||
return size; | ||
} | ||
|
||
bool flashmem_erase_sector( uint32_t sector_id ) | ||
bool flashmem_erase_sector(uint32_t sector_id) | ||
{ | ||
WRITE_PERI_REG(0x60000914, 0x73); | ||
return spi_flash_erase_sector( sector_id ) == SPI_FLASH_RESULT_OK; | ||
WDT_FEED(); | ||
return spi_flash_erase_sector(sector_id) == SPI_FLASH_RESULT_OK; | ||
} | ||
|
||
SPIFlashInfo flashmem_get_info() | ||
|
@@ -149,9 +226,7 @@ uint16_t flashmem_get_size_sectors() | |
return flashmem_get_size_bytes() / SPI_FLASH_SEC_SIZE; | ||
} | ||
|
||
// Helper function: find the flash sector in which an address resides | ||
// Return the sector number, as well as the start and end address of the sector | ||
uint32_t flashmem_find_sector( uint32_t address, uint32_t *pstart, uint32_t *pend ) | ||
uint32_t flashmem_find_sector(uint32_t address, uint32_t *pstart, uint32_t *pend) | ||
{ | ||
// All the sectors in the flash have the same size, so just align the address | ||
uint32_t sect_id = address / INTERNAL_FLASH_SECTOR_SIZE; | ||
|
@@ -172,19 +247,11 @@ uint32_t flashmem_get_sector_of_address( uint32_t addr ) | |
|
||
uint32_t flashmem_write_internal( const void *from, uint32_t toaddr, uint32_t size ) | ||
{ | ||
SpiFlashOpResult r; | ||
const uint32_t blkmask = INTERNAL_FLASH_WRITE_UNIT_SIZE - 1; | ||
uint32_t *apbuf = NULL; | ||
if( ((uint32_t)from) & blkmask ){ | ||
apbuf = (uint32_t *)malloc(size); | ||
if(!apbuf) | ||
return 0; | ||
memcpy(apbuf, from, size); | ||
} | ||
WRITE_PERI_REG(0x60000914, 0x73); | ||
r = spi_flash_write(toaddr, apbuf?(uint32 *)apbuf:(uint32 *)from, size); | ||
if(apbuf) | ||
free(apbuf); | ||
assert(IS_ALIGNED(from) && IS_ALIGNED(toaddr) && IS_ALIGNED(size)); | ||
|
||
WDT_FEED(); | ||
|
||
SpiFlashOpResult r = spi_flash_write(toaddr, (uint32*)from, size); | ||
if(SPI_FLASH_RESULT_OK == r) | ||
return size; | ||
else{ | ||
|
@@ -195,9 +262,11 @@ uint32_t flashmem_write_internal( const void *from, uint32_t toaddr, uint32_t si | |
|
||
uint32_t flashmem_read_internal( void *to, uint32_t fromaddr, uint32_t size ) | ||
{ | ||
SpiFlashOpResult r; | ||
WRITE_PERI_REG(0x60000914, 0x73); | ||
r = spi_flash_read(fromaddr, (uint32 *)to, size); | ||
assert(IS_ALIGNED(to) && IS_ALIGNED(fromaddr) && IS_ALIGNED(size)); | ||
|
||
WDT_FEED(); | ||
|
||
SpiFlashOpResult r = spi_flash_read(fromaddr, (uint32*)to, size); | ||
if(SPI_FLASH_RESULT_OK == r) | ||
return size; | ||
else{ | ||
|
@@ -208,9 +277,9 @@ uint32_t flashmem_read_internal( void *to, uint32_t fromaddr, uint32_t size ) | |
|
||
uint32_t flashmem_get_first_free_block_address() | ||
{ | ||
if (_flash_code_end == NULL) | ||
if(_flash_code_end == NULL) | ||
{ | ||
debugf("_flash_code_end:%08x\n", (uint32_t)_flash_code_end); | ||
debugf("_flash_code_end is null"); | ||
return 0; | ||
} | ||
|
||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This question applies also to the other new lines where WDT_FEED() is called.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 somefileWrite()
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
andspiffs_check
. Everything else is done with buffering and callbacks so should be fine. For now, probably easier to leave them in.