data_logger: burst write support

subsys/data_logger/Kconfig

@@ -45,6 +45,13 @@ config DATA_LOGGER_RAM_BUFFER
 	  blocks. When enabled, must be configured for individual loggers with the
 	  `extra-ram-buffer` devicetree property.
 
+config DATA_LOGGER_BURST_WRITES
+	bool
+	depends on DATA_LOGGER_RAM_BUFFER
+	help
+	  When multiple blocks are stored contiguously in RAM buffers, more efficient
+	  write operations may be available.
+
 module = DATA_LOGGER
 module-str = data logger
 source "subsys/logging/Kconfig.template.log_config"

subsys/data_logger/backends/Kconfig

@@ -27,6 +27,7 @@ config DATA_LOGGER_EXFAT
 	select FS_FATFS_EXFAT
 	select FS_FATFS_HAS_RTC
 	select FS_FATFS_EXTRA_NATIVE_API
+	imply DATA_LOGGER_BURST_WRITES
 	default y
 	help
 	  Data is stored on an exFAT filesystem in `.bin` files. Note that the

subsys/data_logger/backends/common.h

@@ -107,6 +107,25 @@ struct data_logger_api {
 	int (*write)(const struct device *dev, uint32_t phy_block, enum infuse_type data_type,
 		     const void *data, uint16_t data_len);
 
+#if defined(CONFIG_DATA_LOGGER_BURST_WRITES) || defined(__DOXYGEN__)
+	/**
+	 * @brief Write multiple blocks to the logger at once
+	 *
+	 * This function enables taking advantage of multiple blocks sitting in a contiguous
+	 * RAM buffer to reduce transaction overhead and therefore increase write throughput.
+	 *
+	 * @param dev Logger device
+	 * @param start_block Physical block index to start data write at
+	 * @param num_blocks Number of blocks of data
+	 * @param data Data for @a num_blocks blocks to write
+	 *
+	 * @retval 0 on success
+	 * @retval -errno otherwise
+	 */
+	int (*write_burst)(const struct device *dev, uint32_t start_block, uint32_t num_blocks,
+			   const void *data);
+#endif /* CONFIG_DATA_LOGGER_BURST_WRITES */
+
 	/**
 	 * @brief Read data from the logger
 	 *

subsys/data_logger/backends/exfat_multi_file.c

@@ -83,11 +83,13 @@ static uint32_t disk_lba_from_block(const struct device *dev, uint32_t phy_block
 static int binary_container_create(const struct device *dev, uint32_t phy_block)
 {
 	const struct dl_exfat_config *config = dev->config;
+	struct dl_exfat_data *data = dev->data;
 	uint32_t file_num = phy_block / BLOCKS_PER_FILE;
 	uint32_t fsize = CONFIG_DATA_LOGGER_EXFAT_FILE_SIZE;
 	uint64_t dev_id = infuse_device_id();
 	uint32_t start_lba;
 	char filename[40];
+	bool del = false;
 	FRESULT res;
 	FIL fp;
 
@@ -102,11 +104,25 @@ static int binary_container_create(const struct device *dev, uint32_t phy_block)
 	}
 	res = f_expand(&fp, fsize, 1);
 	if (res != FR_OK) {
-		LOG_ERR("f_expand failed: %d", res);
+		if (res == FR_DENIED) {
+			LOG_WRN("Disk full at %d/%d blocks", data->common.current_block,
+				data->common.physical_blocks);
+			data->common.logical_blocks = data->common.current_block;
+			data->common.physical_blocks = data->common.current_block;
+			/* Delete the file so init doesn't think data exists on the empty file */
+			del = true;
+		} else {
+			LOG_ERR("f_expand failed: %d", res);
+		}
 		res = -ENOMEM;
 	}
 	(void)f_close(&fp);
 
+	if (del) {
+		LOG_INF("Deleting %s", filename);
+		f_unlink(filename);
+	}
+
 	/* Reset entire file to erased state */
 	if (res == FR_OK) {
 		start_lba = disk_lba_from_block(dev, phy_block);
@@ -115,17 +131,18 @@ static int binary_container_create(const struct device *dev, uint32_t phy_block)
 	return res;
 }
 
-static int logger_exfat_write(const struct device *dev, uint32_t phy_block,
-			      enum infuse_type data_type, const void *mem, uint16_t mem_len)
+static int logger_exfat_write_burst(const struct device *dev, uint32_t start_block,
+				    uint32_t num_blocks, const void *block_data)
 {
 	const struct dl_exfat_config *config = dev->config;
-	uint32_t disk_lba;
+	struct dl_exfat_data *data = dev->data;
+	uint32_t disk_lba, disk_lba_end;
+	uint32_t write_iter;
 	int rc;
 
-	__ASSERT(mem_len == DATA_LOGGER_EXFAT_BLOCK_SIZE, "Not full block");
-
 	(void)logger_exfat_filesystem_claim(dev, NULL, NULL, K_FOREVER);
-	disk_lba = disk_lba_from_block(dev, phy_block);
+restart:
+	disk_lba = disk_lba_from_block(dev, start_block);
 
 	/* No memory left on filesystem */
 	if (disk_lba == LBA_NO_MEM) {
@@ -135,25 +152,47 @@ static int logger_exfat_write(const struct device *dev, uint32_t phy_block,
 	/* File does not exist on filesystem */
 	else if (disk_lba == LBA_NO_FILE) {
 		/* Allocate the binary file on the filesystem */
-		rc = binary_container_create(dev, phy_block);
+		rc = binary_container_create(dev, start_block);
 		if (rc < 0) {
 			goto end;
 		}
-		/* Recalculate the LBA */
-		disk_lba = disk_lba_from_block(dev, phy_block);
+		/* Re-evaluate */
+		goto restart;
 	}
 
-	LOG_DBG("Writing to logger block: %08X LBA: %08X", phy_block, disk_lba);
-	rc = disk_access_write(config->disk, mem, disk_lba, 1);
+	/* Number of empty blocks remaining on the current file */
+	disk_lba_end = data->cached_file_lba + BLOCKS_PER_FILE;
+	/* How many blocks to write this iteration */
+	write_iter = MIN(num_blocks, (disk_lba_end - disk_lba));
+
+	LOG_DBG("Writing to logger block: %08X (%d) LBA: %08X", start_block, write_iter, disk_lba);
+	rc = disk_access_write(config->disk, block_data, disk_lba, write_iter);
 	if (rc == 0) {
 		/* Sync on each write for now */
 		rc = disk_access_ioctl(config->disk, DISK_IOCTL_CTRL_SYNC, NULL);
 	}
+	num_blocks -= write_iter;
+	if (num_blocks) {
+		/* Didn't write the entire block, loop again */
+		LOG_DBG("Looping for remaining %d", num_blocks);
+		start_block += write_iter;
+		block_data = (uint8_t *)block_data + (DATA_LOGGER_EXFAT_BLOCK_SIZE * write_iter);
+		goto restart;
+	}
+
 end:
 	logger_exfat_filesystem_release(dev);
 	return rc;
 }
 
+static int logger_exfat_write(const struct device *dev, uint32_t phy_block,
+			      enum infuse_type data_type, const void *mem, uint16_t mem_len)
+{
+	__ASSERT(mem_len == DATA_LOGGER_EXFAT_BLOCK_SIZE, "Not full block");
+
+	return logger_exfat_write_burst(dev, phy_block, 1, mem);
+}
+
 static int logger_exfat_read(const struct device *dev, uint32_t phy_block, uint16_t block_offset,
 			     void *mem, uint16_t mem_len)
 {
@@ -293,7 +332,11 @@ static int logger_exfat_range_hint(const struct device *dev, uint32_t *block_sta
 		char *fname_idx_ptr = fno.fname + fname_len - 10;
 		int fname_idx = atoi(fname_idx_ptr);
 
-		last_file_idx = MAX(last_file_idx, fname_idx);
+		/* Only consider files that match the expected size */
+		if (fno.fsize == CONFIG_DATA_LOGGER_EXFAT_FILE_SIZE) {
+			last_file_idx = MAX(last_file_idx, fname_idx);
+		}
+
 		/* Next item */
 		fr = f_findnext(&dj, &fno);
 	}
@@ -384,6 +427,9 @@ int logger_exfat_init(const struct device *dev)
 
 const struct data_logger_api data_logger_exfat_api = {
 	.write = logger_exfat_write,
+#ifdef CONFIG_DATA_LOGGER_BURST_WRITES
+	.write_burst = logger_exfat_write_burst,
+#endif /* CONFIG_DATA_LOGGER_BURST_WRITES */
 	.read = logger_exfat_read,
 	.reset = logger_exfat_reset,
 	.search_hint = logger_exfat_range_hint,

subsys/data_logger/backends/exfat_single_file.c

@@ -27,28 +27,35 @@
 
 LOG_MODULE_REGISTER(data_logger_exfat, CONFIG_DATA_LOGGER_EXFAT_LOG_LEVEL);
 
-static int logger_exfat_write(const struct device *dev, uint32_t phy_block,
-			      enum infuse_type data_type, const void *mem, uint16_t mem_len)
+static int logger_exfat_write_burst(const struct device *dev, uint32_t start_block,
+				    uint32_t num_blocks, const void *block_data)
 {
 	const struct dl_exfat_config *config = dev->config;
 	struct dl_exfat_data *data = dev->data;
-	uint32_t disk_lba = data->cached_file_lba + phy_block;
+	uint32_t disk_lba = data->cached_file_lba + start_block;
 	int rc;
 
-	__ASSERT(mem_len == DATA_LOGGER_EXFAT_BLOCK_SIZE, "Not full block");
-
 	(void)logger_exfat_filesystem_claim(dev, NULL, NULL, K_FOREVER);
 
-	LOG_DBG("Writing to logger block: %08X LBA: %08X", phy_block, disk_lba);
-	rc = disk_access_write(config->disk, mem, disk_lba, 1);
+	LOG_DBG("Writing %d blocks: %08X LBA: %08X", num_blocks, start_block, disk_lba);
+	rc = disk_access_write(config->disk, block_data, disk_lba, num_blocks);
 	if (rc == 0) {
 		/* Sync on each write for now */
 		rc = disk_access_ioctl(config->disk, DISK_IOCTL_CTRL_SYNC, NULL);
 	}
 	logger_exfat_filesystem_release(dev);
+
 	return rc;
 }
 
+static int logger_exfat_write(const struct device *dev, uint32_t phy_block,
+			      enum infuse_type data_type, const void *mem, uint16_t mem_len)
+{
+	__ASSERT(mem_len == DATA_LOGGER_EXFAT_BLOCK_SIZE, "Not full block");
+
+	return logger_exfat_write_burst(dev, phy_block, 1, mem);
+}
+
 static int logger_exfat_read(const struct device *dev, uint32_t phy_block, uint16_t block_offset,
 			     void *mem, uint16_t mem_len)
 {
@@ -254,6 +261,9 @@ int logger_exfat_init(const struct device *dev)
 
 const struct data_logger_api data_logger_exfat_api = {
 	.write = logger_exfat_write,
+#ifdef CONFIG_DATA_LOGGER_BURST_WRITES
+	.write_burst = logger_exfat_write_burst,
+#endif /* CONFIG_DATA_LOGGER_BURST_WRITES */
 	.read = logger_exfat_read,
 	.reset = logger_exfat_reset,
 };

subsys/data_logger/data_logger.c

@@ -125,7 +125,7 @@ static int handle_block_write(const struct device *dev, enum infuse_type type, v
 	uint8_t unaligned, padding;
 
 	if (!config->requires_full_block_write && (config->block_write_align > 1)) {
-		/* Check for analigned data block length */
+		/* Check for unaligned data block length */
 		unaligned = block_len % config->block_write_align;
 		if (unaligned > 0) {
 			/* Pad the block to the alignment requirement */
@@ -138,7 +138,68 @@ static int handle_block_write(const struct device *dev, enum infuse_type type, v
 #ifdef CONFIG_DATA_LOGGER_RAM_BUFFER
 	if (config->ram_buf_len) {
 		uint32_t space = config->ram_buf_len - data->ram_buf_offset;
-		int rc;
+		int rc = 0;
+
+#ifdef CONFIG_DATA_LOGGER_BURST_WRITES
+		const struct data_logger_api *api = dev->api;
+
+		if (api->write_burst) {
+			struct data_logger_persistent_block_header *header = block;
+			uint32_t pending;
+
+			/* We have already asserted in `init` that all blocks are the same size */
+			__ASSERT_NO_MSG(IS_PERSISTENT_LOGGER(api));
+			__ASSERT_NO_MSG(block_len == data->block_size);
+
+			/* Push on the block prefix */
+			header->block_type = type;
+			header->block_wrap = (data->current_block / data->physical_blocks) + 1;
+
+			/* Copy block into RAM buffer */
+			memcpy(config->ram_buf_data + data->ram_buf_offset, block, block_len);
+			data->ram_buf_offset += block_len;
+			pending = data->ram_buf_offset / data->block_size;
+			LOG_DBG("RAM buffer: %d/%d", data->ram_buf_offset, config->ram_buf_len);
+
+			if ((data->ram_buf_offset == config->ram_buf_len) ||
+			    (data->current_block + pending == data->logical_blocks)) {
+				int64_t flush_end, flush_start = k_uptime_get();
+				int32_t flush_duration;
+
+				/* Out of space, or end of physical blocks, perform burst write */
+				LOG_DBG("%s writing %d blocks to logical block %u", dev->name,
+					pending, data->current_block);
+
+				/* Request backend to be powered */
+				rc = pm_device_runtime_get(dev);
+				if (rc < 0) {
+					/* Reset pending data, not much else we can do */
+					data->ram_buf_offset = 0;
+					return rc;
+				}
+
+				/* Do the burst write */
+				rc = api->write_burst(dev, data->current_block, pending,
+						      config->ram_buf_data);
+
+				/* Release device after a delay */
+				(void)pm_device_runtime_put_async(dev, K_MSEC(100));
+
+				flush_end = k_uptime_get();
+				data->ram_buf_offset = 0;
+				flush_duration = flush_end - flush_start;
+				LOG_INF("%s -> Flushed %d blocks in %d ms", dev->name, pending,
+					flush_duration);
+
+				if (rc != -ENOMEM) {
+					data->bytes_logged += pending * block_len;
+					data->current_block += pending;
+				}
+			}
+			/* Extra handling not required */
+			return rc;
+		}
+#endif /* CONFIG_DATA_LOGGER_BURST_WRITES */
 
 		if (space > (sizeof(struct ram_buf_header) + block_len)) {
 			/* Space for this block, add header and data to FIFO */
@@ -522,6 +583,20 @@ int data_logger_common_init(const struct device *dev)
 		if (config->ram_buf_len) {
 			LOG_INF("%s -> Extra %zu byte RAM buffer", dev->name, config->ram_buf_len);
 		}
+#ifdef CONFIG_DATA_LOGGER_BURST_WRITES
+		if (api->write_burst) {
+			__ASSERT(IS_PERSISTENT_LOGGER(api), "Expected persistent logger");
+			__ASSERT(config->requires_full_block_write,
+				 "Expected only full block writes");
+			__ASSERT(config->ram_buf_len % data->block_size == 0,
+				 "RAM buffer must be multiple of block size");
+			/* To simplify the initial implementation, only support loggers that don't
+			 * erase (SD)
+			 */
+			__ASSERT(data->logical_blocks == data->physical_blocks,
+				 "Expected no wrapping");
+		}
+#endif /* CONFIG_DATA_LOGGER_BURST_WRITES */
 	}
 #endif /* CONFIG_DATA_LOGGER_RAM_BUFFER */
 	return 0;

tests/subsys/data_logger/backends/exfat_multi_file/src/main.c

@@ -142,6 +142,7 @@ static void test_sequence(bool reinit)
 	}
 
 #ifndef CONFIG_DISK_DRIVER_SDMMC
+	uint32_t failing_block = 0;
 	int rc = 0;
 
 	/* Somewhere in here we should get a write error */
@@ -151,8 +152,14 @@ static void test_sequence(bool reinit)
 
 		/* We expect an expand to fail (-ENOMEM) followed by failures to write (-ENOMEM) */
 		if (rc == -ENOMEM) {
-			uint32_t failing_block = state.current_block;
+			failing_block = state.current_block;
 
+			/* Out of space, expect sizes to be truncated */
+			data_logger_get_state(logger, &state);
+			zassert_equal(failing_block, state.physical_blocks);
+			zassert_equal(failing_block, state.logical_blocks);
+
+			/* Try to write extra blocks */
 			for (int j = 0; j < 5; j++) {
 				rc = data_logger_block_write(logger, 6, input_buffer,
 							     state.block_size);
@@ -164,6 +171,22 @@ static void test_sequence(bool reinit)
 		}
 	}
 	zassert_equal(-ENOMEM, rc);
+
+	/* Re-initialise a full disk (Doesn't know we're out of memory) */
+	zassert_equal(0, logger_exfat_init(logger));
+	data_logger_get_state(logger, &state);
+	zassert_not_equal(0, state.physical_blocks);
+	zassert_not_equal(0, state.logical_blocks);
+	zassert_equal(failing_block, state.current_block);
+	zassert_equal(0, state.earliest_block);
+
+	/* But trying to write again will update state again */
+	rc = data_logger_block_write(logger, 7, input_buffer, state.block_size);
+	data_logger_get_state(logger, &state);
+	zassert_equal(rc, -ENOMEM);
+	zassert_equal(failing_block, state.current_block);
+	zassert_equal(failing_block, state.physical_blocks);
+	zassert_equal(failing_block, state.logical_blocks);
 #endif
 }
 

tests/subsys/data_logger/ram_buffering_disk/CMakeLists.txt

@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: Apache-2.0
+
+cmake_minimum_required(VERSION 3.20.0)
+find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE})
+project(ram_buffering_disk)
+
+target_sources(app PRIVATE
+  src/main.c
+)