bd: Add MBR block device for standard storage partitioning #3936

geky · 2017-03-16T00:40:32Z

The MBRBlockDevice adds support for standard storage partitioning. This is the standard partitioning scheme everyone knows and loves (and is probably using underneath their OS). For more information:
https://en.wikipedia.org/wiki/Master_boot_record

Here's the example:

#include "mbed.h"
#include "HeapBlockDevice.h"
#include "MBRBlockDevice.h"

// Create a block device with 64 blocks of size 512
HeapBlockDevice mem(64*512, 512);

// Partition into two partitions with ~half the blocks
MBRBlockDevice::partition(&mem, 1, 0x00, 0*512, 32*512);
MBRBlockDevice::partition(&mem, 2, 0x00, 32*512);

// Create a block device that maps to the first 32 blocks (excluding MBR block)
MBRBlockDevice part1(&mem, 1);

// Create a block device that maps to the last 32 blocks
MBRBlockDevice part2(&mem, 2);

Also comes in with tests and doxygen.

cc @simonqhughes

0xc0170 · 2017-03-20T15:58:01Z

features/filesystem/bd/MBRBlockDevice.cpp

+    _size   = fromle32(table->entries[_part-1].lba_size)   * sector;
+
+    // Check that block addresses are valid
+    printf("offset %lld\n", _offset);


Is this intended to be printed always (not a debug msg) ? or just left behind?

Ah whoops, good catch 👍

0xc0170 · 2017-03-20T15:58:28Z

@simonqhughes please review

0xc0170 · 2017-03-20T16:01:29Z

features/TESTS/filesystem/mbr_block_device/main.cpp

+HeapBlockDevice bd(BLOCK_COUNT*BLOCK_SIZE, BLOCK_SIZE);
+
+// Testing formatting of master boot record
+void test_mbr_format() {


code style same as the implementation

void test_mbr_format() { //code here }

Ah good catch 👍

sg- · 2017-03-22T11:42:00Z

I kinda get this but does the FATFileSystem mount a MBRBlockDevice?

geky · 2017-03-22T16:41:56Z

You can, it depends on how you structure your classes:

SDBlockDevice sd(s0, s1, s2, s3); // SD card
MBRBlockDevice part1(&sd, 1); // Partition 1 on SD card
FATFileSystem fs("fat", &part1); // FAT filesystem on partition 1

Should be noted the MBRBlockDevice is optional. External storage doesn't need an mbr for pcs, and chanfs already traverses an mbr if it runs into one.

The MBRBlockDevice is useful if you want to access other partitions on the device while still allowing mountability across other devices. It also provides a few other uses, such as defining storage regions at manufacturing time, or storing partition attributes.

adbridge · 2017-03-30T15:50:47Z

@sg- are you happy with @geky comments ?

sg- · 2017-04-10T16:14:12Z

@geky LGTM. Just need an API page for this and stage some updates to storage docs in the handbook that show how to use it.

theotherjimmy · 2017-04-24T15:21:33Z

@geky any news on the docs update?

geky · 2017-05-12T21:22:31Z

I added a blurb in the API page for the block device API that links to the documentation in the MBRBlockDevice.h header:
ARMmbed/mbed_OS_API_Docs#80

I'm not sure the MBR block device needs it's own handbook page as it's just one part of the storage apis, but let me know if you want more documentation. (The doxygen in the class contains example usage).

geky · 2017-05-12T21:43:40Z

I also added a bit more to the class documentation and included the SD card example.

0xc0170 · 2017-05-15T12:31:28Z

features/filesystem/bd/MBRBlockDevice.cpp

+
+// Little-endian conversion, should compile to noop
+// if system is little-endian
+static inline uint32_t tole32(uint32_t a) {


can you align the code style

Good catch, should be updated

0xc0170 · 2017-05-15T12:37:37Z

features/TESTS/filesystem/mbr_block_device/main.cpp

+    TEST_ASSERT_EQUAL(0, err);
+
+    // Test attributes on partitions
+    printf("partition 1 partition number: %d\n",        part1.get_partition_number());


Do we need all these prints? if they fail, these numbers will be printed as part of the error?

The output would help debugging what went wrong in the test. Multiple attributes may be wrong, but the assert would stop after the first.

Understood, but still the test should not print anything if is all green ? This is more for future request (than solving it here with this PR) - having logging library. Some tests already have something like DEBUG_PRINT that is a config attribute and by default disabled. Is it time now to consider having something similar to it - a proper replacement?

I rambled grumpily over here: #4317 (comment), there's also related discussion over there.

It does seem like we should have a configurable logging/debug we can remove at compile time, but I'm going to leave these as is for now. They aren't currently causing a problem, and we should probably apply a consistent solution to all of the tests we have in a seperate pr.

adbridge · 2017-05-17T13:50:09Z

@0xc0170 are you happy with @geky responses ?

0xc0170 · 2017-05-17T13:55:44Z

@0xc0170 are you happy with @geky responses ?

Yes, LGTM

geky · 2017-05-18T17:00:36Z

/morph test-nightly

mbed-bot · 2017-05-19T05:24:30Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 269

Test failed!

0xc0170 · 2017-05-19T09:44:55Z

features-tests-filesystem-mbr_block_device failures in the last CI build, please look at them

adbridge · 2017-05-23T15:52:04Z

@geky Any update on this ?

geky · 2017-05-24T20:24:15Z

/morph test-nightly

mbed-bot · 2017-05-25T01:06:25Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 336

All builds and test passed!

This saves 64 blocks (32KB when used with 512B blocks) and drops the minimum storage size from 64KB to 32KB.

geky · 2017-05-25T19:29:29Z

Found an issue with how the fat filesystem calculated the minimum block count required without the MBR (3ee77e3). Sorry about creating the rereview.

0xc0170 · 2017-05-26T11:54:51Z

/morph test-nightly

mbed-bot · 2017-05-26T17:00:30Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 358

Test failed!

geky · 2017-05-26T18:19:20Z

11:05:13 [1495814808.48][CONN][RXD] UDP: tx -> 5
11:05:13 [1495814808.50][CONN][RXD] UDP: rx <- 128
11:05:13 [1495814808.52][CONN][RXD] UDP: rx <- 400
11:05:13 [1495814808.54][CONN][RXD] UDP: rx <- 208
11:05:13 [1495814808.55][CONN][RXD] UDP: rx <- 224
11:06:09 [1495814864.31][HTST][INF] test suite run finished after 60.81 sec...
...
11:06:09 [1495814864.52][HTST][INF] {{result;timeout}}
11:06:09 mbedgt: checking for GCOV data...
11:06:09 mbedgt: mbed-host-test-runner: stopped and returned 'TIMEOUT'

This is a sporadic udp packet loss error fixed by this guy: #4369

sg- · 2017-05-29T13:10:37Z

/morph test-nightly

mbed-bot · 2017-05-29T22:12:52Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 366

Test failed!

0xc0170 · 2017-05-30T08:07:51Z

/morph test-nightly

mbed-bot · 2017-05-30T11:26:30Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 367

Test failed!

0xc0170 · 2017-05-30T11:59:30Z

/morph test-nightly

mbed-bot · 2017-05-30T16:11:37Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 369

All builds and test passed!

geky · 2017-05-30T16:20:11Z

Woah! One got through! LGTM?

0xc0170 requested a review from simonqhughes March 20, 2017 15:50

0xc0170 reviewed Mar 20, 2017

View reviewed changes

0xc0170 added the needs: review label Mar 20, 2017

0xc0170 reviewed Mar 20, 2017

View reviewed changes

geky force-pushed the bd-mbr branch 2 times, most recently from 655d485 to ab5a2b9 Compare March 21, 2017 19:48

sg- added scope: feature needs: work and removed needs: review labels Apr 10, 2017

geky mentioned this pull request May 12, 2017

Add blurb about the MBR and other utility block devices ARMmbed/mbed_OS_API_Docs#80

Merged

geky force-pushed the bd-mbr branch from ab5a2b9 to 68a5dee Compare May 12, 2017 21:43

0xc0170 reviewed May 15, 2017

View reviewed changes

geky force-pushed the bd-mbr branch from 68a5dee to d9d6ac7 Compare May 16, 2017 15:37

sg- added needs: CI and removed needs: work labels May 18, 2017

adbridge added the needs: work label May 23, 2017

sg- approved these changes May 25, 2017

View reviewed changes

sg- added the release-version: 5.5.0-rc1 label May 25, 2017

0xc0170 added ready for merge and removed needs: work labels May 25, 2017

FAT: Fixed volume count check when formatted without MBR

3ee77e3

This saves 64 blocks (32KB when used with 512B blocks) and drops the minimum storage size from 64KB to 32KB.

geky requested a review from sg- May 25, 2017 19:26

geky added needs: review and removed ready for merge labels May 25, 2017

geky mentioned this pull request May 25, 2017

I2C Flash BOM Update ARMmbed/ci-test-shield#36

Open

sg- approved these changes May 25, 2017

View reviewed changes

geky mentioned this pull request May 25, 2017

FatFileSystem (unnecessarily) requires a minimum allocation of 64K #4386

Closed

geky added needs: CI and removed needs: review labels May 25, 2017

sg- added ready for merge and removed needs: CI labels May 30, 2017

sg- merged commit f438251 into ARMmbed:master May 30, 2017

sg- removed the ready for merge label May 30, 2017

bd: Add MBR block device for standard storage partitioning #3936

bd: Add MBR block device for standard storage partitioning #3936

Uh oh!

Conversation

geky commented Mar 16, 2017

Uh oh!

0xc0170 Mar 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Mar 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sg- commented Mar 22, 2017

Uh oh!

geky commented Mar 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adbridge commented Mar 30, 2017

Uh oh!

sg- commented Apr 10, 2017

Uh oh!

theotherjimmy commented Apr 24, 2017

Uh oh!

geky commented May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geky commented May 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geky May 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 May 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adbridge commented May 17, 2017

Uh oh!

0xc0170 commented May 17, 2017

Uh oh!

geky commented May 18, 2017

Uh oh!

mbed-bot commented May 19, 2017

Output

Uh oh!

0xc0170 commented May 19, 2017

Uh oh!

adbridge commented May 23, 2017

Uh oh!

geky commented May 24, 2017

Uh oh!

mbed-bot commented May 25, 2017

Output

Uh oh!

geky commented May 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented May 26, 2017

Uh oh!

mbed-bot commented May 26, 2017

Output

Uh oh!

geky commented May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sg- commented May 29, 2017

0xc0170 Mar 20, 2017 •

edited

Loading

geky commented Mar 22, 2017 •

edited

Loading

geky commented May 12, 2017 •

edited

Loading

geky May 15, 2017 •

edited

Loading

0xc0170 May 16, 2017 •

edited

Loading

geky commented May 25, 2017 •

edited

Loading

geky commented May 26, 2017 •

edited

Loading