Skip to content

Dynamically allocate the SD sector cache on use, save ~512 bytes when SD not used #4204

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.

Sign up for GitHub

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

Closed
wants to merge 8 commits into from
Closed

Dynamically allocate the SD sector cache on use, save ~512 bytes when SD not used #4204

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
35dd992
Dynamically allocate the SD sector cache on use
earlephilhower Jan 18, 2018
5cd00db
Revert accidental change to platform.txt
earlephilhower Jan 21, 2018
bfc025b
Merge branch 'master' into dynamic_sd_cache
earlephilhower Mar 8, 2018
1d9af16
Merge branch 'master' into dynamic_sd_cache
devyte Mar 20, 2018
6c12968
Merge branch 'master' into dynamic_sd_cache
devyte Nov 14, 2018
22d8e3a
Control cache with refcnt, convert to unix format
earlephilhower Nov 18, 2018
faa2552
Merge branch 'master' into dynamic_sd_cache
earlephilhower Nov 18, 2018
8dcbf46
Fix typo
earlephilhower Nov 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions libraries/SD/src/utility/SdFat.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,14 +431,16 @@ union cache_t {
class SdVolume {
public:
/** Create an instance of SdVolume */
SdVolume(void) :allocSearchStart_(2), fatType_(0) {}
SdVolume(void) :allocSearchStart_(2), fatType_(0) { if (!cacheBuffer_) cacheBuffer_ = (cache_t *)malloc(sizeof(cache_t)); }
Copy link
Collaborator

Choose a reason for hiding this comment

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

null is not checked. I don't really know myself what to do in constructors when this happen.
Maybe you can also help with this in #4134 :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe errors in constructors can only throw an exception. Unfortunately, that's not the Arduino way, so I'm as at a loss as yourself.

The alternative here, and elsewhere, is to use lazy allocation and/or defensive. Before use of the cache it can be checked and the method can error out if it can't be allocated, assuming the API allows it. Perusing the SD headers, it looks like SEGV'ing is really the only thing that's possible using the current API.

Something like link time optimization (-flto) might eliminate the need for this kind of change as it should be able to detect no call graph into this static class variable, but I've not been able to make it work (and I believe we're upgrading GCC soon, and at that point it should be "automatic" anyway).

/** Delete an instance of SdVolume */
~SdVolume() { free(cacheBuffer_); cacheBuffer_ = NULL; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

The original cache was a static array, accessible from any instance. This change makes it a static pointer, and the constructor checks if it has been allocated before, in which case nothing is done, or not, in which case it is allocated.
This destructor just blindly frees the mem. If there is more than one instance, the others will then try to access after free.
I think it's unlikely that there will be more than one instance of this class, but I think this is something that should be fixed to cover e.g.: temps (pass by value) and copy.
I suggest use of a shared_ptr.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had only considered the single global SD object. Since then, with the BSSL stuff I've been introduced to the wonders of the std::shared_ptr and this would be a good use of one like you suggest. Will see if I can fix it up this weekend.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, but using a separate refcnt instead of std::ptr as there needs to be a special-case for cnt==1 (like the BearSSL issue).

/** Clear the cache and returns a pointer to the cache. Used by the WaveRP
* recorder to do raw write to the SD card. Not for normal apps.
*/
static uint8_t* cacheClear(void) {
cacheFlush();
cacheBlockNumber_ = 0XFFFFFFFF;
return cacheBuffer_.data;
return cacheBuffer_->data;
}
/**
* Initialize a FAT volume. Try partition one first then try super
Expand Down Expand Up @@ -499,7 +501,7 @@ class SdVolume {
// value for action argument in cacheRawBlock to indicate cache dirty
static uint8_t const CACHE_FOR_WRITE = 1;

static cache_t cacheBuffer_; // 512 byte cache for device blocks
static cache_t *cacheBuffer_; // 512 byte cache for device blocks
static uint32_t cacheBlockNumber_; // Logical number of block in the cache
static Sd2Card* sdCard_; // Sd2Card object for cache
static uint8_t cacheDirty_; // cacheFlush() will write block if true
Expand Down
16 changes: 8 additions & 8 deletions libraries/SD/src/utility/SdFile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ uint8_t SdFile::addDirCluster(void) {
// return pointer to cached entry or null for failure
dir_t* SdFile::cacheDirEntry(uint8_t action) {
if (!SdVolume::cacheRawBlock(dirBlock_, action)) return NULL;
return SdVolume::cacheBuffer_.dir + dirIndex_;
return SdVolume::cacheBuffer_->dir + dirIndex_;
}
//------------------------------------------------------------------------------
/**
Expand Down Expand Up @@ -323,7 +323,7 @@ uint8_t SdFile::makeDir(SdFile* dir, const char* dirName) {
if (!SdVolume::cacheRawBlock(block, SdVolume::CACHE_FOR_WRITE)) return false;

// copy '.' to block
memcpy(&SdVolume::cacheBuffer_.dir[0], &d, sizeof(d));
memcpy(&SdVolume::cacheBuffer_->dir[0], &d, sizeof(d));

// make entry for '..'
d.name[1] = '.';
Expand All @@ -335,7 +335,7 @@ uint8_t SdFile::makeDir(SdFile* dir, const char* dirName) {
d.firstClusterHigh = dir->firstCluster_ >> 16;
}
// copy '..' to block
memcpy(&SdVolume::cacheBuffer_.dir[1], &d, sizeof(d));
memcpy(&SdVolume::cacheBuffer_->dir[1], &d, sizeof(d));

// set position after '..'
curPosition_ = 2 * sizeof(d);
Expand Down Expand Up @@ -442,7 +442,7 @@ uint8_t SdFile::open(SdFile* dirFile, const char* fileName, uint8_t oflag) {

// use first entry in cluster
dirIndex_ = 0;
p = SdVolume::cacheBuffer_.dir;
p = SdVolume::cacheBuffer_->dir;
}
// initialize as empty file
memset(p, 0, sizeof(dir_t));
Expand Down Expand Up @@ -510,7 +510,7 @@ uint8_t SdFile::open(SdFile* dirFile, uint16_t index, uint8_t oflag) {
// open a cached directory entry. Assumes vol_ is initializes
uint8_t SdFile::openCachedEntry(uint8_t dirIndex, uint8_t oflag) {
// location of entry in cache
dir_t* p = SdVolume::cacheBuffer_.dir + dirIndex;
dir_t* p = SdVolume::cacheBuffer_->dir + dirIndex;

// write or truncate is an error for a directory or read-only file
if (p->attributes & (DIR_ATT_READ_ONLY | DIR_ATT_DIRECTORY)) {
Expand Down Expand Up @@ -709,7 +709,7 @@ int16_t SdFile::read(void* buf, uint16_t nbyte) {
} else {
// read block to cache and copy data to caller
if (!SdVolume::cacheRawBlock(block, SdVolume::CACHE_FOR_READ)) return -1;
uint8_t* src = SdVolume::cacheBuffer_.data + offset;
uint8_t* src = SdVolume::cacheBuffer_->data + offset;
uint8_t* end = src + n;
while (src != end) *dst++ = *src++;
}
Expand Down Expand Up @@ -763,7 +763,7 @@ dir_t* SdFile::readDirCache(void) {
curPosition_ += 31;

// return pointer to entry
return (SdVolume::cacheBuffer_.dir + i);
return (SdVolume::cacheBuffer_->dir + i);
}
//------------------------------------------------------------------------------
/**
Expand Down Expand Up @@ -1196,7 +1196,7 @@ size_t SdFile::write(const void* buf, uint16_t nbyte) {
goto writeErrorReturn;
}
}
uint8_t* dst = SdVolume::cacheBuffer_.data + blockOffset;
uint8_t* dst = SdVolume::cacheBuffer_->data + blockOffset;
uint8_t* end = dst + n;
while (dst != end) *dst++ = *src++;
}
Expand Down
24 changes: 12 additions & 12 deletions libraries/SD/src/utility/SdVolume.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
// raw block cache
// init cacheBlockNumber_to invalid SD block number
uint32_t SdVolume::cacheBlockNumber_ = 0XFFFFFFFF;
cache_t SdVolume::cacheBuffer_; // 512 byte cache for Sd2Card
cache_t* SdVolume::cacheBuffer_; // 512 byte cache for Sd2Card
Sd2Card* SdVolume::sdCard_; // pointer to SD card object
uint8_t SdVolume::cacheDirty_ = 0; // cacheFlush() will write block if true
uint32_t SdVolume::cacheMirrorBlock_ = 0; // mirror block for second FAT
Expand Down Expand Up @@ -98,12 +98,12 @@ uint8_t SdVolume::allocContiguous(uint32_t count, uint32_t* curCluster) {
//------------------------------------------------------------------------------
uint8_t SdVolume::cacheFlush(void) {
if (cacheDirty_) {
if (!sdCard_->writeBlock(cacheBlockNumber_, cacheBuffer_.data)) {
if (!sdCard_->writeBlock(cacheBlockNumber_, cacheBuffer_->data)) {
return false;
}
// mirror FAT tables
if (cacheMirrorBlock_) {
if (!sdCard_->writeBlock(cacheMirrorBlock_, cacheBuffer_.data)) {
if (!sdCard_->writeBlock(cacheMirrorBlock_, cacheBuffer_->data)) {
return false;
}
cacheMirrorBlock_ = 0;
Expand All @@ -116,7 +116,7 @@ uint8_t SdVolume::cacheFlush(void) {
uint8_t SdVolume::cacheRawBlock(uint32_t blockNumber, uint8_t action) {
if (cacheBlockNumber_ != blockNumber) {
if (!cacheFlush()) return false;
if (!sdCard_->readBlock(blockNumber, cacheBuffer_.data)) return false;
if (!sdCard_->readBlock(blockNumber, cacheBuffer_->data)) return false;
cacheBlockNumber_ = blockNumber;
}
cacheDirty_ |= action;
Expand All @@ -127,9 +127,9 @@ uint8_t SdVolume::cacheRawBlock(uint32_t blockNumber, uint8_t action) {
uint8_t SdVolume::cacheZeroBlock(uint32_t blockNumber) {
if (!cacheFlush()) return false;

// loop take less flash than memset(cacheBuffer_.data, 0, 512);
// loop take less flash than memset(cacheBuffer_->data, 0, 512);
for (uint16_t i = 0; i < 512; i++) {
cacheBuffer_.data[i] = 0;
cacheBuffer_->data[i] = 0;
}
cacheBlockNumber_ = blockNumber;
cacheSetDirty();
Expand All @@ -156,9 +156,9 @@ uint8_t SdVolume::fatGet(uint32_t cluster, uint32_t* value) const {
if (!cacheRawBlock(lba, CACHE_FOR_READ)) return false;
}
if (fatType_ == 16) {
*value = cacheBuffer_.fat16[cluster & 0XFF];
*value = cacheBuffer_->fat16[cluster & 0XFF];
} else {
*value = cacheBuffer_.fat32[cluster & 0X7F] & FAT32MASK;
*value = cacheBuffer_->fat32[cluster & 0X7F] & FAT32MASK;
}
return true;
}
Expand All @@ -180,9 +180,9 @@ uint8_t SdVolume::fatPut(uint32_t cluster, uint32_t value) {
}
// store entry
if (fatType_ == 16) {
cacheBuffer_.fat16[cluster & 0XFF] = value;
cacheBuffer_->fat16[cluster & 0XFF] = value;
} else {
cacheBuffer_.fat32[cluster & 0X7F] = value;
cacheBuffer_->fat32[cluster & 0X7F] = value;
}
cacheSetDirty();

Expand Down Expand Up @@ -232,7 +232,7 @@ uint8_t SdVolume::init(Sd2Card* dev, uint8_t part) {
if (part) {
if (part > 4)return false;
if (!cacheRawBlock(volumeStartBlock, CACHE_FOR_READ)) return false;
part_t* p = &cacheBuffer_.mbr.part[part-1];
part_t* p = &cacheBuffer_->mbr.part[part-1];
if ((p->boot & 0X7F) !=0 ||
p->totalSectors < 100 ||
p->firstSector == 0) {
Expand All @@ -242,7 +242,7 @@ uint8_t SdVolume::init(Sd2Card* dev, uint8_t part) {
volumeStartBlock = p->firstSector;
}
if (!cacheRawBlock(volumeStartBlock, CACHE_FOR_READ)) return false;
bpb_t* bpb = &cacheBuffer_.fbs.bpb;
bpb_t* bpb = &cacheBuffer_->fbs.bpb;
if (bpb->bytesPerSector != 512 ||
bpb->fatCount == 0 ||
bpb->reservedSectorCount == 0 ||
Expand Down