Fix setting ROI

CHANGELOG.md

@@ -7,9 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
-## [0.1.6](https://github.com/acquire-project/acquire-driver-hdcam/compare/v0.1.5...v0.1.6) - 2023-08-10
+## [0.1.6](https://github.com/acquire-project/acquire-driver-hdcam/compare/v0.1.5...v0.1.6) - 2023-08-15
 
 - Fixes a bug where the output trigger source property was set when it shouldn't be.
+- Fixes a bug where roi bound checking wasn't working.
 
 ## [0.1.5](https://github.com/acquire-project/acquire-driver-hdcam/compare/v0.1.4...v0.1.5) - 2023-07-28
 

src/dcam.camera.c

@@ -1,5 +1,6 @@
 #include "dcam.camera.h"
 #include "device/props/device.h"
+#include "device/props/metadata.h"
 #include "logger.h"
 
 #include "dcam.error.h"
@@ -365,14 +366,14 @@ aq_dcam_get_metadata__inner(const struct Dcam4Camera* self,
     is_ok &= READ_PROP_META(
       &metadata->readout_direction, DCAM_IDPROP_READOUT_DIRECTION, 1.0f);
 
-    is_ok &=
-      READ_PROP_META(&metadata->offset.x, DCAM_IDPROP_SUBARRAYHPOS, 1.0f);
-    is_ok &=
-      READ_PROP_META(&metadata->offset.y, DCAM_IDPROP_SUBARRAYVPOS, 1.0f);
     is_ok &=
       READ_PROP_META(&metadata->shape.x, DCAM_IDPROP_SUBARRAYHSIZE, 1.0f);
     is_ok &=
       READ_PROP_META(&metadata->shape.y, DCAM_IDPROP_SUBARRAYVSIZE, 1.0f);
+    is_ok &=
+      READ_PROP_META(&metadata->offset.x, DCAM_IDPROP_SUBARRAYHPOS, 1.0f);
+    is_ok &=
+      READ_PROP_META(&metadata->offset.y, DCAM_IDPROP_SUBARRAYVPOS, 1.0f);
 
 #undef READ_PROP_META
 
@@ -462,6 +463,11 @@ aq_dcam_set__inner(struct Dcam4Camera* self,
 
     int is_ok = 1;
 
+    // Set readout speed and mode.
+    // It's important to do this first as it effects what settings are valid
+    // for downstream parameters (e.g. subarray, and exposure)
+    is_ok &= set_readout_speed(hdcam);
+
     // pixel type
     if (IS_CHANGED(pixel_type)) {
         is_ok &= set_sample_type(hdcam, &props->pixel_type);
@@ -513,19 +519,28 @@ aq_dcam_set__inner(struct Dcam4Camera* self,
     // roi
     if (IS_CHANGED(offset.x) || IS_CHANGED(offset.y) || IS_CHANGED(shape.x) ||
         IS_CHANGED(shape.y)) {
-        dcamprop_setvalue(hdcam, DCAM_IDPROP_SUBARRAYMODE, DCAMPROP_MODE__ON);
-        is_ok &=
-          prop_write(u32, hdcam, DCAM_IDPROP_SUBARRAYHPOS, &props->offset.x);
-        is_ok &=
-          prop_write(u32, hdcam, DCAM_IDPROP_SUBARRAYVPOS, &props->offset.y);
+        is_ok &= (dcamprop_setvalue(hdcam,
+                                    DCAM_IDPROP_SUBARRAYMODE,
+                                    DCAMPROP_MODE__ON) == DCAMERR_SUCCESS);
+
+        // temporarily set offset to (0,0) so we're free to change the shape
+        is_ok &= dcamprop_setvalue(hdcam, DCAM_IDPROP_SUBARRAYVPOS, 0.0f) ==
+                 DCAMERR_SUCCESS;
+        is_ok &= dcamprop_setvalue(hdcam, DCAM_IDPROP_SUBARRAYHPOS, 0.0f) ==
+                 DCAMERR_SUCCESS;
+
         is_ok &=
           prop_write(u32, hdcam, DCAM_IDPROP_SUBARRAYHSIZE, &props->shape.x);
         is_ok &=
           prop_write(u32, hdcam, DCAM_IDPROP_SUBARRAYVSIZE, &props->shape.y);
+
+        is_ok &=
+          prop_write(u32, hdcam, DCAM_IDPROP_SUBARRAYVPOS, &props->offset.y);
+        is_ok &=
+          prop_write(u32, hdcam, DCAM_IDPROP_SUBARRAYHPOS, &props->offset.x);
     }
 
     // exposure
-    is_ok &= set_readout_speed(hdcam);
     if (IS_CHANGED(exposure_time_us) || IS_CHANGED(line_interval_us)) {
         is_ok &= prop_write_scaled(f32,
                                    hdcam,

src/dcam.getset.c

@@ -50,11 +50,12 @@ prop_write_u32(HDCAM hdcam,
                const char* prop_name)
 {
     double v = *value;
-    DCAM(dcamprop_setgetvalue(hdcam, prop_id, &v, 0));
+    DCAMERR ecode;
+    DCAM(ecode = dcamprop_setgetvalue(hdcam, prop_id, &v, 0));
     *value = (uint32_t)v;
     return 1;
 Error:
-    LOG("Failed to write %s", prop_name);
+    LOG("Failed to write %s.", prop_name);
     return 0;
 }
 

tests/CMakeLists.txt

@@ -22,6 +22,7 @@ else()
         dcam-reset-on-fail
         one-video-stream
         set-output-trigger
+        set-roi
     )
 
     foreach(name ${tests})

tests/set-roi.cpp

@@ -0,0 +1,99 @@
+//! Test: Runs through setting up acquiring from a sub-rectangle of the sensor
+#include "acquire.h"
+#include "device/hal/device.manager.h"
+#include "device/props/components.h"
+#include "logger.h"
+#include <stdio.h>
+#include <string.h>
+
+#define L (aq_logger)
+#define LOG(...) L(0, __FILE__, __LINE__, __FUNCTION__, __VA_ARGS__)
+#define ERR(...) L(1, __FILE__, __LINE__, __FUNCTION__, __VA_ARGS__)
+#define EXPECT(e, ...)                                                         \
+    do {                                                                       \
+        if (!(e)) {                                                            \
+            ERR(__VA_ARGS__);                                                  \
+            goto Error;                                                        \
+        }                                                                      \
+    } while (0)
+#define CHECK(e) EXPECT(e, "Expression evaluated as false: %s", #e)
+#define DEVOK(e) CHECK(Device_Ok == (e))
+#define OK(e) CHECK(AcquireStatus_Ok == (e))
+
+/// Check that a==b
+/// example: `ASSERT_EQ(int,"%d",42,meaning_of_life())`
+#define ASSERT_EQ(T, fmt, a, b)                                                \
+    do {                                                                       \
+        T a_ = (T)(a);                                                         \
+        T b_ = (T)(b);                                                         \
+        EXPECT(a_ == b_, "Expected %s==%s but " fmt "!=" fmt, #a, #b, a_, b_); \
+    } while (0)
+
+#define countof(e) (sizeof(e) / sizeof(*(e)))
+
+void
+reporter(int is_error,
+         const char* file,
+         int line,
+         const char* function,
+         const char* msg)
+{
+    fprintf(is_error ? stderr : stdout,
+            "%s%s(%d) - %s: %s\n",
+            is_error ? "ERROR " : "",
+            file,
+            line,
+            function,
+            msg);
+}
+
+int
+setup(AcquireRuntime* runtime)
+{
+    AcquireProperties props = {};
+
+    const DeviceManager* dm = 0;
+    CHECK(dm = acquire_device_manager(runtime));
+
+#define SIZED(s) s, sizeof(s) - 1
+    DEVOK(device_manager_select(dm,
+                                DeviceKind_Camera,
+                                SIZED("Hamamatsu C15440-20UP.*"),
+                                &props.video[0].camera.identifier));
+    DEVOK(device_manager_select(dm,
+                                DeviceKind_Storage,
+                                SIZED("Trash"),
+                                &props.video[0].storage.identifier));
+#undef SIZED
+
+    props.video[0].max_frame_count = 10;
+    props.video[0].camera.settings.shape = { .x = 1700, .y = 512 };
+    props.video[0].camera.settings.offset = { .x = 304, .y = 896 };
+    OK(acquire_configure(runtime, &props));
+    // Expect that the roi is what we intended.
+
+    ASSERT_EQ(int, "%d", props.video[0].camera.settings.shape.x, 1700);
+    ASSERT_EQ(int, "%d", props.video[0].camera.settings.shape.y, 512);
+    ASSERT_EQ(int, "%d", props.video[0].camera.settings.offset.x, 304);
+    ASSERT_EQ(int, "%d", props.video[0].camera.settings.offset.y, 896);
+    return 1;
+Error:
+    return 0;
+}
+
+int
+main()
+{
+    int ecode = 0;
+    AcquireRuntime* runtime = 0;
+    CHECK(runtime = acquire_init(reporter));
+    CHECK(setup(runtime));
+    OK(acquire_start(runtime));
+    OK(acquire_stop(runtime));
+Finalize:
+    acquire_shutdown(runtime);
+    return ecode;
+Error:
+    ecode = 1;
+    goto Finalize;
+}