[OpenWrt-Devel] [PATCH v2] gpio-button-hotplug: mind debounce interval consistently

[Christian Lamparter]

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. 

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

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