Skip to content
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

Implement BufferedBlockDevice #6757

Merged
merged 1 commit into from May 22, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
139 changes: 139 additions & 0 deletions features/TESTS/filesystem/buffered_block_device/main.cpp
@@ -0,0 +1,139 @@
/* mbed Microcontroller Library
* Copyright (c) 2018 ARM Limited
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
#include "greentea-client/test_env.h"
#include "unity.h"
#include "utest.h"

#include "BufferedBlockDevice.h"
#include "HeapBlockDevice.h"
#include <stdlib.h>

using namespace utest::v1;

static const bd_size_t heap_erase_size = 512;
static const bd_size_t heap_prog_size = heap_erase_size;
static const bd_size_t heap_read_size = 256;
static const bd_size_t num_blocks = 4;

void functionality_test()
{
HeapBlockDevice heap_bd(num_blocks * heap_erase_size, heap_read_size, heap_prog_size, heap_erase_size);
BufferedBlockDevice bd(&heap_bd);

int err = bd.init();
TEST_ASSERT_EQUAL(0, err);

uint8_t *read_buf, *write_buf;
read_buf = new uint8_t[heap_prog_size];
write_buf = new uint8_t[heap_prog_size];

TEST_ASSERT_EQUAL(1, bd.get_read_size());
TEST_ASSERT_EQUAL(1, bd.get_program_size());
TEST_ASSERT_EQUAL(heap_erase_size, bd.get_erase_size());

for (bd_size_t i = 0; i < num_blocks; i++) {
memset(write_buf, i, heap_prog_size);
err = heap_bd.program(write_buf, i * heap_prog_size, heap_prog_size);
TEST_ASSERT_EQUAL(0, err);
}

err = bd.read(read_buf, heap_prog_size + heap_prog_size / 2, 1);
TEST_ASSERT_EQUAL(0, err);
TEST_ASSERT_EQUAL(1, read_buf[0]);

err = bd.read(read_buf, 2 * heap_prog_size + heap_prog_size / 2, 4);
TEST_ASSERT_EQUAL(0, err);
memset(write_buf, 2, 4);
TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, 4);

memset(write_buf, 1, heap_prog_size);
memset(write_buf + 64, 0x5A, 8);
memset(write_buf + 72, 0xA5, 8);
err = bd.program(write_buf + 64, heap_prog_size + 64, 8);
TEST_ASSERT_EQUAL(0, err);
err = bd.program(write_buf + 72, heap_prog_size + 72, 8);
TEST_ASSERT_EQUAL(0, err);
err = bd.read(read_buf, heap_prog_size, heap_prog_size);
TEST_ASSERT_EQUAL(0, err);
TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, heap_prog_size);
memset(write_buf, 1, heap_prog_size);
// Underlying BD should not be updated before sync
err = heap_bd.read(read_buf, heap_prog_size, heap_prog_size);
TEST_ASSERT_EQUAL(0, err);
TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, heap_prog_size);
err = bd.sync();
TEST_ASSERT_EQUAL(0, err);
memset(write_buf + 64, 0x5A, 8);
memset(write_buf + 72, 0xA5, 8);
// Should be updated now
err = bd.read(read_buf, heap_prog_size, heap_prog_size);
TEST_ASSERT_EQUAL(0, err);
TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, heap_prog_size);
err = heap_bd.read(read_buf, heap_prog_size, heap_prog_size);
TEST_ASSERT_EQUAL(0, err);
TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, heap_prog_size);

memset(write_buf, 0xAA, 16);
memset(write_buf + 16, 0xBB, 16);
err = bd.program(write_buf, 3 * heap_prog_size - 16, 32);
TEST_ASSERT_EQUAL(0, err);
// First block should sync, but second still shouldn't
memset(write_buf, 2, heap_prog_size - 16);
memset(write_buf + heap_prog_size - 16, 0xAA, 16);
err = heap_bd.read(read_buf, 2 * heap_prog_size, heap_prog_size);
TEST_ASSERT_EQUAL(0, err);
TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, heap_prog_size);
memset(write_buf, 3, heap_prog_size);
err = heap_bd.read(read_buf, 3 * heap_prog_size, heap_prog_size);
TEST_ASSERT_EQUAL(0, err);
TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, heap_prog_size);
memset(write_buf, 0xBB, 16);
err = bd.read(read_buf, 3 * heap_prog_size, heap_prog_size);
TEST_ASSERT_EQUAL(0, err);
TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, heap_prog_size);
// Moving to another block should automatically sync
err = bd.read(read_buf, 15, 1);
TEST_ASSERT_EQUAL(0, err);
TEST_ASSERT_EQUAL(0, read_buf[0]);
err = heap_bd.read(read_buf, 3 * heap_prog_size, heap_prog_size);
TEST_ASSERT_EQUAL(0, err);
TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, heap_prog_size);
err = bd.read(read_buf, 3 * heap_prog_size, heap_prog_size);
TEST_ASSERT_EQUAL(0, err);
TEST_ASSERT_EQUAL_UINT8_ARRAY(write_buf, read_buf, heap_prog_size);

delete[] read_buf;
delete[] write_buf;
}

// Test setup
utest::v1::status_t test_setup(const size_t number_of_cases)
{
GREENTEA_SETUP(30, "default_auto");
return verbose_test_setup_handler(number_of_cases);
}

Case cases[] = {
Case("BufferedBlockDevice functionality test", functionality_test),
};

Specification specification(test_setup, cases);

int main()
{
return !Harness::run(specification);
}

228 changes: 228 additions & 0 deletions features/filesystem/bd/BufferedBlockDevice.cpp
@@ -0,0 +1,228 @@
/* mbed Microcontroller Library
* Copyright (c) 2018 ARM Limited
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#include "BufferedBlockDevice.h"
#include "mbed_assert.h"
#include <algorithm>
#include <string.h>

static inline uint32_t align_down(bd_size_t val, bd_size_t size)
{
return val / size * size;
Copy link
Contributor

Choose a reason for hiding this comment

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

In the future, this guy should definitely have parenthesis

}

BufferedBlockDevice::BufferedBlockDevice(BlockDevice *bd)
: _bd(bd), _bd_program_size(0), _curr_aligned_addr(0), _flushed(true), _cache(0)
{
}

BufferedBlockDevice::~BufferedBlockDevice()
{
deinit();
}

int BufferedBlockDevice::init()
{
int err = _bd->init();
if (err) {
return err;
}

_bd_program_size = _bd->get_program_size();

if (!_cache) {
_cache = new uint8_t[_bd_program_size];
Copy link
Contributor

Choose a reason for hiding this comment

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

If this alloc is being deferred to the error-returning init, does it make sense to use (nothrow) and return an error on memory failure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reason for this being allocated in init is the fact that it relies on the underlying BD's get_program_size, whose value may only be valid after init. Code originally included an assertion for allocation failure, but then was told by @geky that it was redundant, as mbed-os automatically asserts on allocation failures. Is this not correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's correct - it just seems slightly odd to throw given that all other errors in the function return an error code.

On the other hand, it's not that you really want dynamic allocation with check - if it weren't for that get_program_size you'd be doing a throwing new in the constructor, right? So maybe it's just that, deferred.

I'm generally in favour of throwing for unexpected errors, rather than returning an error no-one checks. So fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I try to distinguish "fatal" errors (like not being able to allocate at init) from "non fatal" ones (errors in read/write for wrong addresses etc.). At least my code (like StorageLite) checks for error returned by the latter. Maybe I should return the assert for allocation failure just for visibility sake.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine - I'll leave it up to you.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the question is "would an application want to recover from an OOM in the buffered block device?"

}

_curr_aligned_addr = _bd->size();
_flushed = true;

return 0;
}

int BufferedBlockDevice::deinit()
{
delete[] _cache;
_cache = 0;
return _bd->deinit();
}

int BufferedBlockDevice::flush()
{
if (!_flushed) {
int ret = _bd->program(_cache, _curr_aligned_addr, _bd_program_size);
if (ret) {
return ret;
}
_flushed = true;
}
return 0;
}

int BufferedBlockDevice::sync()
{
int ret = flush();
if (ret) {
return ret;
}
return _bd->sync();
}

int BufferedBlockDevice::read(void *b, bd_addr_t addr, bd_size_t size)
{
MBED_ASSERT(_cache);
bool moved_unit = false;

bd_addr_t aligned_addr = align_down(addr, _bd_program_size);

uint8_t *buf = static_cast<uint8_t *> (b);

if (aligned_addr != _curr_aligned_addr) {
// Need to flush if moved to another program unit
flush();
_curr_aligned_addr = aligned_addr;
moved_unit = true;
}

while (size) {
_curr_aligned_addr = align_down(addr, _bd_program_size);
if (moved_unit) {
int ret = _bd->read(_cache, _curr_aligned_addr, _bd_program_size);
if (ret) {
return ret;
}
}
bd_addr_t offs_in_buf = addr - _curr_aligned_addr;
bd_size_t chunk = std::min(_bd_program_size - offs_in_buf, size);
memcpy(buf, _cache + offs_in_buf, chunk);
moved_unit = true;
buf += chunk;
addr += chunk;
size -= chunk;
}

return 0;
}

int BufferedBlockDevice::program(const void *b, bd_addr_t addr, bd_size_t size)
{
MBED_ASSERT(_cache);
int ret;
bool moved_unit = false;

bd_addr_t aligned_addr = align_down(addr, _bd_program_size);

const uint8_t *buf = static_cast <const uint8_t *> (b);

// Need to flush if moved to another program unit
if (aligned_addr != _curr_aligned_addr) {
flush();
_curr_aligned_addr = aligned_addr;
moved_unit = true;
}

while (size) {
_curr_aligned_addr = align_down(addr, _bd_program_size);
bd_addr_t offs_in_buf = addr - _curr_aligned_addr;
bd_size_t chunk = std::min(_bd_program_size - offs_in_buf, size);
const uint8_t *prog_buf;
if (chunk < _bd_program_size) {
// If moved a unit, and program doesn't cover entire unit, it means we don't have the entire
// program unit cached - need to complete it from underlying BD
if (moved_unit) {
ret = _bd->read(_cache, _curr_aligned_addr, _bd_program_size);
if (ret) {
return ret;
}
}
memcpy(_cache + offs_in_buf, buf, chunk);
prog_buf = _cache;
} else {
// No need to copy data to our cache on each iteration. Just make sure it's updated
// on the last iteration, when size is not greater than program size (can't be smaller, as
// this is covered in the previous condition).
prog_buf = buf;
if (size == _bd_program_size) {
memcpy(_cache, buf, _bd_program_size);
}
}

// Don't flush on the last iteration, just on all preceding ones.
if (size > chunk) {
ret = _bd->program(prog_buf, _curr_aligned_addr, _bd_program_size);
if (ret) {
return ret;
}
_bd->sync();
} else {
_flushed = false;
}

moved_unit = true;
buf += chunk;
addr += chunk;
size -= chunk;
}

return 0;
}

int BufferedBlockDevice::erase(bd_addr_t addr, bd_size_t size)
{
MBED_ASSERT(is_valid_erase(addr, size));
return _bd->erase(addr, size);
}

int BufferedBlockDevice::trim(bd_addr_t addr, bd_size_t size)
{
MBED_ASSERT(is_valid_erase(addr, size));

if ((_curr_aligned_addr >= addr) && (_curr_aligned_addr <= addr + size)) {
_flushed = true;
_curr_aligned_addr = _bd->size();
}
return _bd->trim(addr, size);
}

bd_size_t BufferedBlockDevice::get_read_size() const
{
return 1;
}

bd_size_t BufferedBlockDevice::get_program_size() const
{
return 1;
}

bd_size_t BufferedBlockDevice::get_erase_size() const
{
return _bd->get_erase_size();
}

bd_size_t BufferedBlockDevice::get_erase_size(bd_addr_t addr) const
{
return _bd->get_erase_size(addr);
}

int BufferedBlockDevice::get_erase_value() const
{
return _bd->get_erase_value();
}

bd_size_t BufferedBlockDevice::size() const
{
return _bd->size();
}