Persistent storage (#42)

# Description Implements `storage_start`, `storage_stop`, and `storage_set`, allowing Storage objects to be used outside of acquisition time. Also one or two small unrelated fixes. Related acquire-project/acquire-video-runtime#125 ## Type of change - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [X] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] Documentation - [ ] Other (please describe): # How has this been tested? Tested against runtime [here](acquire-project/acquire-video-runtime#125). # Checklist - [X] I have performed a self-review of my own code. - [X] I have commented my code, particularly in hard-to-understand areas. - [X] I have updated the documentation, if necessary. - [X] I added tests for my changes, and/or the testing strategy is described above. - [X] I updated the [changelog](../CHANGELOG.md). --------- Co-authored-by: Nathan Clack <[email protected]>
acquire-project · Dec 5, 2023 · 60cb52c · 60cb52c
1 parent 8bea9c8
commit 60cb52c
Show file tree

Hide file tree

Showing 4 changed files with 126 additions and 31 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,15 +9,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
-- Users can specify the full chunk size in width, height, and planes. 
+- Users can specify the full chunk size in width, height, and planes.
+- `acquire-device-hal`: `storage_open` no longer takes a `StorageProperties*` parameter.
 
 ### Removed
 
 - `max_bytes_per_chunk` from `StorageProperties` chunking properties.
 
 ### Fixed
 
-- Removes 30-second timeout from `thread_join` on Windows. 
+- Removes 30-second timeout from `thread_join` on Windows.
+- Memory leak in `copy_string`.
+- Avoid an unnecessary call to `realloc`.
 
 ### Added
 
@@ -28,6 +31,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `acquire-device-properties`: A `struct storage_properties_sharding_s` member `shard_dims_chunks` of
   `StorageProperties`. Users can now configure sharding properties where supported.
 - `acquire-device-properties`: A convenience function for setting sharding parameters.
+- `acquire-device-hal`: `storage_start`, `storage_stop`, and `storage_set` functions.
 
 ## [0.1.3](https://github.com/acquire-project/acquire-core-libs/compare/v0.1.2...v0.1.3) - 2023-06-27
 

diff --git a/src/acquire-device-hal/device/hal/storage.c b/src/acquire-device-hal/device/hal/storage.c
@@ -73,8 +73,7 @@ storage_validate(const struct DeviceManager* system,
 
 struct Storage*
 storage_open(const struct DeviceManager* system,
-             const struct DeviceIdentifier* identifier,
-             struct StorageProperties* settings)
+             const struct DeviceIdentifier* identifier)
 {
     struct Storage* self = 0;
 
@@ -90,18 +89,39 @@ storage_open(const struct DeviceManager* system,
         self = containerof(device, struct Storage, device);
     }
 
-    if (self) {
-        self->state = self->set(self, settings);
-        CHECK(self->state == DeviceState_Armed);
-        self->state = self->start(self);
-        CHECK(self->state == DeviceState_Running);
-    }
+    // Check the required interface functions are non-null
+    CHECK(self->set != NULL);
+    CHECK(self->get != NULL);
+    CHECK(self->get_meta != NULL);
+    CHECK(self->start != NULL);
+    CHECK(self->append != NULL);
+    CHECK(self->stop != NULL);
+    CHECK(self->destroy != NULL);
+    CHECK(self->reserve_image_shape != NULL);
+
     return self;
 Error:
     storage_close(self);
     return 0;
 }
 
+enum DeviceStatusCode
+storage_set(struct Storage* self, const struct StorageProperties* settings)
+{
+    CHECK(self);
+    CHECK(settings);
+
+    self->state = self->set(self, settings);
+    EXPECT(DeviceState_Armed == self->state,
+           "Expected Armed. Got %s.",
+           device_state_as_string(self->state));
+
+    return Device_Ok;
+
+Error:
+    return Device_Err;
+}
+
 enum DeviceStatusCode
 storage_get(const struct Storage* self, struct StorageProperties* settings)
 {
@@ -125,6 +145,47 @@ storage_get_meta(const struct Storage* self,
     return Device_Err;
 }
 
+enum DeviceStatusCode
+storage_start(struct Storage* self)
+{
+    CHECK(self);
+    CHECK(self->state == DeviceState_Armed);
+
+    enum DeviceStatusCode status_code;
+    switch (self->state = self->start(self)) {
+        case DeviceState_Running:
+            status_code = Device_Ok;
+            break;
+        default:
+            status_code = Device_Err;
+            break;
+    }
+
+    return status_code;
+Error:
+    return Device_Err;
+}
+
+enum DeviceStatusCode
+storage_stop(struct Storage* self)
+{
+    enum DeviceStatusCode ecode = Device_Ok;
+    CHECK(self);
+    CHECK(self->stop);
+    if (self->state == DeviceState_Running) {
+        EXPECT((self->state = self->stop(self)) == DeviceState_Armed ||
+                 self->state == DeviceState_AwaitingConfiguration,
+               "Expected Armed or AwaitingConfiguration. Got state: %s.",
+               device_state_as_string(self->state));
+    }
+
+Finalize:
+    return ecode;
+Error:
+    ecode = Device_Err;
+    goto Finalize;
+}
+
 enum DeviceStatusCode
 storage_append(struct Storage* self,
                const struct VideoFrame* beg,
@@ -145,25 +206,15 @@ storage_append(struct Storage* self,
     return Device_Err;
 }
 
-enum DeviceStatusCode
+void
 storage_close(struct Storage* self)
 {
-    enum DeviceStatusCode ecode = Device_Ok;
-    CHECK_SILENT(self);
-    self->state = self->stop(self);
-    EXPECT(self->state == DeviceState_Armed ||
-             self->state == DeviceState_AwaitingConfiguration,
-           "Expected Armed or AwaitingConfiguration. Got state: %s.",
-           device_state_as_string(self->state));
-Finalize:
-    if (self) {
-        driver_close_device(&self->device);
-        self->state = DeviceState_Closed;
-    }
-    return ecode;
-Error:
-    ecode = Device_Err;
-    goto Finalize;
+    CHECK(self);
+    storage_stop(self);
+
+    driver_close_device(&self->device);
+    self->state = DeviceState_Closed;
+Error:;
 }
 
 enum DeviceState

diff --git a/src/acquire-device-hal/device/hal/storage.h b/src/acquire-device-hal/device/hal/storage.h
@@ -15,9 +15,19 @@ extern "C"
                          const struct DeviceIdentifier* identifier,
                          const struct StorageProperties* settings);
 
+    /// @brief Open a storage device identified by @p identifier.
+    /// @param[in] system The device manager.
+    /// @return A pointer to the storage device, or NULL if the device could not
+    /// be opened.
     struct Storage* storage_open(const struct DeviceManager* system,
-                                 const struct DeviceIdentifier* identifier,
-                                 struct StorageProperties* settings);
+                                 const struct DeviceIdentifier* identifier);
+
+    /// @brief Set the storage device properties.
+    /// @param[in] settings The properties to set.
+    /// @returns Device_Ok if the properties were set successfully, otherwise
+    /// Device_Err.
+    enum DeviceStatusCode storage_set(struct Storage* self,
+                                      const struct StorageProperties* settings);
 
     enum DeviceStatusCode storage_get(const struct Storage* self,
                                       struct StorageProperties* settings);
@@ -26,14 +36,37 @@ extern "C"
       const struct Storage* self,
       struct StoragePropertyMetadata* meta);
 
+    /// @brief Start the storage device.
+    /// @details This function signals the storage device that data is about to
+    /// begin streaming to the storage. Any preparation the device needs to do
+
+    /// should be handled here (e.g. opening a file or a network connection).
+    /// A successful call to this function will set self->state to
+    /// DeviceState_Running.
+    /// @returns Device_Ok if the device was started successfully, otherwise
+    /// Device_Err.
+    enum DeviceStatusCode storage_start(struct Storage* storage);
+
+    /// @brief Stop the storage device.
+    /// @details This function signals the storage device that no more data is
+    /// forthcoming. The device is free to close any open files or network
+    /// connections. A successful call to this function will set self->state to
+    /// DeviceState_Armed.
+    /// @returns Device_Ok if the device was stopped successfully, otherwise
+    /// Device_Err.
+    enum DeviceStatusCode storage_stop(struct Storage* storage);
+
     /// @brief Append data in `[beg,end)` to Storage
     /// @param[in] beg The beginning of the packet of frames to write.
     /// @param[in] end The end of the packet of frames to write.
     enum DeviceStatusCode storage_append(struct Storage* self,
                                          const struct VideoFrame* beg,
                                          const struct VideoFrame* end);
 
-    enum DeviceStatusCode storage_close(struct Storage* self);
+    /// @brief Close the storage device.
+    /// @details The storage device is deallocated and any resources it was
+    /// using are freed.
+    void storage_close(struct Storage* self);
 
     enum DeviceState storage_get_state(const struct Storage* self);
 

diff --git a/src/acquire-device-properties/device/props/storage.c b/src/acquire-device-properties/device/props/storage.c
@@ -32,12 +32,19 @@ copy_string(struct String* dst, const struct String* src)
         // dst string pointer refers to caller-allocated memory.
         // Allocate a new string on the heap.
         CHECK(dst->str = malloc(src->nbytes)); // NOLINT
+        dst->nbytes = src->nbytes;
         dst->is_ref = 0;                       // mark as owned
     }
 
     CHECK(dst->is_ref == 0);
     if (src->nbytes > dst->nbytes) {
-        CHECK(dst->str = realloc(dst->str, src->nbytes));
+        char* str = realloc(dst->str, src->nbytes);
+        if (!str) {
+            LOGE("Failed to allocate %llu bytes for string copy.",
+                 (unsigned long long)src->nbytes);
+            goto Error;
+        }
+        dst->str = str;
     }
 
     dst->nbytes = src->nbytes;