[driver] Block device #328

rleh · 2018-01-12T04:18:04Z

This PR adds a block device interface to xpcc, inspired by mbeds block device interface.

Drivers to store data in a SPI SST flash xpcc::BdSpiFlash, to store data in a file (on hosted) xpcc::BdFile and to store data in RAM (for testing purposes) xpcc::BdHeap are included.
Additionally xpcc::BdMirror is a virtual block device that behaves like RAID1 devices.

For every driver a test is included.

The flash chip driver has been tested on a Nucleo-F429ZI board.

salkinium

Very nice interface and implementation. The drivers need to inherit the interface though for doxygen to understand this relationship.

salkinium · 2018-01-12T17:10:54Z

examples/linux/block_device/mirror/main.cpp

+}
+
+struct FilenameA {
+	static constexpr const char* name = "testA.bin";


Did you mean to commit the examples/linux/block_device/mirror/testA.bin and testB.bin files below?

Yes, because otherwise the test*.bin files have to be created manually by the user (or CI) before running the example.
The is no nice way to create the files from C++.

Idea: Create the file in the SConstruct:

# path to the xpcc root directory xpccpath = '../../../..' # create empty `test.bin~` file (if it does not exist) open('test.bin~', 'a').close() # execute the common SConstruct file exec(compile(open(xpccpath + '/scons/SConstruct', "rb").read(), xpccpath + '/scons/SConstruct', 'exec'))

(Filename changed to test.bin~ so that it is ignored by gitignore.)

(Edit: filename test.bin~)

salkinium · 2018-01-12T17:16:08Z

src/xpcc/architecture/interface/block_device.hpp

+ * @author	Raphael Lehmann
+ * @ingroup	storage
+ */
+#ifdef __DOXYGEN__


Shouldn't this be inside the class? And the drivers then inherit from this class.

salkinium · 2018-01-12T17:18:45Z

src/xpcc/architecture/interface/block_device.hpp

+	 *  @return			True on success
+	 */
+	xpcc::ResumableResult<bool>
+	read(uint8_t *buffer, uint32_t address, size_t size);


Maybe add a using address_t = uint32_t; to use here and then specialize this for implementations.

salkinium · 2018-01-12T17:22:03Z

src/xpcc/driver/storage/block_device_heap.hpp

+/**
+ * \brief	Block device using heap (RAM)
+ *
+ * Usually not persistant, but at least useful for testing


This is useful for battery backed SRAM, some stm32 have up to 4kB of this stuff. Very nice!

For this use-case BdHeap is now extended to optionally use externally supplied memory instead of allocation memory. See here.

salkinium · 2018-01-12T17:29:22Z

src/xpcc/driver/storage/block_device_file.hpp

+#ifndef XPCC_BLOCK_DEVICE_FILE_HPP
+#define XPCC_BLOCK_DEVICE_FILE_HPP
+
+#include <xpcc/processing/resumable.hpp>


This and all other drivers should include <xpcc/architecture/interface/block_device.hpp> and inherit from it.

salkinium · 2018-01-12T17:40:10Z

src/xpcc/driver/storage/block_device_spiflash_impl.hpp

+#ifndef XPCC_BLOCK_DEVICE_SPIFLASH_HPP
+#error	"Don't include this file directly, use 'block_device_spiflash.hpp' instead!"
+#endif
+#include "block_device_spiflash.hpp"


salkinium · 2018-01-12T17:40:24Z

src/xpcc/driver/storage/block_device_mirror_impl.hpp

+#ifndef XPCC_BLOCK_DEVICE_MIRROR_HPP
+	#error	"Don't include this file directly, use 'block_device_mirror.hpp' instead!"
+#endif
+#include "block_device_mirror.hpp"


Why? This includes don't hurt and make QtCreators syntax highlighting and auto-completion work.

Yeah, ok, that's a good enough reason.

salkinium · 2018-01-12T17:40:38Z

src/xpcc/driver/storage/block_device_file_impl.hpp

+#ifndef XPCC_BLOCK_DEVICE_FILE_HPP
+	#error	"Don't include this file directly, use 'block_device_file.hpp' instead!"
+#endif
+#include "block_device_file.hpp"


salkinium · 2018-01-12T17:49:02Z

src/xpcc/driver/storage/block_device_heap.hpp

+
+	/** Writes data to one or more blocks after erasing them
+	*
+	*  The blocks are erased proir to being programmed


salkinium · 2018-01-12T17:49:21Z

src/xpcc/driver/storage/block_device_mirror.hpp

+
+	/** Writes data to one or more blocks after erasing them
+	*
+	*  The blocks are erased proir to being programmed


salkinium · 2018-01-12T17:58:55Z

Oh, and Travis is broken, just ignore it.

salkinium

Nice!

chris-durand · 2018-01-12T20:35:19Z

src/xpcc/driver/storage/block_device_spiflash.hpp

+ * \ingroup	driver_storage
+ * \author	Raphael Lehmann
+ */
+template <typename Spi, typename Cs, size_t flashSize>


On AVR size_t is a 16 bit unsigned int. This wouldn't allow you to use a flash larger than 65535 bytes. You could take a plain uint32_t or something like using flash_size_t = ... and define it to uint32_t on microcontrollers and size_t on hosted to also support 4GB+ files. The same applies for the size_t function parameters.

This wouldn't allow you to use a flash larger than 65535 bytes.

No. You are just unable to write, read or delete >64k memory at once. address is a uint32_t, so you can write/read to/from a <=4GBit flash chip in <=64kBit blocks.

For write and read operations you would need a >64k buffer in your AVRs RAM, but erasing a complete 64MBit flash chip at once on AVR is more realistic. Hmmm...

Sorry, I missed that this size_t is the template parameter and not an read/write/erase function argument.

I'll change the flashSize type to uint32_t here.

chris-durand · 2018-01-12T20:37:47Z

examples/linux/block_device/mirror/main.cpp

+#undef	XPCC_LOG_LEVEL
+#define	XPCC_LOG_LEVEL xpcc::log::INFO
+
+void printMemoryContent(uint8_t* address, std::size_t size) {


const uint8_t*

chris-durand · 2018-01-12T20:39:28Z

examples/linux/block_device/ram/main.cpp

+#undef	XPCC_LOG_LEVEL
+#define	XPCC_LOG_LEVEL xpcc::log::INFO
+
+void printMemoryContent(uint8_t* address, std::size_t size) {


const uint8_t*

chris-durand · 2018-01-12T20:43:21Z

src/xpcc/architecture/interface/block_device.hpp

+	 * Most persistent storage devices require `program()`,  `erase()`
+	 * and sometimes `read()` operations to be executed on fixed sized blocks.
+	 */
+	static constexpr std::size_t BlockSizeRead = 1;


Is there a reason for mixing std::size_t and plain size_t? It is guaranteed to be the same type anyway (since C++11).

No. I'll change all size_t to std::size_t.

(std::size_t has 385 vs size_t has only 184 occurrences in xpcc.)

chris-durand · 2018-01-12T20:59:59Z

src/xpcc/driver/storage/block_device_mirror.hpp

+	*
+	*  @return	BlockDeviceA
+	*/
+	inline BlockDeviceA getBlockDeviceA() {return blockDeviceA;};


Returning a copy is probably not what you intended. I suppose you meant BlockDeviceA&. Probably being able to copy an spi flash driver object is a bad idea. Maybe deleting the copy constructor and copy assignment operator would prevent bugs like that.

chris-durand · 2018-01-12T21:00:22Z

src/xpcc/driver/storage/block_device_mirror.hpp

+	*
+	*  @return	BlockDeviceB
+	*/
+	inline BlockDeviceB getBlockDeviceB() {return blockDeviceB;};


BlockDeviceB&

salkinium added feature platform drivers labels Jan 12, 2018

salkinium added this to the 2018q1 milestone Jan 12, 2018

salkinium suggested changes Jan 12, 2018

View reviewed changes

rleh force-pushed the feature/block_device branch 2 times, most recently from fa13256 to 106f443 Compare January 12, 2018 19:48

salkinium previously approved these changes Jan 12, 2018

View reviewed changes

chris-durand suggested changes Jan 12, 2018

View reviewed changes

rleh added 9 commits January 13, 2018 02:37

[interface] add block device interface

0cca5a8

[driver] block device: SPI Flash SST

1a7448d

[driver] block device: RAM/Heap (for testing)

92bfb84

[driver] block device: File-based (for hosted)

2784317

[driver] virtual block device: mirrored block device (redundant)

9c4228a

[examples] Nucleo-F429ZI: SPI (SST) flash chip

c0d0c59

[examples] Hosted: File-based block device

758cc40

[examples] Hosted: RAM/Heap block device

1bb00c1

[examples] Hosted: Mirrored (redundant) file block device

e1e8af3

rleh dismissed salkinium’s stale review via e1e8af3 January 13, 2018 02:52

rleh force-pushed the feature/block_device branch from 106f443 to e1e8af3 Compare January 13, 2018 02:52

salkinium approved these changes Jan 14, 2018

View reviewed changes

salkinium merged commit e1e8af3 into roboterclubaachen:develop Jan 14, 2018

rleh deleted the feature/block_device branch January 14, 2018 17:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[driver] Block device #328

[driver] Block device #328

rleh commented Jan 12, 2018

salkinium left a comment

salkinium Jan 12, 2018

rleh Jan 12, 2018 •

edited

Loading

salkinium Jan 12, 2018

salkinium Jan 12, 2018

rleh Jan 14, 2018

salkinium Jan 12, 2018

rleh Jan 14, 2018

salkinium Jan 12, 2018

salkinium Jan 12, 2018

salkinium Jan 12, 2018

rleh Jan 12, 2018

salkinium Jan 12, 2018

salkinium Jan 12, 2018

rleh Jan 12, 2018

salkinium Jan 12, 2018

salkinium Jan 12, 2018

salkinium commented Jan 12, 2018

salkinium left a comment

chris-durand Jan 12, 2018

rleh Jan 12, 2018

rleh Jan 12, 2018

chris-durand Jan 12, 2018

chris-durand Jan 12, 2018

chris-durand Jan 12, 2018

rleh Jan 12, 2018 •

edited

Loading

chris-durand Jan 12, 2018

chris-durand Jan 12, 2018

[driver] Block device #328

[driver] Block device #328

Conversation

rleh commented Jan 12, 2018

salkinium left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rleh Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salkinium commented Jan 12, 2018

salkinium left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rleh Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rleh Jan 12, 2018 •

edited

Loading

rleh Jan 12, 2018 •

edited

Loading