<div dir="ltr"><div dir="ltr">Hi Paul,<div><br></div><div>Thank you for your review feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 1 Mar 2021 at 09:25, <<a href="mailto:paul.elder@ideasonboard.com">paul.elder@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
On Tue, Feb 16, 2021 at 08:53:40AM +0000, Naushir Patuck wrote:<br>
> On DelayedControls::reset(), the values retrieved from the sensor device<br>
> were added to the queues with the updated flag set to true. This would<br>
> cause the helper to write out the value to the device again on the first<br>
> DelayedControls::applyControls() call. This is unnecessary, as the<br>
> controls written are identical to what is stored in the device driver.<br>
> <br>
> Fix this by explicitly setting the update flag to false in<br>
> DelayedControls::reset() when adding the controls to the queue.<br>
> <br>
> Additionally, use the Info() constructor when adding items to the queue<br>
> for consistency.<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> Fixes: 3d4b7b005911 ("libcamera: delayed_controls: Add helper for controls that apply with a delay")<br>
> ---<br>
>  include/libcamera/internal/delayed_controls.h |  4 ++--<br>
>  src/libcamera/delayed_controls.cpp            | 14 ++++++++------<br>
>  2 files changed, 10 insertions(+), 8 deletions(-)<br>
> <br>
> diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h<br>
> index 564d9f2e2440..2a6a912bde10 100644<br>
> --- a/include/libcamera/internal/delayed_controls.h<br>
> +++ b/include/libcamera/internal/delayed_controls.h<br>
> @@ -43,8 +43,8 @@ private:<br>
>               {<br>
>               }<br>
>  <br>
> -             Info(const ControlValue &v)<br>
> -                     : ControlValue(v), updated(true)<br>
> +             Info(const ControlValue &v, bool updated_ = true)<br>
> +                     : ControlValue(v), updated(updated_)<br>
>               {<br>
>               }<br>
>  <br>
> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp<br>
> index 3ed1dfebd035..21dc3e164fe9 100644<br>
> --- a/src/libcamera/delayed_controls.cpp<br>
> +++ b/src/libcamera/delayed_controls.cpp<br>
> @@ -111,7 +111,11 @@ void DelayedControls::reset()<br>
>       values_.clear();<br>
>       for (const auto &ctrl : controls) {<br>
>               const ControlId *id = device_->controls().idmap().at(ctrl.first);<br>
> -             values_[id][0] = Info(ctrl.second);<br>
> +             /*<br>
> +              * Do not mark this control value as updated, it does not need<br>
> +              * to be written to to device on startup.<br>
> +              */<br>
> +             values_[id][0] = Info(ctrl.second, false);<br>
>       }<br>
>  }<br>
>  <br>
> @@ -126,11 +130,10 @@ void DelayedControls::reset()<br>
>   */<br>
>  bool DelayedControls::push(const ControlList &controls)<br>
>  {<br>
> -     /* Copy state from previous frame. */<br>
> +     /* Copy state from previous frame and clear the updated flag */<br>
>       for (auto &ctrl : values_) {<br>
>               Info &info = ctrl.second[queueCount_];<br>
> -             info = values_[ctrl.first][queueCount_ - 1];<br>
> -             info.updated = false;<br>
> +             info = Info(values_[ctrl.first][queueCount_ - 1], false);<br>
<br>
Isn't values_[ctrl.first][queueCount_ - 1] an Info instance? I didn't<br>
think implicit copy constructor worked with the extra argument.<br></blockquote><div><br></div><div>It works as expected, but does seem confusing.  I will update the code</div><div>above to be more explicit.</div><div><br></div><div>Thanks,</div><div>Naush</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
The rest looks good to me.<br>
<br>
<br>
Paul<br>
<br>
>       }<br>
>  <br>
>       /* Update with new controls. */<br>
> @@ -150,8 +153,7 @@ bool DelayedControls::push(const ControlList &controls)<br>
>  <br>
>               Info &info = values_[id][queueCount_];<br>
>  <br>
> -             info = control.second;<br>
> -             info.updated = true;<br>
> +             info = Info(control.second);<br>
>  <br>
>               LOG(DelayedControls, Debug)<br>
>                       << "Queuing " << id->name()<br>
> -- <br>
> 2.25.1<br>
> <br>
> _______________________________________________<br>
> libcamera-devel mailing list<br>
> <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
</blockquote></div></div>