lkml.org 
[lkml]   [2009]   [Dec]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] staging: s5k3e2fx.c: reduce complexity by factoring
On 12/15/2009 11:10 AM, Ray Lee wrote:
> On Tue, Dec 15, 2009 at 10:37 AM, Justin Madru<jdm64@gawab.com> wrote:
>
>> But, wouldn't you agree that if the code was suppose to deal with "rounding
>> issues" that there's a
>> simpler expression?
>>
> No, I don't agree. Five minutes of effort below shows your patch will
> generate different numbers than the original. If this is controlling a
> stepper motor trying to hit a home position, it's off now. Or, the
> errors in the expressions for moving near and far may have balanced
> each other out before, and now there may be a systematic error causing
> a skew over time toward one end rather than the other.
>
> My point is that you need to run this past the guy with the actual
> hardware who wrote it in the first place such that it can be tested,
> and make sure the slapped-together expression isn't just working by
> accident, as ugly as it might be.
>
> #include<stdio.h>
>
> typedef int int32_t;
> typedef short int16_t;
> typedef unsigned int uint32_t;
>
> enum {MOVE_NEAR, MOVE_FAR} move_direction;
>
> int32_t s5k3e2fx_move_focus(int direction, int32_t num_steps)
> {
> int32_t i;
> int16_t step_direction;
> int16_t actual_step;
> int16_t s_move[5], s_move_2[5];
> uint32_t gain, gain_2;
>
> if (direction == MOVE_NEAR)
> step_direction = 20;
> else
> step_direction = -20;
>
> actual_step = step_direction * (int16_t)num_steps;
>
> gain = actual_step * 0x400 / 5;
> gain_2 = actual_step / 5;
>
> for (i = 0; i<= 4; i++) {
> if (actual_step>= 0)
> s_move[i] = ((((i+1)*gain+0x200) -
> (i*gain+0x200))/0x400);
> else
> s_move[i] = ((((i+1)*gain-0x200) -
> (i*gain-0x200))/0x400);
> }
>
> for (i = 0; i<= 4; i++)
> s_move_2[i] = gain_2;
>
> if (memcmp(s_move, s_move_2, sizeof(s_move))) {
> printf("s1, s2 differ for direction %d, num_steps %d\n", direction,
> num_steps);
> for (i=0; i<5; i++)
> printf(" [%d] %d %d", i, s_move[i], s_move_2[i]);
> printf("\n");
> }
>
> }
>
> int main(void) {
> int steps;
> for (steps = -65535; steps< 65536; steps++) {
> s5k3e2fx_move_focus(MOVE_NEAR, steps);
> s5k3e2fx_move_focus(MOVE_FAR, steps);
> }
> }
>
>

Ok, I tested the example code and it does lead to different values!
But, I did some testing and came up with a new patch that has been tested
this time to come up with the same values as the old code but uses less
calculations.

gain = ((actual_step << 10) / 5) >> 10;
for (i = 0; i <= 4; i++)
s_move[i] = gain;

Greg, disregard my last patch. Instead, please accept my new patch --
pending review.
http://lkml.org/lkml/2009/12/15/453

Justin Madru


\
 
 \ /
  Last update: 2009-12-15 22:05    [W:0.050 / U:0.104 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site