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

Allow TOUCH_BUTTONS to be swapped #15100

Conversation

robbycandra
Copy link
Contributor

@robbycandra robbycandra commented Aug 30, 2019

For Prusa Model Printer, then
The Toolhead will move UP when EN_B ("+" Button Pressed)

For Hypercube Printer, then
Bed will move DOWN when EN_B ("+" Button Pressed).

Therefore the UP and DOWN sign at touch button can make user confused.
The Solution is to enable REVERSE MENU DIRECTION and change the Button Icon accordingly.


We also need to seperate Encoder Handling that processed at update_buttons(). Therefore the process should be done before processing input from touch screen.

@thinkyhead
Copy link
Member

I prefer narratives over proses.

@thinkyhead thinkyhead force-pushed the Reverse_Menu_Direction_for_TFT_Touch_Button branch from fbddcbc to 30372c7 Compare August 31, 2019 00:28
@thinkyhead thinkyhead changed the title [TFT TOUCH]: Add Option to Reverse Menu Encoder, better touch screen proses. Allow TOUCH_BUTTONS to be swapped Aug 31, 2019
@thinkyhead thinkyhead force-pushed the Reverse_Menu_Direction_for_TFT_Touch_Button branch 2 times, most recently from a44cb4e to 7e85a68 Compare August 31, 2019 02:31
@thinkyhead thinkyhead force-pushed the Reverse_Menu_Direction_for_TFT_Touch_Button branch from 7e85a68 to a3a2d94 Compare August 31, 2019 02:38
#endif
if (touch_arrows) {
next_button_update_ms = ms + 50; // Set delay for repeat
encoderDiff = (ENCODER_STEPS_PER_MENU_ITEM) * (ENCODER_PULSES_PER_STEP) * encoderDirection;
Copy link
Member

Choose a reason for hiding this comment

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

This value should be different when navigating a menu versus when editing a value. But perhaps on the touch display the "pulses per step" is always set to 1…?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm… By default the ENCODER_PULSES_PER_STEP falls back to 5. If the TOUCH_BUTTONS are the only encoder, then this can be set to 1 to slightly simplify the code. Is it possible for TOUCH_BUTTONS to exist alongside a regular click-wheel encoder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thinkyhead , I propose this code. The reason is to prepare full area touchscreen

Copy link
Member

Choose a reason for hiding this comment

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

A full touch-screen is going to need an entirely different approach and won't go through this "button" nonsense. Sorry about trouncing BTN_D. An oversight.

Copy link
Member

Choose a reason for hiding this comment

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

At present the vertical touch coordinate alone isn't going to very useful since we have a variety of screen layouts in Marlin. Every single screen layout type will need to have its own method to interpret touches and pass them on to the correct handlers. In cases where we only have a touch screen and no other buttons, we don't need to have up/down buttons on the screen unless something will actually be moved up/down. For menus, it will be better to have square buttons instead of textual menu items where possible, and textual menu items will need to be spaced apart more so that touches are less likely to invoke the wrong item. It will be better to write a new handler for these events rather than keep hacking everything on top of the rotary encoder.

Copy link
Contributor

@tpruvot tpruvot Sep 1, 2019

Choose a reason for hiding this comment

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

sorry, i had the "slider" picture in my head, the component name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

very good yep, but as i said before, this is not the upscale dogm anymore...

Copy link
Member

Choose a reason for hiding this comment

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

I like the bare-bones text in a simple line-box for the menu items. Color-coding by type is also cool, where yellow might mean "editable," and so on. If there were more buttons than could fit on the screen, it would get a pager, I suppose. But the nice thing about such a bare-bones approach is that it may encourage people to want to make it more interesting. But can it do all the languages?

Choose a reason for hiding this comment

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

https://youtu.be/ClhTLofXOnA Check this out.

Hi Can you provide the config files for MKS robin TFT

@thinkyhead
Copy link
Member

I've simplified the code so it passes the EN_C bit on to the buttons variable for standard handling while adding a separate flag to indicate an arrow is being held down and that it should be repeated if it stays held down. The new flag costs a byte SRAM, but I threw away the touch_buttons global to recover the byte.

@thinkyhead thinkyhead force-pushed the Reverse_Menu_Direction_for_TFT_Touch_Button branch from 913f9d1 to ee4b53c Compare September 1, 2019 02:29
@robbycandra
Copy link
Contributor Author

I think return Value from touch.read_button should be coordinate only, to make the code universal for all type of display mode.

then we make a function in MarlinUI that determine what button is touched in that position.
The function will be writen in ultralcd_something.cpp.

@tpruvot
Copy link
Contributor

tpruvot commented Sep 1, 2019

try to git diff tpruvot/master Marlin/src/feature/touch/

I had to add this function for the calibration "app", basically :

+bool XPT2046::getTouchPoint(uint16_t &x, uint16_t &y)
+{
+  if (!isTouched()) return false;
+  x = getInTouch(XPT2046_X);
+  y = getInTouch(XPT2046_Y);
+  if (!isTouched()) return false;
+
+  return true;
+}

@tpruvot
Copy link
Contributor

tpruvot commented Sep 1, 2019

Else, just tested the PR as one commit, and the behavior is fully reversed..

Menus are reversed and buttons - + do the opposite as what they should do... for values

To be short: cant be worst misconfigured, reversed arrows position and also -/+

@robbycandra
Copy link
Contributor Author

robbycandra commented Sep 2, 2019

Its working on my MKS Robin mini. But the code is a mesh now.

@tpruvot , can you message me on discord ?

@tpruvot
Copy link
Contributor

tpruvot commented Sep 2, 2019

if i (only) set REVERSE_ENCODER_DIRECTION, arrows are ok, but values editing are reversed.

Also, im not sure its related, but the ok/menu button often go down now... i need more tests without the commit and/or double check the default X calibration..

@tpruvot
Copy link
Contributor

tpruvot commented Sep 2, 2019

Fix required to set "minus" negative by default, fix both cases.. but remains the OK problem, only the OK seems to have EN_B position conflict on the touchscreen

diff --git a/Marlin/src/lcd/ultralcd.cpp b/Marlin/src/lcd/ultralcd.cpp
index 98f15fb07..85541af07 100644
--- a/Marlin/src/lcd/ultralcd.cpp
+++ b/Marlin/src/lcd/ultralcd.cpp
@@ -791,7 +791,7 @@ void MarlinUI::update() {
           buttons |= (touch_buttons & (EN_C | EN_D));   // Pass on Click and Back buttons
           if (touch_buttons & (EN_A | EN_B)) {          // A and/or B button?
             encoderDiff = (ENCODER_STEPS_PER_MENU_ITEM) * (ENCODER_PULSES_PER_STEP) * encoderDirection;
-            if (touch_buttons & EN_B) encoderDiff *= -1;
+            if (touch_buttons & EN_A) encoderDiff *= -1;
             next_button_update_ms = ms + 50;            // Assume the repeat delay

@robbycandra
Copy link
Contributor Author

@tpruvot, change

-            if (touch_buttons & EN_B) encoderDiff *= -1;
+            if (touch_buttons & EN_A) encoderDiff *= -1;

Have same effect with REVERSE_ENCODER_DIRECTION, cause it just switch between EN_A and EN_B

our LCD touch work with REVERSE_ENCODER_DIRECTION enabled (=marlin default)
REVERSE_MENU_DIRECTION can be enabled or disabled depend on our need.

I test, the code is ok now. Except you need to switch from
' X , - , + , Ent ' ----> ' X , + , - , Ent '

@tpruvot
Copy link
Contributor

tpruvot commented Sep 3, 2019

i dont want a + on the left, its fully illogic

tpruvot added a commit to tpruvot/Marlin that referenced this pull request Sep 24, 2019
The new changes in Marlin make the touchscreen erratic.
Keep the "inverted" arrows for REVERSE_MENU_DIRECTION

Partially reverts/fix commit 6b05d5d.

- STD_ENCODER_PULSES_PER_STEP 1 make weird menu "rollback" on "untouch"
- touch_buttons variable seems required to avoid bad points for now..
tpruvot added a commit to tpruvot/Marlin that referenced this pull request Dec 2, 2019
The new changes in Marlin make the touchscreen erratic.
Keep the "inverted" arrows for REVERSE_MENU_DIRECTION

Partially reverts/fix commit 6b05d5d.

- STD_ENCODER_PULSES_PER_STEP 1 make weird menu "rollback" on "untouch"
- touch_buttons variable seems required to avoid bad points for now..

+ handle arrows in priority
may avoid some bad clicks, but doesnt really change things..
tpruvot added a commit to tpruvot/Marlin that referenced this pull request Dec 4, 2019
The new changes in Marlin make the touchscreen erratic.
Keep the "inverted" arrows for REVERSE_MENU_DIRECTION

Partially reverts/fix commit 6b05d5d.

- STD_ENCODER_PULSES_PER_STEP 1 make weird menu "rollback" on "untouch"
- touch_buttons variable seems required to avoid bad points for now..

+ handle arrows in priority
may avoid some bad clicks, but doesnt really change things..
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants