Re: ad714x wheel support and other shortcomings

[Michael Hennerich <michael.hennerich@analog.com>]

On 05/01/2012 05:01 PM, Jean-Francois Dagenais wrote:
> Hi guys,
> (sorry for the long message, but there are A LOT of issues here in this
> driver...)

Hi Jean-Francois,

Thanks for your detailed observations.

A few words about the history of this driver.
Bryan, not longer working for ADI, developed this driver based on
a few routines someone else in ADI developed some time ago.
He didn't had proper hardware that would have allowed him to test
all physical arrangements, such as wheels, touch-pads, etc.

When I took over ownership, I only had a board with a few buttons,
and a really tiny wheel. So testing on my side was basically limited to
the dimensions of the wheel.
I fixed a series of bugs associated with the wheel algo,
such as divide by zero, and other things.

>
> Ok, I took a step back after my failed mod
> (1335460639-1362-2-git-send-email-jeff.dagenais@gmail.com), and discovered many
> shortcomings in the driver code around the wheel feature, hw_init and more
> generically the abs_pos calculation algorithm. It looks like we're the only
> "kernel-participating" party that has tried to integrate the wheel in a real
> system...?
I know some people are using this driver successfully.
But when it comes to the wheel, that could be the case.

>
> This is sort of a story based account of my recent dealings with the ad714x
> driver, I know it's chatty, but please bear with me...
>
> The motion of the wheel near the roll around point (ex. between stages 7 and 0
> for an 8 stage wheel) has a dead zone. This is because the slices of max_coord
> being added up are too large, and near the last segment, the value is greater
> than max_coord, but is capped at max_coord, hence the dead zone. Now this
> effect, caused by the enlarged slices, is tolerable for a slider since there is
> no rolling around, but for the wheel, this is unusable.
>
> Simply shrinking the slice size didn't fix the problem, the values capped at
> max_coord before the mid-point between the last and first stages, making a dead
> zone, then a skip when the finger nears the center of start_stage. So I came up
> with a new algorithm which relocates the positioning one turn of the wheel
> ahead, then modulo's the value back into the max_coord range to eliminate this
> problem.
>
> I had to stepped away from the a_param and b_param based mean calculation
> because (and this is true for the slider as well) it has bumps in it. The bumps
> appear when the determined "highest_stage" changes. The recalculated values
> near this frontier skips ahead or backward by a noticeable amount, hence the
> "bump". It is especially annoying when you keep your finger around a tipping
> point between two stages. The value then skips by a large quantity rapidly back
> and forth. IMPORTANT NOTE: since the slider uses a similar algorithm, I tried
> telling the driver my wheel as a slider to invoke that code, and did the bump
> test there in the middle of my wheel, SAME PROBLEM!
>
> My new algo still grabs the largest response and the two adjacent stages, the
> response "floor" (or 0) is brought up to the smallest of the two adjacent
> stages. This basically eliminates one of the adjacent stages and while
> adjusting the ratio between the largest response and the next largest one. With
> these two stages left, a proportion is given to the largest vs. the other. This
> becomes a vector which offsets the coord (+/-) from the largest response
> stage's center coordinate. Ultra simple and works really well. IT COULD/SHOULD
> BE PORTED TO THE SLIDER and/or to the generic ad714x_cal_abs_pos function.
Sounds good to me.

>
> Once I got that working, I got jerky behaviour from the reported position
> around the edges (i.e. 0 and max_coord). The cause was the flt_pos calculation
> which is basically broken for circular coordinates.
>
> The problem is that when using a max_coord of 1024 for example, then coord 0
> equals coord 1024. So the abs_pos and old flt_pos have to be brought in the
> same "quadrant" (for lack of a better word) for the calculation to be valid.
> But this is still not enough for things to be smooth in the whole range of
> values.
>
> The other issue one encounters is that, even if the values are in the same
> "quadrant" and you modulo the end value, when you add several turns to the
> coordinates for the flt_pos calculation, it doesn't yield the same result as if
> you don't. My solution was to offset the abs_pos and old flt_pos around
> max_coord, make the calculation and "de-offset" the result after. This means
> the calculation is always done using the same scale (i.e. max_coord).
>
> The resulting position is regular and smooth. But then again, my abs_pos was
> fine without the flt_pos calculation. It made me wonder if the filtering, which
> is really just a time-base smoothing function, had been added because of the
> bump problem I talked about earlier. Any thoughts?
Think I changed that in commit f1e430e6369f5edac552d99bff15369ef8c6bbd2.
I did that because the flt_pos gave me better results.
Now that fixed the underlying problem, we should definitely use the abs_pos.

>
> BTW, just so it doesn't go un-noticed in my upcoming patch, while refactoring
> this, I noticed a clear bug in the current ad714x_wheel_cal_abs_pos :
> 	first_before = (sw->highest_stage + stage_num - 1) % stage_num;
> 	highest = sw->highest_stage;
> 	first_after = (sw->highest_stage + stage_num + 1) % stage_num;
> ... this will fail IF start_stage IS NOT 0 for this wheel. I have changed it to
> something like this :
> 	int highest_idx_rel = sw->highest_stage - hw->start_stage;
> 	...
> 	first_before = ((highest_idx_rel + stage_num - 1) % stage_num)
> 	                            + hw->start_stage ;
> 	...
> Agreed?
Good catch! Agreed.

>
> So now, using this strategy, the wheel motion is both precise and has no breaks
> or bumps in it. I even tested the code using stages 1..8 instead of 0..7 and it
> still works correctly. This suggests that my index calculations are ok.
>
> (patch form was too noisy, I will send a patch after I get feedback if you guys
> don't mind)
>
> static void ad714x_wheel_cal_abs_pos(struct ad714x_chip *ad714x, int idx)
> {
> 	struct ad714x_wheel_plat *hw =&ad714x->hw->wheel[idx];
> 	struct ad714x_wheel_drv *sw =&ad714x->sw->wheel[idx];
> 	int stage_num = hw->end_stage - hw->start_stage + 1;
> 	/* index of the highest stage relative to start_stage */
> 	int highest_idx_rel = sw->highest_stage - hw->start_stage;
> 	/* the number of positions between each stages */
> 	int slice_size = DIV_ROUND_CLOSEST(hw->max_coord, stage_num);
> 	int a, b, c; /* the 3 vals to consider */
> 	int dir; /* direction of the adjustment from the highest stage pos */
>
> 	/* Init abs_pos at the highest stage's physical location, but one turn
> 	 * of the wheel ahead (modulo'd later down), then add half the slice
> 	 * size because we want coordinate 0 to be half way between end_stage
> 	 * and start_stage.
> 	 */
> 	sw->abs_pos = (slice_size * highest_idx_rel)
> 		       + hw->max_coord + (slice_size/2);
>
> 	/* grab the three values we are interested in. These are the highest
> 	 * index, and the one before and after, in a circular roll-over type
> 	 * increment and decrement, also considering start_stage != 0.
> 	 */
> 	a = ad714x->sensor_val[((highest_idx_rel + stage_num - 1) % stage_num)
> 			       + hw->start_stage];
> 	b = ad714x->sensor_val[sw->highest_stage];
> 	c = ad714x->sensor_val[((highest_idx_rel + stage_num + 1) % stage_num)
> 			       + hw->start_stage];
>
> 	/* eliminate the smallest val from the equation, by substracting the
> 	 * smallest to all values, in other words, bring the signal reference
> 	 * up to the smallest value of the 3. After this "if-else", 'bM is
> 	 * still the highest val, 'a' contains the second biggest val, and
> 	 * 'dir' contains a record of the direction we need to adjust abs_pos.
> 	 *        : .                          . :
> 	 *      : : :                          : : :
> 	 *  if: a b c  adjust right (1), else: a b c adjust left (-1)
> 	 *
> 	 */
> 	if(a<  c) {
> 		c -= a;
> 		b -= a;
> 		a = c;
> 		dir = 1;
> 	} else {
> 		a -= c;
> 		b -= c;
> 		dir = -1;
> 	}
> 	/* add/substract a proportional to a/a+b quantity to abs_pos */
> 	sw->abs_pos = (sw->abs_pos +
> 		       DIV_ROUND_CLOSEST(a * dir * slice_size, a+b)) %
> 		       hw->max_coord;
> }
>
> static void ad714x_wheel_cal_flt_pos(struct ad714x_chip *ad714x, int idx)
> {
> 	struct ad714x_wheel_plat *hw =&ad714x->hw->wheel[idx];
> 	struct ad714x_wheel_drv *sw =&ad714x->sw->wheel[idx];
> 	int half_coord_range = hw->max_coord/2;
> 	int abs_pos = sw->abs_pos;
> 	int diff = sw->abs_pos - sw->flt_pos;
>
> 	/* try to put both pos within max_coord/2 of each other by adding
> 	 * one turn of the wheel, this turn is removed by modulo after calc.
> 	 */
> 	if (diff>  half_coord_range)
> 		sw->flt_pos += hw->max_coord;
> 	else if (diff<  -half_coord_range)
> 		abs_pos += hw->max_coord;
>
> 	/* if difference is still too great, just use abs_pos */
> 	if (abs(abs_pos - sw->flt_pos)>  half_coord_range)
> 		sw->flt_pos = sw->abs_pos;
> 	else {
> 		/* for the filter to work without "breakage" around the wheel,
> 		 * we need to offset the values to bring the two values around
> 		 * max_coord. Pretend the old flt_pos is max_coord.
> 		 */
> 		diff = hw->max_coord - sw->flt_pos;
> 		abs_pos += diff;
>
> 		sw->flt_pos = (DIV_ROUND_CLOSEST(((hw->max_coord * 30) +
> 						 (abs_pos * 71)), 100) - diff)
> 			       % hw->max_coord;
> 	}
> }
>
>
>
> Alright, while I have your attention... some more questions:
>
> In hw_init, why do we read back all the sys registers but do nothing with the
> data?
There are a few registers that are read-to-clear.
But these shouldn't have any side effects.
Dead code - feel free to remove it.

>
> Also, a few lines further in hw_init:
> ad714x->write(ad714x, AD714X_STG_CAL_EN_REG, 0xFFF);
> ...which completely disregards the settings provided by platform init
> (ad714x->hw->sys_cfg_reg[1]) which are programmed a few lines before for
> nothing basically. I can understand that the driver could "hard-code" the
> calib_en feature for it's behaviour, but writing 0xfff overwrites AVG_F/LP_SKIP
> registers to 0. Since the settings are provided by platform, I would just
> delete the line that does this , and trust the platform to init those properly,
> it is already responsible for writing most of the registers anyway.
Sounds good to me.

>
> Another weird thing is the presence of the 3 STAGE_(LOW/HIGH/COMP)_INT_ENABLE
> registers in the platform init structure, even though the driver specifically
> overwrites those in the ad714x_use_(com/thr)_int functions. I would shrink the
> platform data's sys_cfg_reg array to 5 since these last three registers are
> under the control of the driver, and the other configuration item in these 3
> regs is the GPIO feature, which is not useable by the current driver code
> anyway.
You're right for the sliders and wheels.
Setup routines for these will do a read modify write on affected registers.
However the buttons still need to have a proper config...
>
>
> Thanks for reading through!


-- 
Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368;
Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin,
Margaret Seif