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

add basic button support #164

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bushmango
Copy link

I added basic support for handling three of the four buttons when the watch is in Watchface mode.

This can be an example for others, or I'm happy to modify to make it part of the core base, as it is nice to use the buttons to change settings on some watchfaces.

Enjoy! Thanks for the fun product.

@synthead
Copy link

synthead commented May 12, 2022

Oh hey interesting! It looks like we were both thinking the same thing! I also just submitted #165 😄

I think your PR has an interesting implementation, because it lives alongside core functionality without overriding anything. Ultimately, both changes can be merged without conflicts, which would give users the option to override specific buttons, or the implementation as a whole.

How would you feel about also adding a fourth button function, and putting showMenu(menuIndex, false); in it by default? That way, the user could even conditionally override the default behavior of pulling up the menu, if they wanted.

Also, how would you feel about following the existing function names a little closer, and calling them something like Watchy::handleButton1Press?

@synthead
Copy link

Also, how about following the button numbers in the Getting Started doc?

image

@bushmango
Copy link
Author

bushmango commented May 12, 2022 via email

Copy link

@synthead synthead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding the aforementioned suggestions as a review 👍

It does make sense to let the user override the menu even if normally they
shouldn't 😛

A relatable example for this could be SW1's "confirm" functionality. If a watch face has a menu of its own (i.e. themes or backgrounds), not being able to hit the same button to confirm the menu selection could be a UX smell. Additionally, if the user presses the "confirm" button thinking that it would confirm a menu (as the doc states), and a second menu gets brought up without "closing" the first one, they might get lost in what just happened.

@@ -114,6 +114,7 @@ void Watchy::handleButtonPress() {
} else if (guiState == FW_UPDATE_STATE) {
showMenu(menuIndex, false); // exit to menu if already in app
} else if (guiState == WATCHFACE_STATE) {
button1();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about:

Suggested change
button1();
handleButton2Press();

This lines up with the function naming pattern that already exists, handleButtonPress.

Also, this is SW2 on the PCB, and here is SW2 documented in the Getting Started guide: https://watchy.sqfmi.com/docs/getting-started/

image

@@ -126,6 +127,7 @@ void Watchy::handleButtonPress() {
}
showMenu(menuIndex, true);
} else if (guiState == WATCHFACE_STATE) {
button2();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
button2();
handleButton3Press();

Same deal, with this one being SW3:

image

@@ -138,6 +140,7 @@ void Watchy::handleButtonPress() {
}
showMenu(menuIndex, true);
} else if (guiState == WATCHFACE_STATE) {
button3();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
button3();
handleButton4Press();

And this one 🙂

image

Comment on lines +585 to +595
void Watchy::button1()
{
}
void Watchy::button2()
{
}
void Watchy::button3()
{
}


Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will help keep the style consistent:

Suggested change
void Watchy::button1()
{
}
void Watchy::button2()
{
}
void Watchy::button3()
{
}
void Watchy::handleButton2Press() {}
void Watchy::handleButton3Press() {}
void Watchy::handleButton4Press() {}

Comment on lines +76 to +78
virtual void button1(); // override these methods to handle different non-menu button presses
virtual void button2();
virtual void button3();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
virtual void button1(); // override these methods to handle different non-menu button presses
virtual void button2();
virtual void button3();
virtual void handleButton2Press(); // override these methods to handle different non-menu button presses
virtual void handleButton3Press();
virtual void handleButton4Press();

@aneeshdurg
Copy link
Contributor

aneeshdurg commented May 14, 2022

This is my 2 cents - maybe instead of referrng to buttons by numbers in the functions names we could refer to buttons by their default functionality? e.g. handleButtonBackPress, handleButtonMenuPress, handleButtonUp/DownPress

Really like this approach though! ❤️

@JollyWizard
Copy link

This is my 2 cents - maybe instead of referrng to buttons by numbers in the functions names we could refer to buttons by their default functionality? e.g. handleButtonBackPress, handleButtonMenuPress, handleButtonUp/DownPress

Really like this approach though! heart

This would create confusion if the dev/user wanted to swap the handedness of the watch. It would be more appropriate to use a clear coordinate naming system, such as handleButtonUpperLeft, handleButtonUpperRight, handleButtonLowerLeft, handleButtonLowerRight.

This also creates a coherency issue with vertical flips, but is tied semantically to the physical orientation of the device, without making unnecessary assumptions about use case.

@edent
Copy link

edent commented Jul 8, 2023

Is it possible to use this to update the watch face? I've updated the .cpp and .h file with the handleButton2Press(); etc code. If I add something like this to my code:

 void handleButton2Press(){
          pinMode(VIB_MOTOR_PIN, OUTPUT);
          digitalWrite(VIB_MOTOR_PIN, true);
          delay(150); 
          digitalWrite(VIB_MOTOR_PIN, false);
}

Then the Watchy vibrates when I press the button. But how can I get the display to change when a button is pressed?

Code like this doesn't work:

void handleButton4Press(){ 
          display.fillScreen(GxEPD_WHITE);
          display.setTextColor(GxEPD_BLACK);
          display.setFont(&latin8pt7b);

          display.setCursor(0, 10);
          display.println("Button 4 Pressed!");
}

Is there something else I need to do to get the screen to update?

@naturecodevoid
Copy link

naturecodevoid commented Nov 15, 2023

@edent judging from this line of code:

https://github.com/sqfmi/Watchy/blob/master/src/Watchy.cpp#L610

display.display() (WatchyDisplay::refresh()?) needs to be called for the display to update

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

Successfully merging this pull request may close these issues.

None yet

6 participants