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

bd: Add get_erase_value function to the block device API #5925

Merged
merged 3 commits into from Jan 31, 2018

Conversation

Projects
None yet
7 participants
@geky
Member

geky commented Jan 24, 2018

This allows a user to determine the state of storage after an erase and take advantage of the NANDing nature of flash in the filesystem, NVStore, and FTL layers.

class BlockDevice {
    /** Get the value of storage when erased
     *
     *  If get_erase_value returns a non-negative byte value, the underlying
     *  storage will be set to that value when erased, and storage containing
     *  that value can be programmed without another erase.
     *
     *  @return         The value of storage when erased, or -1 if the value of
     *                  erased storage can't be relied on
     */
    virtual int get_erase_value() const;
}

should resolve #4023
cc @deepikabhavnani, @cmonr, @davidsaada

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Jan 25, 2018

Does -ve value indicate not supported by block device? Erase value or state can be either 0/1 is that correct? Can we mention what will be valid return values from this function?

@geky

This comment has been minimized.

Member

geky commented Jan 25, 2018

Good point, I'll add examples. I'm expecting 0x00, 0xff, any byte value between those, or negative for state between erase is undefined.

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Jan 26, 2018

Two comments:

  1. This is indeed a required functionality. I'd only pick a different name for the function. Looking at this name may lead one to think of another functionality - like the one discussed earlier with you, returning the blank value for this device (0xFF for flash, 0 for Heap etc.). get_erase_state or something like this should be more intuitive IMO.
  2. Would it be safe to assume that the resolution of this API is in program units? If so, I'd add a word in the explanation, so that users would check the desired blocks more efficiently (as in check addresses per each program unit instead of each byte).
@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Jan 26, 2018

Looking further in the implementation and based on @davidsaada queries, I have questions:

  1. get_erase_value() can be called for each block? Shouldn't we pass block number as argument then?
    (Note: I thought it is related to underlying block device and will be called to know the behavior of device)
  2. get_erase_value() API will be directly linked to erase, user calling erase function followed by get_erase_value will get the state of last erased block?
@geky

This comment has been minimized.

Member

geky commented Jan 26, 2018

This is indeed a required functionality. I'd only pick a different name for the function. Looking at this name may lead one to think of another functionality - like the one discussed earlier with you, returning the blank value for this device (0xFF for flash, 0 for Heap etc.). get_erase_state or something like this should be more intuitive IMO.

Uh, I think we might be on different pages. Definitely means the documentation should be improved. This was actually supposed to be the idea discussed earlier.

so:

SPIFBlockDevice::get_erase_value() -> 0xff
HeapBlockDevice::get_erase_value() -> 0x00
SDBlockDevice::get_erase_value() -> -1

Calling erase does not change the result of get_erase_value. Just like get_erase_size and friends.


It sounds like I used the word byte and state too much. How about this?

class BlockDevice {
    /** Get the value of storage when erased
     *
     *  If get_erase_value returns a non-negative byte value, the underlying
     *  storage will be set to that value when erased, and storage containing
     *  that value can be programmed without another erase.
     *
     *  @return         The value of storage when erased, or -1 if the value of
     *                  erased storage can't be relied on
     */
    virtual int get_erase_value() const;
}
@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Jan 26, 2018

Oh OK, then all clear now. No problem here.

@cmonr cmonr added needs: work and removed needs: review labels Jan 26, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 26, 2018

Marking as "needs work" for docs.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 29, 2018

@AnotherButler, could you take a look at this?

@cmonr cmonr added needs: CI and removed needs: work labels Jan 29, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 29, 2018

/morph test

geky added some commits Jan 24, 2018

bd: Added get_erase_value function to the block device API
Default implementation returns -1 and is backwards compatible

@geky geky force-pushed the geky:bd-erase-value branch from 3dd626f to 88aad81 Jan 29, 2018

@geky

This comment has been minimized.

Member

geky commented Jan 29, 2018

@cmonr, sorry the documentation wasn't actually updated yet. Is updated now.

@AnotherButler

This comment has been minimized.

Contributor

AnotherButler commented Jan 29, 2018

Query: Should ExhaustibleBlockDevice, ObservingBlockDevice and ReadOnlyBlockDevice be listed in our API references on os.mbed.com/docs? They're not there now, but the others are.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 29, 2018

Query: Should ExhaustibleBlockDevice, ObservingBlockDevice and ReadOnlyBlockDevice be listed in our API references on os.mbed.com/docs? They're not there now, but the others are.

@AnotherButler, I think so, but that page would need to be updated a bit more than simply listing all supported block devices. Some block devices act as interfaces for specific hardware, whereas others serve as helper block devices. Some block devices have their own repos, with their own example code, whereas others exist within mbed-os.

I don't think it should be a part of this PR, but definitely think the page should be updated to list all supported block devices (with examples?).

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 29, 2018

/morph build

@AnotherButler

This comment has been minimized.

Contributor

AnotherButler commented Jan 29, 2018

I guess what I'm asking is should they each get their own page the way HeapBlockDevice does: https://os.mbed.com/docs/v5.7/reference/heapblockdevice.html?

os.mbed.com/docs should only include code on master in mbed-os (perhaps with the exception of porting guide information, which we're in discussions about).

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 29, 2018

@AnotherButler, I think for the block devices already a part of mbed-os, they should have a page of their own.

For the block devices that are not a part of mbed-os, I think they should also have their own docs page, but maybe in order to do that, they need to be merged into mbed-os first.

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 29, 2018

Build : SUCCESS

Build number : 1005
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5925/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci

This comment has been minimized.

@geky

This comment has been minimized.

Member

geky commented Jan 30, 2018

They are listed here:
https://os.mbed.com/docs/v5.7/reference/storage.html#utility-block-devices

But I agree with @cmonr, they should have individual pages.

bd: Copy edit BlockDevices
Copy edit SlicingBlockDevice.h
Copy edit ReadOnlyBlockDevice.h
Copy edit ProfilingBlockDevice.h
Copy edit ObservingBlockDevice.h
Copy edit MBRBlockDevice.h
Copy edit ExhaustibleBlockDevice.h
Copy edit ChainingBlockDevice.h
Copy edit BlockDevice.h

Copy edit files for active voice and consistent tense.

@geky geky force-pushed the geky:bd-erase-value branch from 8726471 to e6949db Jan 30, 2018

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 30, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 30, 2018

Build : SUCCESS

Build number : 1008
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5925/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr cmonr merged commit 5cd30b9 into ARMmbed:master Jan 31, 2018

19 checks passed

ARM mbed CI Verification build successful.
Details
AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Local events testing has passed
Details
travis-ci/littlefs Local littlefs testing has passed
Details
travis-ci/mbed2-ATMEL Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-RENESAS Local mbed2-RENESAS testing has passed
Details
travis-ci/mbed2-SILICON_LABS Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM Local mbed2-STM testing has passed
Details
travis-ci/tools Local tools testing has passed
Details

@cmonr cmonr removed the ready for merge label Jan 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment