Re: ad714x wheel support and other shortcomings

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

On 05/03/2012 04:10 PM, Jean-Francois Dagenais wrote:
> 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;
No - I can't think of a reason why to treat them differently

>
>
> 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?
Sounds like a good improvement.
>
> 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@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
>>
>>
>


-- 
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