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

Separate system event generated when setup button is double-clicked #818

Merged
merged 4 commits into from Jan 20, 2016

Conversation

@sergeuz
Copy link
Member

commented Jan 17, 2016

The patch introduces separate system event generated when setup button is double-clicked, as well as test application handling regular and double clicks.

@sergeuz

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2016

I've tested this patch on Electron and setup button seem to work as expected, i.e. it causes RSSI to be displayed on single click and powers down Electron on double click.

@technobly

This comment has been minimized.

Copy link
Member

commented Jan 18, 2016

@sergeuz have you tested this with the Photon? Wiring_SetupButtonUX was originally in place to prevent this code from causing the Photon to respond to SETUP button single/double clicks.

Eventually we would like to create a user API for enabling single/double click functionality of the SETUP button on the Photon/P1 (disabled by default), but enable it by default for the Electron (with the ability to disable it). I could see having the ability to take over the system action by hooking into the system event. So for instance on the Photon I might not want to show RSSI with a single click, but want to setup a quick user function based on single clicks. This sounds like a separate issue/PR though.

@sergeuz

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2016

@technobly RSSI and power off features are still guarded by Wiring_SetupButtonUX and thus are disabled on Photon. I only reused existent timing code to generate button_double_click event for user applications.

Totally agree that built-in button behavior should be customizable eventually.

@m-mcgowan m-mcgowan added this to the 0.4.9 milestone Jan 18, 2016

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2016

This looks good! So, to recap we had the button_status event, which can be used to determine how long the button has been pressed for, and then we've added the double click event. I'm thinking it makes sense to also add a single click event, to make it easy for application code to detect a single click. Thoughts?

@technobly

This comment has been minimized.

Copy link
Member

commented Jan 18, 2016

@sergeuz ah ok that sounds good. Just wanted to make sure this didn't cause any issues with users who might already be looking at the SETUP button for short presses.

@m-mcgowan yes that sounds great, in the back of my mind I thought you had added single click events in the past, but since you are asking I guess not ;-)

@sergeuz

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2016

Personally, I'm kinda opposed to the idea of dedicated single click event, because it's impossible to say whether single click is happened or not before timeout for a second click is elapsed. Practically it means that single click events will be always delayed up to a second according to current code. I guess GUI frameworks like Qt don't have separate events for single clicks due to exactly same reason.

Anyway, it's easy to add such event to this PR as well, so let me know :)

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2016

That's fair reasoning.

To it seems we have these choices:

  • make single/double click mutually exclusive - requiring a delay before reporting a single slick to rule out double click
  • make them inclusive: always report a single click, and then a double click.

If the application is designed such that the action of a single click naturally comes before the action for a double click, then the 2nd one makes sense. This is how Windows works - the action of the single click is safe to perform before the double click.

But how about we support both methods, by having 3 events: click, single-click and double-click:

  • a click is any press/release of the button: applications listen for this if they don't care about the number of clicks. It's reported as soon as the button is released.
  • a single click specifically means only one click with no possibility of a double click. It is sent with a delay after the device has determined a double click can't occur.
  • similarly, a double click means the button was clicked twice in succession. no single click event is fired.
@technobly

This comment has been minimized.

Copy link
Member

commented Jan 18, 2016

The single click even can be generated in the same place the code uses to determine it was a single and not a double. There is a delay because of this, but it will be a unique event:
https://github.com/sergeuz/firmware/blob/button/system/src/main.cpp#L477-L482

This gives users a dedicated single and double click event with a minimal amount of code.

I also like the idea of a separate click event (press/release) would be nice and will require the user to create their own debouncing routines. It would allow someone to create an event driven ClickButton library using the SETUP button (which would be pretty sweet).

@sergeuz

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2016

@m-mcgowan Well, as long as possible delay in click-related events is tolerated, why not to generalize your approach to "unlimited" number of clicks? I mean our users are likely to have completely independent functions in their single/double click handlers, because it's a nice and simple way to communicate with the device without additional hardware and the cloud. So why limit them with just two clicks? :)

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2016

Sure, I can see the number of clicks being a parameter of the event. So we have "click", which is sent with each click, and we have "clicks" (insert better name here!) for N clicks in succession where N>=1

@technobly

This comment has been minimized.

Copy link
Member

commented Jan 18, 2016

I think we are starting to talk about several different things now. One is because we are already "decoding" single and double clicks for a user, it's easy to generate that event and easy for a user to write a small amount of code to use it.

The other is the same button_click event could come with data that contains the number of clicks (1 or 2 is all we are currently decoding).

I don't think it's appropriate to report any more than 2 currently because we are not "decoding" it, i.e., not properly debounced.

It also might be worth revisiting the system single/double click handling and seeing if the ClickButton library could be adapted to make it have less of a delay for single button clicks.

@sergeuz

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2016

Sure, that's just for the sake of discussion. It's certainly not critical to have highly advanced button subsystem right now :)

So what's our decision? I like the approach that Mat has described previously. My only concern here is that having several different click-related events, where one is delayed and another is not, can be confusing for a user. But if it's ok, I can add necessary events to this PR easily.

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2016

We can do both of these things - both publish click events (with a click count) as the clicks are happening, and also at the end post a click event with the final click count.

Applications then subscribe to the event that makes sense for their use case.

In the interest of reducing the number of event types (we have a limit of 64), we can encode all of this in one event. The parameter passed to the event handler encodes the number of clicks, and if this was the final click count or not.

Later, a future API will make this much simpler with Button objects and the like.

@sergeuz

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2016

So, basically, for the "advanced" approach we're introducing two events: button_click and, let's say, button_multiple_click. Both carry number of clicks as their data, but while button_click is generated instantly for every click in series, button_multiple_click is generated only once at the end of the series (via timeout).

If above is an acceptable API for future use, we can also quickly update current code to generate button_click and button_multiple_click only for 1 and 2 clicks for now, and get back to proper support for multiple clicks later, if it's not a good idea to work on this feature now.

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2016

Agreed. Let's call the 2nd event button_final_click since it's sent as the last event after one or more clicks. (I suggest this to replace button_multiple_click since the word multiple implies there are always more than one click, which isn't always the case. Also I think the name helps reflect the semantics that it's sent after the preceding button_click events.)

@sergeuz sergeuz force-pushed the sergeuz:button branch from 59a5261 to c68af84 Jan 19, 2016

@sergeuz

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2016

Added support for multiple click events to this PR. Changes are quite straightforward and everything seems to work as expected on both Photon and Electron according to my tests. tests/app/button_event test application is updated as well.

@sergeuz

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2016

Forgot to mention: the implementation has a limit for maximum number of clicks in series (5 clicks currently): https://github.com/sergeuz/firmware/blob/button/system/src/main.cpp#L206, so that at 5th click button_final_click is fired instantly.

reset_button_click(release_time);
system_handle_double_click();
}
if (depressed_duration < 500) { // a short button press

This comment has been minimized.

Copy link
@technobly

technobly Jan 19, 2016

Member

Looks great @sergeuz thank you for simplifying this. I tested the PR and it works well. It is rare but possible to get a SETUP button press to "bounce" and cause a soft power down or potentially 3-5 clicks, when you only wanted RSSI. (for that matter, getter a larger number of clicks than intended). To mitigate this we could add a lower bounds to the depressed_duration. I played with 30ms which is almost impossible to manually push the button faster than, but at the same time provides a fairly decent debounce time.
if (depressed_duration > 30 && depressed_duration < 500) { // a short button press
If the press is outside this range (30-500), it is ignored. Thoughts?

This comment has been minimized.

Copy link
@sergeuz

sergeuz Jan 20, 2016

Author Member

Currently, any click longer than 500 ms breaks the "multi-click" sequence and button_final_click gets fired instantly. Should the same logic apply to clicks below 30 ms, or it's better to ignore such clicks entirely?

@technobly

This comment has been minimized.

Copy link
Member

commented Jan 20, 2016

Tested on Photon and Electron running tinker and the test app TEST=app/button_event. Looks good!

technobly added a commit that referenced this pull request Jan 20, 2016
Merge pull request #818 from sergeuz/button
Separate system event generated when setup button is double-clicked

@technobly technobly merged commit 365564b into particle-iot:develop Jan 20, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.