Following up...
I intend to send patches soon regarding this.
I am reviewing my earlier patch about a divide by 0 caused by h_state being set,
but CDC results not reflecting this...
Here's a prelude question: Is there a reason (other than historical) why the
slider's cal_sensor_val is the only one not checking the ambient value and
substracting it? Like so:
if (ad714x->adc_reg[i] > ad714x->amb_reg[i])
ad714x->sensor_val[i] =
ad714x->adc_reg[i] - ad714x->amb_reg[i];
else
ad714x->sensor_val[i] = 0;
I want to make a different fix for this divide by 0, to kill two birds with one
stone, it would also bring the chip more in sync when we get an interrupt.
Basically, upon interrupt, I would stop conversions using power_mode bits, then
read all the state registers in one swift move regardless if its a wheel, slider
etc. All used stages would be read and ambient adjusted as a pre-step to running
the state machines. When all are done, I would reset conversion back to 0, then
re-enable conversion as it was prior to the ISR beginning.
This would produce an accurate and consistent state of all the registers that are
read, as well as reducing the unnecessarily high interrupt frequency which causes
a rather high CPU utilization when the wheel is touched.
Thoughts?
Thanks in advance for the answer and opinion!
/jfd
On May 2, 2012, at 5:06, Michael Hennerich wrote:
> 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@xxxxxxxxx), 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
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
