[libcamera-devel] [PATCH v1 2/3] ipa: rpi: agc: Use std::string instead of char arrays

[Kieran Bingham]

Hi Naush,

Quoting Naushir Patuck via libcamera-devel (2023-06-07 11:00:53)
> Replace the char array strings in struct AgcStatus with std::string
> objects. This simplifies the string handling in the source code.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>

This seems better/cleaner indeed.

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> ---
>  src/ipa/rpi/controller/agc_status.h |  8 +++++---
>  src/ipa/rpi/controller/rpi/agc.cpp  | 23 +++++++----------------
>  2 files changed, 12 insertions(+), 19 deletions(-)
> 
> diff --git a/src/ipa/rpi/controller/agc_status.h b/src/ipa/rpi/controller/agc_status.h
> index 6abf09d9df57..6c112e76aa12 100644
> --- a/src/ipa/rpi/controller/agc_status.h
> +++ b/src/ipa/rpi/controller/agc_status.h
> @@ -6,6 +6,8 @@
>   */
>  #pragma once
>  
> +#include <string>
> +
>  #include <libcamera/base/utils.h>
>  
>  /*
> @@ -24,9 +26,9 @@ struct AgcStatus {
>         libcamera::utils::Duration targetExposureValue; /* (unfiltered) target total exposure AGC is aiming for */
>         libcamera::utils::Duration shutterTime;
>         double analogueGain;
> -       char exposureMode[32];
> -       char constraintMode[32];
> -       char meteringMode[32];
> +       std::string exposureMode;
> +       std::string constraintMode;
> +       std::string meteringMode;
>         double ev;
>         libcamera::utils::Duration flickerPeriod;
>         int floatingRegionEnable;
> diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp
> index e79c82e2e65b..b611157af1f0 100644
> --- a/src/ipa/rpi/controller/rpi/agc.cpp
> +++ b/src/ipa/rpi/controller/rpi/agc.cpp
> @@ -226,7 +226,7 @@ Agc::Agc(Controller *controller)
>          * Setting status_.totalExposureValue_ to zero initially tells us
>          * it's not been calculated yet (i.e. Process hasn't yet run).
>          */
> -       memset(&status_, 0, sizeof(status_));
> +       status_ = {};
>         status_.ev = ev_;
>  }
>  
> @@ -524,12 +524,6 @@ void Agc::updateLockStatus(DeviceStatus const &deviceStatus)
>         status_.locked = lockCount_ == maxLockCount;
>  }
>  
> -static void copyString(std::string const &s, char *d, size_t size)
> -{
> -       size_t length = s.copy(d, size - 1);
> -       d[length] = '\0';
> -}
> -
>  void Agc::housekeepConfig()
>  {
>         /* First fetch all the up-to-date settings, so no one else has to do it. */
> @@ -544,30 +538,27 @@ void Agc::housekeepConfig()
>          * Make sure the "mode" pointers point to the up-to-date things, if
>          * they've changed.
>          */
> -       if (strcmp(meteringModeName_.c_str(), status_.meteringMode)) {
> +       if (meteringModeName_ != status_.meteringMode) {
>                 auto it = config_.meteringModes.find(meteringModeName_);
>                 if (it == config_.meteringModes.end())
>                         LOG(RPiAgc, Fatal) << "No metering mode " << meteringModeName_;
>                 meteringMode_ = &it->second;
> -               copyString(meteringModeName_, status_.meteringMode,
> -                          sizeof(status_.meteringMode));
> +               status_.meteringMode = meteringModeName_;
>         }
> -       if (strcmp(exposureModeName_.c_str(), status_.exposureMode)) {
> +       if (exposureModeName_ != status_.exposureMode) {
>                 auto it = config_.exposureModes.find(exposureModeName_);
>                 if (it == config_.exposureModes.end())
>                         LOG(RPiAgc, Fatal) << "No exposure profile " << exposureModeName_;
>                 exposureMode_ = &it->second;
> -               copyString(exposureModeName_, status_.exposureMode,
> -                          sizeof(status_.exposureMode));
> +               status_.exposureMode = exposureModeName_;
>         }
> -       if (strcmp(constraintModeName_.c_str(), status_.constraintMode)) {
> +       if (constraintModeName_ != status_.constraintMode) {
>                 auto it =
>                         config_.constraintModes.find(constraintModeName_);
>                 if (it == config_.constraintModes.end())
>                         LOG(RPiAgc, Fatal) << "No constraint list " << constraintModeName_;
>                 constraintMode_ = &it->second;
> -               copyString(constraintModeName_, status_.constraintMode,
> -                          sizeof(status_.constraintMode));
> +               status_.constraintMode = constraintModeName_;
>         }
>         LOG(RPiAgc, Debug) << "exposureMode "
>                            << exposureModeName_ << " constraintMode "
> -- 
> 2.34.1
>