[OpenWrt-Devel] [PATCH v2] gpio-button-hotplug: mind debounce interval consistently
David Bauer
mail at david-bauer.net
Sat Jul 13 13:53:05 EDT 2019
Hello Christian,
first of all, i wouldn't have imagined that there is so mush to talk about pressing buttons ;)
On 13.07.19 18:04, Christian Lamparter wrote:
> Hello David,
>
> On Wednesday, July 10, 2019 12:31:12 AM CEST David Bauer wrote:
>> thanks for your reworked patch. I think i found two bugs around the debounce
>> interval for both flavors. I'll have to admit, they are both edgecases ;)
>>
>> On 06.07.19 23:57, Christian Lamparter wrote
>>> -static void gpio_keys_polled_check_state(struct gpio_keys_button_data *bdata)
>>> +static void gpio_keys_handle_button(struct gpio_keys_button_data *bdata)
>>> {
>>> + unsigned int type = bdata->b->type ?: EV_KEY;
>>> int state = gpio_button_get_value(bdata);
>>> + unsigned long seen = jiffies;
>>>
>>> - if (state != bdata->last_state) {
>>> - unsigned int type = bdata->b->type ?: EV_KEY;
>>> + pr_debug(PFX "event type=%u, code=%u, pressed=%d\n",
>>> + type, bdata->b->code, state);
>>> +
>>> + /* is this the initialization state? */
>>> + if (bdata->last_state == -1) {
>>> + /*
>>> + * Don't advertise unpressed buttons on initialization.
>>> + * Just save their state and continue otherwise this
>>> + * can cause OpenWrt to enter failsafe.
>>> + */
>>> + if (type == EV_KEY && state == 0)
>>> + goto set_state;
>>
>> As we are calling gpio_keys_handle_button every poll-interval, we need
>> to make sure we save the initial state AFTER the button has been stable
>> for the debounce interval. For irq-based keys we get this "for free" as
>> we modify the execution timer for the irq handling function.
>
> Ah, I think due to how the patch looks (as in this mail and not applied
> to the source code) this feature/behavior got lost there.
>
> I've added the gpio_keys_handle_button() below after that version of the
> patch is applied.
>
> ---
> static void gpio_keys_handle_button(struct gpio_keys_button_data *bdata)
> {
> unsigned int type = bdata->b->type ?: EV_KEY;
> int state = gpio_button_get_value(bdata);
> unsigned long seen = jiffies;
>
> pr_debug(PFX "event type=%u, code=%u, pressed=%d\n",
> type, bdata->b->code, state);
>
> /* is this the initialization state? */
> if (bdata->last_state == -1) {
> /*
> * Don't advertise unpressed buttons on initialization.
> * Just save their state and continue otherwise this
> * can cause OpenWrt to enter failsafe.
> */
> if (type == EV_KEY && state == 0)
> goto set_state;
> /*
> * But we are very interested in pressed buttons and
> * initial switch state. These will be reported to
> * userland.
> */
> } else if (bdata->last_state == state) {
> /* reset asserted counter (only relevant for polled keys) */
> bdata->count = 0;
> return;
> }
>
> if (bdata->count < bdata->threshold) {
> bdata->count++;
> return;
> }
>
> if (bdata->seen == 0)
> bdata->seen = seen;
>
> button_hotplug_create_event(button_map[bdata->map_entry].name, type,
> (seen - bdata->seen) / HZ, state);
> bdata->seen = seen;
>
> set_state:
> bdata->last_state = state;
> bdata->count = 0;
> }
> ---
>
> The pressed button state (for the polled) version will now always have to be
> stable for the debounce-duration (including on the initial state). That's
> guaranteed by the "if (bdata->count < bdata->threshold) { count++;return; }"
> path. If there's a laps during the initial state then last_state gets set to
> 0 and the counter is reset. But this bogus "released" event is not reported
> to userspace, so if it's an inital pressed state it will still be reported
> correctly, altough it might take another full debounce-duration.
Hmm, i might have some problems grasping here, but let's take the following
situation:
goto set_state called here
|
0 5 10 15 20 25 30
| | | | | | |
1111110000000000000000000000000
Debounce: 10ms
Poll: 5ms
So, from my understanding (and to keep the behavior in line), the initial stable
state for GPIO-polled should be set to 0 at 20ms. No event should be created
for this.
With the gpio_keys_handle_button function you snipped above however, we would initially
set our state to 1 and would fire a release event at 20ms, even though this is our real
initial stable state.
failsafe is triggered on both edges, so pressed & released events would trigger it.
Or am i missing something here?
>
> For the irq-driven case, threshold is now 0 and therefore this gets skipped.
> Which should be fine since we schedule the gpio_keys_handle_button() after
> the debounce-interval so it should be stable at that point (if not then the
> debounce interval needs tweaking).
>
> So, the button behavoir should be Ok. The "switch" case is a definitely
> more lenient in that the initial state. It could jump between 0 and 1 and
> not reset. However, a "physical" switch that manages to constantly
> flip-flop during the poll interval probably has a electrical problem.
>
>>> + /*
>>> + * But we are very interested in pressed buttons and
>>> + * initial switch state. These will be reported to
>>> + * userland.
>>> + */
>>> + } else if (bdata->last_state == state) {
>>> + /* reset asserted counter (only relevant for polled keys) */
>>
>> This is also needed for irq-driven keys. If the state changes two times
>> within the debounce interval, gpio_keys_handle_button is still executed
>> and we need to terminate here. Otherwise, we would skip a "release" or
>> "push" action.
>>
>> I did rework those two parts a bit and tested it with the irq and
>> polled flavors without a problem. See below:
>>
> I don't follow there? In your v2, you have fixed the debounce-rearming
> timer for the irq-case by doing mod_delayed_work() function, which is
> correct! So, if a new event within the debounce interval, the timer
> will get rescheduled by another irq to fire after after a new
> debounce-interval passed (so it should be stable by then).
You are right. Your commit was relating to the reset of the counter, my remark was
about "There should be no events in case last_state == current_state also for irq
keys. So no functional problem to see here :)
Best wishes
David
>
> If this is still a problem on ath79, I think the requested irq trigger
> method might have something to do with this, maybe it triggers on the
> wrong level or edge? (For apm821xx, I had to change it to just falling
> edge as the IRQ controller can only do either or but not both edges).
>
> Cheers,
> Christian
>
> I guess this is becoming a moot point since we both have commit access ;)
> Put please, take your pick!
>
>
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
More information about the openwrt-devel
mailing list