Re: [PATCH] staging: s5k3e2fx.c: reduce complexity by factoring

[Justin Madru <jdm64@gawab.com>]

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