GiGE: Fix binning

At least for FLIR GiGE cameras, changing binning changes the apparent size
of the chip, such that the region of interest is configured using a smaller
number of pixels.  This appears to be contrary to how INDI expects binning
to be configured.  Following the CCD Simulator (hopefully it is a decent
example of implementing a INDI::CCD device), it rather appears that the
region of interest is still expected to be set using the entire available
Width/Height.  It therefore appears necessary to mitigate this mismatch
since (at least for ekos) the client appears to mess up the apparent image
if binning is configured but a smaller region is selected, but in reality
the entire avaialble sensor was binned.

So, for now, even when binning is used, this patch makes the driver use the
full WidthMax and HeightMax manually divided by the binning to select the
region of interest.

This means that any use of get_x_offset().val(), get_y_offset().val(),
get_width().val(), or get_height().val() from the GigECCD::camera object must
be manually multiplied or divided by the values from get_bin_x().val() and
get_bin_y().val() as appropriate.  Similarly, camera->set_geometry(...) must
be given values that are divided by the appropriate binning.

It remains to verify this behavior on GiGE cameras from other vendors, or
at least to check what the standard says, if anything useful on the matter.

Author: olsonse

indi-gige/src/ArvGeneric.cpp

@@ -137,6 +137,13 @@ bool ArvGeneric::_get_bounds(void (*fn_arv_bounds)(::ArvCamera *, T *min, T *max
     return true;
 }
 
+template <typename T>
+bool ArvGeneric::_get_incr(T (*fn_arv_incr)(::ArvCamera *, GError**), min_max_property<T> *prop)
+{
+    prop->set_increment(CALL_RETURN(fn_arv_incr, this->camera));
+    return true;
+}
+
 bool ArvGeneric::is_exposing_single()
 {
     bool single = this->single_acquisition_active.load();
@@ -238,17 +245,24 @@ bool ArvGeneric::_set_initial_config()
     return true;
 }
 
+#define _GET_BOUNDS(T, feature, prop) \
+  this->_get_bounds<T>(arv_camera_get_ ## feature ## _bounds, &this->cam.prop)
+#define _GET_BOUNDS_INCR(T, feature, prop) \
+  _GET_BOUNDS(T, feature, prop); \
+  this->_get_incr<T>(arv_camera_get_ ## feature ## _increment, &this->cam.prop)
+
+
 bool ArvGeneric::_get_initial_config()
 {
-    this->_get_bounds<gint>(arv_camera_get_x_binning_bounds, &this->cam.bin_x);
-    this->_get_bounds<gint>(arv_camera_get_y_binning_bounds, &this->cam.bin_y);
-    this->_get_bounds<gint>(arv_camera_get_x_offset_bounds, &this->cam.x_offset);
-    this->_get_bounds<gint>(arv_camera_get_y_offset_bounds, &this->cam.y_offset);
-    this->_get_bounds<gint>(arv_camera_get_width_bounds, &this->cam.width);
-    this->_get_bounds<gint>(arv_camera_get_height_bounds, &this->cam.height);
-    this->_get_bounds<double>(arv_camera_get_frame_rate_bounds, &this->cam.frame_rate);
-    this->_get_bounds<double>(arv_camera_get_exposure_time_bounds, &this->cam.exposure);
-    this->_get_bounds<double>(arv_camera_get_gain_bounds, &this->cam.gain);
+    _GET_BOUNDS_INCR(gint, x_binning, bin_x);
+    _GET_BOUNDS_INCR(gint, y_binning, bin_y);
+    _GET_BOUNDS_INCR(gint, x_offset, x_offset);
+    _GET_BOUNDS_INCR(gint, y_offset, y_offset);
+    _GET_BOUNDS_INCR(gint, width, width);
+    _GET_BOUNDS_INCR(gint, height, height);
+    _GET_BOUNDS(double, frame_rate, frame_rate);
+    _GET_BOUNDS(double, exposure_time, exposure);
+    _GET_BOUNDS(double, gain, gain);
 
     /* No GVCP call for this..., specializations of this class could make this
      * read-only by setting min=max=val (see BlackFly implementation). */
@@ -275,19 +289,17 @@ void ArvGeneric::set_geometry(int const x, int const y, int const w, int const h
                                 this->cam.height.val());
 }
 
-void ArvGeneric::update_geometry(void)
+std::tuple<int,int,int,int> ArvGeneric::update_geometry(void)
 {
-    gint x, y, w, h, binx, biny;
+    gint x, y, w, h;
 
     CALL(arv_camera_get_region, this->camera, &x, &y, &w, &h);
-    CALL(arv_camera_get_binning, this->camera, &binx, &biny);
 
     this->cam.x_offset.set(x);
     this->cam.y_offset.set(y);
     this->cam.width.set(w);
     this->cam.height.set(h);
-    this->cam.bin_x.set(binx);
-    this->cam.bin_y.set(biny);
+    return std::tuple<int,int,int,int>(x, y, w, h);
 }
 
 void ArvGeneric::set_bin(int const bin_x, int const bin_y)
@@ -298,6 +310,17 @@ void ArvGeneric::set_bin(int const bin_x, int const bin_y)
     CALL(arv_camera_set_binning, this->camera, this->cam.bin_x.val(), this->cam.bin_y.val());
 }
 
+std::pair<int,int> ArvGeneric::update_bin()
+{
+    /* read this back to ensure we know what binning was actually set.  For at
+     * least some cameras, a binning of 3 gets promoted to a binning of 4. */
+    gint binx, biny;
+    CALL(arv_camera_get_binning, this->camera, &binx, &biny);
+    this->cam.bin_x.set(binx);
+    this->cam.bin_y.set(biny);
+    return std::pair<int,int>(binx, biny);
+}
+
 template <typename T>
 void ArvGeneric::set_cam_exposure_property(
   void (*arv_set)(::ArvCamera *, T, GError**),

indi-gige/src/ArvGeneric.h

@@ -64,8 +64,9 @@ class ArvGeneric : public arv::ArvCamera
     virtual double get_float(const char * feature) override;
 
     virtual void set_bin(int const bin_x, int const bin_y) override;
+    virtual std::pair<int,int> update_bin(void) override;
     virtual void set_geometry(int const x, int const y, int const w, int const h) override;
-    virtual void update_geometry(void) override;
+    virtual std::tuple<int,int,int,int> update_geometry(void) override;
     virtual void set_exposure_time(double const val) override;
     virtual void set_gain(double const val) override;
 
@@ -83,6 +84,8 @@ class ArvGeneric : public arv::ArvCamera
   private:
     template <typename T>
     bool _get_bounds(void (*fn_arv_bounds)(::ArvCamera *, T *min, T *max, GError**), min_max_property<T> *prop);
+    template <typename T>
+    bool _get_incr(T (*fn_arv_incr)(::ArvCamera *, GError**), min_max_property<T> *prop);
 
     /** Set a given property where it should not be changed during a
      * single-frame acquisition and attemping to do so should cause the

indi-gige/src/ArvInterface.h

@@ -23,6 +23,7 @@
 #include <cstddef>
 
 #include <string>
+#include <utility>
 #include <functional>
 
 namespace arv
@@ -40,12 +41,13 @@ template <class T>
 class min_max_property
 {
   public:
-    min_max_property() {}
-    min_max_property(T const min, T const max, T const val)
+    min_max_property() : _min(0), _max(0), _val(0), _incr(0) {}
+    min_max_property(T const min, T const max, T const val, T const incr = T(0))
     {
         this->_min = min;
         this->_max = max;
         this->_val = val;
+        this->_incr = incr;
     }
 
     void update(T const min, T const max)
@@ -54,8 +56,19 @@ class min_max_property
         this->_max = max;
     }
 
-    void set(T const new_val)
+    void set_increment(T const incr)
     {
+        this->_incr = incr;
+    }
+
+    /** Set a new value for the property, obeying max/min limits and following
+     * incremental restrictions if set (i.e. if not 0). */
+    void set(T new_val)
+    {
+        if (this->_incr != T(0))
+            // enforce alignment to increments
+            new_val = int((new_val - this->_min)/this->_incr) * this->_incr + this->_min;
+
         if (new_val > this->_max)
             this->_val = this->_max;
         else if (new_val < this->_min)
@@ -68,9 +81,10 @@ class min_max_property
     T val() { return this->_val; }
     T min() { return this->_min; }
     T max() { return this->_max; }
+    T incr() { return this->_incr; }
 
   private:
-    T _min, _max, _val;
+    T _min, _max, _val, _incr;
 };
 
 class ArvCamera
@@ -113,8 +127,15 @@ class ArvCamera
 
     /* Set geometry */
     virtual void set_bin(int const bin_x, int const bin_y)                        = 0;
+    /** update this class information of binning from the camera hardware.
+     * @return pair of binning values for x and y.
+     */
+    virtual std::pair<int,int> update_bin(void)                                   = 0;
     virtual void set_geometry(int const x, int const y, int const w, int const h) = 0;
-    virtual void update_geometry(void)                                            = 0;
+    /** update this class information of geometry from the camera hardware.
+     * @return tuple of (x, y, w, h).
+     */
+    virtual std::tuple<int,int,int,int> update_geometry(void)                     = 0;
 
     /* Set exposure */
     virtual void set_exposure_time(double const val) = 0;

indi-gige/src/indi_gige.cpp

@@ -45,6 +45,31 @@
 #define TIMER_US_TO_S  (1000000)
 #define CAPS           (CCD_CAN_ABORT | CCD_CAN_BIN | CCD_CAN_SUBFRAME | CCD_HAS_STREAMING)
 
+/* NOTE ABOUT BINNING:
+ * At least for FLIR GiGE cameras, changing binning changes the apparent size of
+ * the chip, such that the region of interest is configured using a smaller
+ * number of pixels.  This appears to be contrary to how INDI expects binning to
+ * be configured.  Following the CCD Simulator (if that is a decent example of
+ * implementing a INDI::CCD device, it rather appears that the region of
+ * interest is still expected to be set using the entire available Width/Height.
+ * It therefore appears necessary to mitigate this mismatch since (at least for
+ * ekos) the client appears to mess up the apparent image if binning is
+ * configured but a smaller region is selected, but in reality the entire
+ * avaialble sensor was binned.
+ *
+ * So, for now (as of 26 Feb 2026), even when binning is used, this driver will
+ * use the full MaxWidth and MaxHeight manually divided by the binning to select
+ * the region of interest.
+ *
+ * This means that any use of get_x_offset().val(), get_y_offset().val(),
+ * get_width().val(), or get_height().val() from the GigECCD::camera object must
+ * be manually multiplied or divided by the values from get_bin_x().val() and
+ * get_bin_y().val() as appropriate.  Similarly, camera->set_geometry(...) must
+ * be given values that are divided by the appropriate binning.
+ *
+ * TODO: verify this behavior on GiGE cameras from other vendors.
+ */
+
 static class Loader
 {
     std::deque<std::unique_ptr<GigECCD>> cameras;
@@ -85,18 +110,23 @@ bool GigECCD::_update_geometry(void)
 {
     std::lock_guard<std::recursive_mutex> lock(this->camera_mutex);
     /* Get actual values */
-    this->camera->update_geometry();
+    auto [x, y, w, h] = this->camera->update_geometry();
+
+    /* As per note at begin of this file, we manually manage the region of
+     * interest conversion between the camera and INDI because the camera
+     * changes the apparent region of interest available depending on binning,
+     * while INDI does not. */
+    auto bin_x = this->camera->get_bin_x().val(),
+         bin_y = this->camera->get_bin_y().val();
 
     /* Sync these with INDI */
-    PrimaryCCD.setBin(this->camera->get_bin_x().val(), this->camera->get_bin_y().val());
-    PrimaryCCD.setFrame(this->camera->get_x_offset().val(), this->camera->get_y_offset().val(),
-                        this->camera->get_width().val(), this->camera->get_height().val());
+    INDI::CCD::UpdateCCDFrame(x*bin_x, y*bin_y, w*bin_x, h*bin_y);
 
     /* Sanity checks, reserve buffers */
-    [[maybe_unused]] int const width           = this->camera->get_width().val();
-    [[maybe_unused]] int const height          = this->camera->get_height().val();
     int const frame_byte_size = this->camera->get_frame_byte_size();
-    int const indi_bufsize    = PrimaryCCD.getSubW() * PrimaryCCD.getSubH() * PrimaryCCD.getBPP() / 8;
+    int const indi_bufsize    = (PrimaryCCD.getSubW() / PrimaryCCD.getBinX())
+                              * (PrimaryCCD.getSubH() / PrimaryCCD.getBinY())
+                              * PrimaryCCD.getBPP() / 8;
 
     if (indi_bufsize != frame_byte_size)
     {
@@ -111,7 +141,6 @@ bool GigECCD::_update_geometry(void)
     }
 
     this->Streamer->setPixelFormat(INDI_MONO, PrimaryCCD.getBPP());
-    this->Streamer->setSize(PrimaryCCD.getXRes(), PrimaryCCD.getYRes());
     return true;
 }
 
@@ -173,6 +202,7 @@ bool GigECCD::updateProperties()
                            this->camera->get_bpp().val(), this->camera->get_pixel_pitch().val(),
                            this->camera->get_pixel_pitch().val());
 
+        this->_update_bin();
         (void)this->_update_geometry();
         this->timer_id = this->SetTimer(this->getCurrentPollingPeriod());
     }
@@ -461,10 +491,19 @@ bool GigECCD::ISNewNumber(const char *dev, const char *name, double values[], ch
 
 bool GigECCD::UpdateCCDFrame(int x, int y, int w, int h)
 {
-    LOGF_INFO("%s x=%i y=%i w=%i h=%i", __PRETTY_FUNCTION__, x, y, w, h);
-
     std::lock_guard<std::recursive_mutex> lock(this->camera_mutex);
-    this->camera->set_geometry(x, y, w, h);
+
+    /* As per note at begin of this file, we manually manage the region of
+     * interest conversion between the camera and INDI because the camera
+     * changes the apparent region of interest available depending on binning,
+     * while INDI does not. */
+    auto bin_x = this->camera->get_bin_x().val(),
+         bin_y = this->camera->get_bin_y().val();
+
+    LOGF_INFO("%s x=%i y=%i w=%i h=%i binx=%i biny=%i",
+              __PRETTY_FUNCTION__, x, y, w, h, bin_x, bin_y);
+
+    this->camera->set_geometry(x/bin_x, y/bin_y, w/bin_x, h/bin_y);
     return this->_update_geometry();
 }
 
@@ -473,9 +512,17 @@ bool GigECCD::UpdateCCDBin(int binx, int biny)
     LOGF_INFO("%s binx=%i biny=%i", __PRETTY_FUNCTION__, binx, biny);
     std::lock_guard<std::recursive_mutex> lock(this->camera_mutex);
     camera->set_bin(binx, biny);
+    this->_update_bin();
     return UpdateCCDFrame(PrimaryCCD.getSubX(), PrimaryCCD.getSubY(), PrimaryCCD.getSubW(), PrimaryCCD.getSubH());
 }
 
+void GigECCD::_update_bin(void) {
+    auto [bx, by] = this->camera->update_bin(); /* Get actual values */
+
+    /* Sync actuals values to INDI */
+    INDI::CCD::UpdateCCDBin(bx, by);
+}
+
 bool GigECCD::UpdateCCDFrameType(INDI::CCDChip::CCD_FRAME fType)
 {
     LOGF_INFO("%s", __PRETTY_FUNCTION__);

indi-gige/src/indi_gige.h

@@ -65,7 +65,8 @@ class GigECCD : public INDI::CCD
   private:
     void _delete_indi_properties(void);
     void _update_indi_properties(void);
-    bool _update_geometry(void);
+    void _update_bin(void); /// update binning to INDI from hardware
+    bool _update_geometry(void); /// update geometry to INDI from hardware
     void _update_image(uint8_t const *const data, size_t size);
 
     void _handle_failed(void);