Re: ad714x wheel support and other shortcomings

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

On 05/03/2012 06:39 PM, Jean-Francois Dagenais wrote:
> On May 3, 2012, at 10:10, 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;
>>
> While working on this...
> Weird! where are the touchpad Y axis CDC vals read from the chip???
Good catch -
Looks like another bug. As I said the touchpad as far as I know is pretty
untested due to deficient test hardware.
> Anyway, my planned mod would fix this.
>> 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@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