Messages in this thread | | | Date | Tue, 15 Dec 2009 12:57:45 -0800 | From | Justin Madru <> | Subject | Re: [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
| |