<div dir="ltr"><div dir="ltr">Hi Kieran,<div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 12 Mar 2021 at 13:38, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@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>
<br>
On 04/03/2021 08:17, Naushir Patuck wrote:<br>
> The recent fixes applied to DelayedControls change the behavior of the<br>
> library. As such, the tests ended up failing as they relied on the old<br>
> behavior of the helper. Update the tests to account for the new behavior<br>
> and get the tests passing again.<br>
> <br>
> Specifically, the following changes have been made for each test:<br>
> <br>
> singleControlNoDelay():<br>
> - Add a call to reset() to initialise internal DelayedControls state<br>
> only after setting the control on the device.<br>
> - Trigger a first frame start by calling applyControls() as a pipeline<br>
> handler would.<br>
> - Test frames from 1 onwards, as we will never have a queue item at<br>
> frame 0, since the reset() handles that.<br>
> <br>
> singleControlWithDelay():<br>
> - Trigger a first frame start by calling applyControls() as a pipeline<br>
> handler would.<br>
> - Test frames from 1 onwards, as we will never have a queue item at<br>
> frame 0, since the reset() handles that.<br>
> <br>
> dualControlsWithDelay() and dualControlsMultiQueue():<br>
> - Trigger a first frame start by calling applyControls() as a pipeline<br>
> handler would.<br>
> - Test frames from (startOffset + 1) onwards, as we will never have a<br>
> queue item at frame startOffset.<br>
> - Use the max delay (2 in this case) to determine the expected result<br>
> value.<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
<br>
I wondered if these should be squashed into the commits that change it -<br>
but I don't think it's a problem.<br></blockquote><div><br></div><div>I did consider that before submitting this version.  However, it does get a bit</div><div>unwieldy.  No one previous commit fixes the overall behaviour of the helper,</div><div>and as such, there would be churn in the test code for each commit, and not</div><div>just incremental fixes, if you get what I mean?</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Hard to track all these offsets :-( I'm glad you've created a tool to<br>
help debugging!<br></blockquote><div><br></div><div>Yes, the parsing script has kept me sane when trying to debug these</div><div>oscillations :)</div><div><br></div><div>Regards,</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>
Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
<br>
> ---<br>
>  test/delayed_contols.cpp | 35 ++++++++++++++++++++++++++---------<br>
>  1 file changed, 26 insertions(+), 9 deletions(-)<br>
> <br>
> diff --git a/test/delayed_contols.cpp b/test/delayed_contols.cpp<br>
> index 3855eb18ecd4..c6f195b7bc7f 100644<br>
> --- a/test/delayed_contols.cpp<br>
> +++ b/test/delayed_contols.cpp<br>
> @@ -82,9 +82,13 @@ protected:<br>
>  Â  Â  Â  Â  Â  Â  Â /* Reset control to value not used in test. */<br>
>  Â  Â  Â  Â  Â  Â  Â ctrls.set(V4L2_CID_BRIGHTNESS, 1);<br>
>  Â  Â  Â  Â  Â  Â  Â dev_->setControls(&ctrls);<br>
> +  Â  Â  Â  Â  Â  Â delayed->reset();<br>
> +<br>
> +  Â  Â  Â  Â  Â  Â /* Trigger the first frame start event */<br>
> +  Â  Â  Â  Â  Â  Â delayed->applyControls(0);<br>
>  <br>
>  Â  Â  Â  Â  Â  Â  Â /* Test control without delay are set at once. */<br>
> -  Â  Â  Â  Â  Â  Â for (unsigned int i = 0; i < 100; i++) {<br>
> +  Â  Â  Â  Â  Â  Â for (unsigned int i = 1; i < 100; i++) {<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â int32_t value = 100 + i;<br>
>  <br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â ctrls.set(V4L2_CID_BRIGHTNESS, value);<br>
> @@ -122,8 +126,11 @@ protected:<br>
>  Â  Â  Â  Â  Â  Â  Â dev_->setControls(&ctrls);<br>
>  Â  Â  Â  Â  Â  Â  Â delayed->reset();<br>
>  <br>
> +  Â  Â  Â  Â  Â  Â /* Trigger the first frame start event */<br>
> +  Â  Â  Â  Â  Â  Â delayed->applyControls(0);<br>
> +<br>
>  Â  Â  Â  Â  Â  Â  Â /* Test single control with delay. */<br>
> -  Â  Â  Â  Â  Â  Â for (unsigned int i = 0; i < 100; i++) {<br>
> +  Â  Â  Â  Â  Â  Â for (unsigned int i = 1; i < 100; i++) {<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â int32_t value = 10 + i;<br>
>  <br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â ctrls.set(V4L2_CID_BRIGHTNESS, value);<br>
> @@ -150,9 +157,11 @@ protected:<br>
>  <br>
>  Â  Â  Â int dualControlsWithDelay(uint32_t startOffset)<br>
>  Â  Â  Â {<br>
> +  Â  Â  Â  Â  Â  Â static const unsigned int maxDelay = 2;<br>
> +<br>
>  Â  Â  Â  Â  Â  Â  Â std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â { V4L2_CID_BRIGHTNESS, { 1, false } },<br>
> -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â { V4L2_CID_CONTRAST, { 2, false } },<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â { V4L2_CID_CONTRAST, { maxDelay, false } },<br>
>  Â  Â  Â  Â  Â  Â  Â };<br>
>  Â  Â  Â  Â  Â  Â  Â std::unique_ptr<DelayedControls> delayed =<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â std::make_unique<DelayedControls>(dev_.get(), delays);<br>
> @@ -165,8 +174,11 @@ protected:<br>
>  Â  Â  Â  Â  Â  Â  Â dev_->setControls(&ctrls);<br>
>  Â  Â  Â  Â  Â  Â  Â delayed->reset();<br>
>  <br>
> +  Â  Â  Â  Â  Â  Â /* Trigger the first frame start event */<br>
> +  Â  Â  Â  Â  Â  Â delayed->applyControls(startOffset);<br>
> +<br>
>  Â  Â  Â  Â  Â  Â  Â /* Test dual control with delay. */<br>
> -  Â  Â  Â  Â  Â  Â for (unsigned int i = 0; i < 100; i++) {<br>
> +  Â  Â  Â  Â  Â  Â for (unsigned int i = 1; i < 100; i++) {<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â uint32_t frame = startOffset + i;<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â int32_t value = 10 + i;<br>
>  <br>
> @@ -189,7 +201,7 @@ protected:<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â return TestFail;<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â }<br>
>  <br>
> -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â expected = i < 1 ? expected : value - 1;<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â expected = i < maxDelay ? expected : value - 1;<br>
>  Â  Â  Â  Â  Â  Â  Â }<br>
>  <br>
>  Â  Â  Â  Â  Â  Â  Â return TestPass;<br>
> @@ -197,9 +209,11 @@ protected:<br>
>  <br>
>  Â  Â  Â int dualControlsMultiQueue()<br>
>  Â  Â  Â {<br>
> +  Â  Â  Â  Â  Â  Â static const unsigned int maxDelay = 2;<br>
> +<br>
>  Â  Â  Â  Â  Â  Â  Â std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â { V4L2_CID_BRIGHTNESS, { 1, false } },<br>
> -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â { V4L2_CID_CONTRAST, { 2, false } }<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â { V4L2_CID_CONTRAST, { maxDelay, false } }<br>
>  Â  Â  Â  Â  Â  Â  Â };<br>
>  Â  Â  Â  Â  Â  Â  Â std::unique_ptr<DelayedControls> delayed =<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â std::make_unique<DelayedControls>(dev_.get(), delays);<br>
> @@ -212,6 +226,9 @@ protected:<br>
>  Â  Â  Â  Â  Â  Â  Â dev_->setControls(&ctrls);<br>
>  Â  Â  Â  Â  Â  Â  Â delayed->reset();<br>
>  <br>
> +  Â  Â  Â  Â  Â  Â /* Trigger the first frame start event */<br>
> +  Â  Â  Â  Â  Â  Â delayed->applyControls(0);<br>
> +<br>
>  Â  Â  Â  Â  Â  Â  Â /*<br>
>  Â  Â  Â  Â  Â  Â  Â  * Queue all controls before any fake frame start. Note we<br>
>  Â  Â  Â  Â  Â  Â  Â  * can't queue up more then the delayed controls history size<br>
> @@ -226,8 +243,8 @@ protected:<br>
>  Â  Â  Â  Â  Â  Â  Â }<br>
>  <br>
>  Â  Â  Â  Â  Â  Â  Â /* Process all queued controls. */<br>
> -  Â  Â  Â  Â  Â  Â for (unsigned int i = 0; i < 16; i++) {<br>
> -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â int32_t value = 10 + i;<br>
> +  Â  Â  Â  Â  Â  Â for (unsigned int i = 1; i < 16; i++) {<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â int32_t value = 10 + i - 1;<br>
>  <br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â delayed->applyControls(i);<br>
>  <br>
> @@ -245,7 +262,7 @@ protected:<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â return TestFail;<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â }<br>
>  <br>
> -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â expected = i < 1 ? expected : value - 1;<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â expected = i < maxDelay ? expected : value - 1;<br>
>  Â  Â  Â  Â  Â  Â  Â }<br>
>  <br>
>  Â  Â  Â  Â  Â  Â  Â return TestPass;<br>
> <br>
<br>
-- <br>
Regards<br>
--<br>
Kieran<br>
</blockquote></div></div>