Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle button with a state machine #268

Closed
szszszsz opened this issue Aug 13, 2019 · 7 comments
Closed

Handle button with a state machine #268

szszszsz opened this issue Aug 13, 2019 · 7 comments

Comments

@szszszsz
Copy link
Contributor

Hi!
I have noticed issues #264 and #211. For the touch button handling we are using a state machine to make the state description more systematic. It should work for your case as well. Would you be interested in merging that?

@conorpp
Copy link
Member

conorpp commented Aug 13, 2019

The current code on Solo is compatible with touch sensing (used for Somu) and it detects which to use at boot. Happy to accept any improvements.

750ms seems really long for button to be pressed, and certainly too long for a tactile button.

@szszszsz
Copy link
Contributor Author

Of course period can be easily changed. The improvement is, that device does not hold the execution during that period, and replies without delay about user not being present to the host while pressing (which is a problem in Solo, unless the implementation has changed). It also has the 'consume' protection, which does not accept the following requests, until user releases the button, and presses again.
Anyway, please close if you are not interested.

@conorpp
Copy link
Member

conorpp commented Aug 13, 2019

with #264, this is the current behavior for U2F (except for the consume protection). The behavior is all in one short function and while it could be clean up a bit, I think it's nice and simple. Adding the consume protection would be good though.

For FIDO2, execution is held for UP and HID status updates are sent in parallel. I don't think returning quickly and waiting for the platform to poll works.

@conorpp
Copy link
Member

conorpp commented Aug 15, 2019

I just refactored the button press to be a bit cleaner and:

  • 1 press (activate + release) permits only one request
  • Works with capacitive touch and dome.
  • Permits request for a two second window (alleviates platforms that do not poll quickly).

The dome and capacitive touch state are interrupt driven and are polled to make sure they are each released before a request is permitted.

    // If button was pressed within last [2] seconds, succeed.
    if (__last_button_press_time && (millis() - __last_button_press_time < 2000))
    {
        goto done;
    }

    // Set LED status and wait.
    led_rgb(0xff3520);

    // Block and wait for some time.
    wait_for_button_activate(up_delay);
    wait_for_button_release(up_delay);

    // If button was pressed within last [2] seconds, succeed.
    if (__last_button_press_time && (millis() - __last_button_press_time < 2000))
    {
        goto done;
    }

    // fail
    return 0;


done:
    ret = wait_for_button_release(up_delay);
    __last_button_press_time = 0;
    return 1;

Tested on Solo + Somu

@szszszsz Do you see any potential issues?

@szszszsz
Copy link
Contributor Author

szszszsz commented Aug 16, 2019

Looks good after a brief look. I have not seen the implementation in the detail yet, so this might be inaccurate (could you link commit/file here please? or is it merged already?). Depending on the wait_for_button_release behavior there might be an issue:

  • if it blocks forever, it suspends the device's response to the platform (does not seem to be the case, since it has time as an argument);
  • if it blocks only for a specific short period, the solution will not protect user from further confirmations without his knowledge (e.g. in the event of the blocked, constant button press) after the period passes - I do not see any state handling regarding this case in the attached listing. It seems consumption is not handled correctly (for the press time longer than up_delay). Execution as well seems to be hold for at least up_delay time.

Could you elaborate, how have you handled the latter one? For reference, it is handled in the state machine I have attached originally.

Edit: to make it work, the up_delay would have to be > 2000, is that the case?

@conorpp
Copy link
Member

conorpp commented Aug 16, 2019

For the button, it is activated by a (debounced) rising edge interrupt. So if the button is held down for a long time, no request will be consumed unless another one comes when the button is finally released. So it's still only 1 consumed.

For the capacitive touch, it is level sensitive and has the problem you point out. I believe I can add a bit more code to make it edge sensitive which should solve it.

@conorpp
Copy link
Member

conorpp commented Aug 23, 2019

Edge trigger to touch sensing added. Let me know if you find other issues or enhancements.

@conorpp conorpp closed this as completed Aug 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants