I just commented on another PR that a long press function would be useful, and now I see this. You've been busy Riksu9000!
I'll do some testing when I get back to my desk, but it won't be for about a week.

Tested, works well, I think it could do with a settings page to choose what single/long/double press does as different people probably want different things. I think the current single and long press behaviour are ideal for me, but if the double press could be configured to launch a specific app (music, navigation, timer...) that would be amazing.

Tested, works well, I think it could do with a settings page to choose what single/long/double press does as different people probably want different things. I think the current single and long press behaviour are ideal for me, but if the double press could be configured to launch a specific app (music, navigation, timer...) that would be amazing.

Thanks for testing. The double press launching notifications is mainly just to demonstrate the ability to detect the event. Having it configurable might be a good idea, but that should be a separate PR after this.

There's also #487. We need to choose which shortcut should be used for favorite apps, or make them work together somehow.

Works perfectly. Really makes browsing menus much more pleasant! Together with #586 the wake-up on press is instant 11 ms.
Thanks.

Just noticed just a small issue. I have display timeout set to 5 seconds. When I press and hold button in different times like 2-3 seconds before screen sleep timeout, then the screen dims a bit (this is a feature few seconds before going to sleep) and then it goes to the About screen.
Sometimes screen goes off for a fraction of second, sometimes it turns off.
I guess you need to add display timeout reset to some parts. Preferably every time user just presses and holds the button.
Otherwise works perfectly.

Good find. Fixed by calling ReloadIdleTimer on press and release. It can still dim if you hold the button for more than three seconds, but I think that's acceptable as nothing happens after two seconds and this avoids calling ReloadIdleTimer continuously.

I also noticed that the way SystemTask processed the button events was a bit weird, so I moved everything to messages, which is futureproof in case we want SystemTask to process the button events in some way.

Thanks, tested and works fine.

The last improvement fd39ba3 brings a bug. The previous code worked fine without this bug.

When the watch is turned on and brightness is max, then I press button to go to sleep. But during the fading down (or maybe a while after screen is off) I press the button again. The watch freezes and resets.

Video
https://photos.app.goo.gl/fzrYU1qYeXzUXLeH8

When I moved the functions to messages, I left out a check for isGoingToSleep. Turns out it is necessary so I added it back.

It seems like wake-sleep-wake quickly (either button or double tap) misses the second wake event - if i press the button 3 times fairly quickly, the device wakes, then sleeps, but doesn't wake again. Half a second later it will wake just fine. Could this be the double press behaviour interfering with the wake behaviour?

I think that happens regardless of this PR. There is for example this code that stops execution for half a second each time the screen is turned off.

void St7789::DisplayOff() {
  WriteCommand(static_cast<uint8_t>(Commands::DisplayOff));
  nrf_delay_ms(500);
}

I think that happens regardless of this PR. There is for example this code that stops execution for half a second each time the screen is turned off.
You are correct!

It seems like wake-sleep-wake quickly (either button or double tap) misses the second wake event - if i press the button 3 times fairly quickly, the device wakes, then sleeps, but doesn't wake again. Half a second later it will wake just fine. Could this be the double press behaviour interfering with the wake behaviour?

I think that happens regardless of this PR. There is for example this code that stops execution for half a second each time the screen is turned off.

void St7789::DisplayOff() {
  WriteCommand(static_cast<uint8_t>(Commands::DisplayOff));
  nrf_delay_ms(500);
}

Why is this even doing this? This feels...wrong. does nrf_delay even yield to RreeRTOS? That feels...bad.

It seems like wake-sleep-wake quickly (either button or double tap) misses the second wake event - if i press the button 3 times fairly quickly, the device wakes, then sleeps, but doesn't wake again. Half a second later it will wake just fine. Could this be the double press behaviour interfering with the wake behaviour?

I think that happens regardless of this PR. There is for example this code that stops execution for half a second each time the screen is turned off.

void St7789::DisplayOff() {
  WriteCommand(static_cast<uint8_t>(Commands::DisplayOff));
  nrf_delay_ms(500);
}

Why is this even doing this? This feels...wrong. does nrf_delay even yield to RreeRTOS? That feels...bad.

Yeah I doubt that's necessary. Someone should look into that..

This PR is working perfectly fine and I would add it to the next release pleaase, 'cause I got used to it :)
I really like the wakeup/debounce speed improvement and doubleclick to the home screen.
(configuraton of button long press could be done i another PR).

Just an idea when I see #708 App chooser made by @coxtor.

Since we have a settings page to choose Favorite app, and this button handler will sooner or later also need some App chooser for doubleclick and looong buton press - wouldn't be the best solution to create a "shared app/settings chooser" which could be used by other apps? That way we could use single code/dialog to set:

• Favorite app
• Button doubleclick
• Button looong press

It might not display only apps but also settings. Now I'm thinking - wouldn't be possible to use current menu for apps and settings? It might work the same way, but if called with some flag, it might display some text in the top bar like "Choose a app" and when selected - it will return to previous app instead of running the app. The same may apply for Settings dialog.
All this should be done in separate PR so this one gets merged ASAP.

This might deduplicate lot of things. What do you think?

Riksu, thanks a ton for keeping these up to date with develop. Any reason for merging instead of rebasing however? We get a lot of "merge" commits this way? Really more of a curiosity. Awesome work as always!

Riksu, thanks a ton for keeping these up to date with develop. Any reason for merging instead of rebasing however? We get a lot of "merge" commits this way? Really more of a curiosity. Awesome work as always!

Well that's what GitHub and others often tell to do, so that's just what I've been doing. I can take a look at rebasing if it's important. I believe it could also just be squashed when merging as well. Any reason not to do that? I don't think there's anything too important in the history that would be lost.

Its definitely a preference argument. pesronally I don't like squashing most merges as you lose design/development intention. Which is sometimes crucial for bug hunting. As for re-basing vs merging...valid reason is valid both work fine and have the same end result! The merge commits can also be squashed or similar, just more work for the merging party where a re-base its done already. At the end of the day its really just a cleanliness thing that's up to @JF002 to tell us what we want the repository's "norm" to be. Thanks for the response :)

I'm sorry, I'm a bit late to the party!

At the end of the day its really just a cleanliness thing that's up to @JF002 to tell us what we want the repository's "norm" to be.

This is not something I had the opportunity to think about so far, but that's an interesting topic :)
Ideally, we should try to keep the history as clean and as readable as possible. When merging a feature branch into the develop branch, I can do the following (I hope I'm not wrong in my understanding of these 3 possibilities) :

• merge all changes and create a 'merge commit'. This is the easiest, but commits will be added in the history with their respective date/time, meaning that all new commits won't be added at the top of the history
• Squash and merge : a single commit is added to the history. That's clean but... we loose the whole history of the branch, which might be interesting in some cases
• Rebase: all commit are re-created at the top of develop. It's nice because all commits will be added together in develop, which makes the history more linear. However, rebasing can be very very confusing, even for me...

@Riksu9000 @geekbozu That's probably something we can talk in another 'discussion' issue, so we can try to specify how we want to merge features and manage the history :)

Now, regarding this PR : This is a really nice feature, thanks, @Riksu9000 !

However, I noticed that this PR introduces a new task to handle the button. As a task costs a bit of CPU time and RAM (320 bytes of stack in this case), are we really sure we need it? It does make the code tidy and easy to read, but I'm wondering if we shouldn't try to consider another solution that would avoid this overhead, mostly in RAM space.

I haven't dig that much into the code, but it might be possible to implement the class ButtonHandler as a "simple" class (with no task) that would be called directly from the IRQ handler). It'll probably need an additional timer (HW or FreeRTOS timer) to implement timeouts for the double push. This implementation would be a bit more complex, but knowing that we are very tight in memory, I think that would be a good idea to at least consider that option.

What's your opinion? Is there something I'm missing that makes the new task necessary?

A timer would be needed for the two levels of long press. I was concerned that it would get too complicated without a task, but with a state machine it might be manageable. It would need to respond to press, release and timer events. I think I'll just start from scratch and create a new PR so we can compare both approaches.

Hi @Riksu9000 , i think that this pull can be either closed or be put as draft since the new is now on 1.7.0.
Unless @JF002 has not yet chooses which pul to merge?

Hi @Riksu9000 , i think that this pull can be either closed or be put as draft since the new is now on 1.7.0. Unless @JF002 has not yet chooses which pul to merge?

It's likely that the new one will be merged instead of this, but both of these PRs are functional, and as one is merged, the other one will be closed.

I'm closing this one as we've just merged #782.