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

[WIP] New Edit Screen for TFT_COLOR_UI + TOUCH #25649

Draft
wants to merge 16 commits into
base: bugfix-2.1.x
Choose a base branch
from

Conversation

lukasradek
Copy link
Contributor

@lukasradek lukasradek commented Apr 7, 2023

Description

While the clever people implement cutting edge features into Marlin, UI is sometimes being left behind.
So that's where the dumb folks step in... and so I decided to improve Edit Screens a bit 😉.

It is still work in progress but the main UI goals are:

  1. Give more context - show min and max allowed values to hint the range of slider (before it was very hard to pick a spot on the slider to click since there was no sense of (absolute) scale.
  2. Give better controls over the input process. Now you can choose the step size of the increment / decrement buttons, click the range limits to quickly set the value to min or max... and... drum roll ... I have added a keypad, so the user can type the value straight in.

I also added a few codebase goals after I looked into the code:

  1. Remove duplication - there is a lot of duplicate code between the files for different resolutions, which can be easily reduced and the UI can be made more "responsive" instead of hardcoded.
  2. Make it a bit easier to develop for COLOR_UI and touchscreens - Adding CALLBACK type touch control (there was actually a todo for something like that in the code 🙂) and adding some higher level methods to draw nicer buttons.

As mentioned... this is a Work In Progress, the code is messy, there are a few bugs (that I know of) and as of posting this it works only on 320x240 landscape.

Feedback is welcome!

Requirements & Config

TFT_COLOR_UI - 320x240 Landscape
Touchscreen

Other than that, standard config will suffice.

Benefits

Better usability. A bit nicer.

Eye candy

(although a bit sour)

Fully implemented increment edit screen
Basic implementation of keypad using numbers to store the value
Keypad is full featured with some bugs, which are commented in code
Only for 320x240 Landscape so far
@lukasradek lukasradek marked this pull request as draft April 7, 2023 19:38
@lukasradek lukasradek force-pushed the Edit-screen-with-more-controls branch from f1cab04 to 9b92dfa Compare April 7, 2023 21:27
@thisiskeithb
Copy link
Member

Can you make this change optional?

The current method allows you to drag a slider to easily change temperature or fan speed (along with a ➕ & ➖and a ✔️ to confirm for finer adjustments) which already works really well, even on a 3.5” TFT.

@lukasradek
Copy link
Contributor Author

lukasradek commented Apr 8, 2023

It is fully backward compatible, that was my main goal. I like the slider as well.

The issue I had with it was that it was hard to touch the right spot - because there was no scale and because the granularity is simply not high enough. So ➕➖ adjustment was always needed one unit at a time.
This is mostly remedied with my version.

The slider still works as before, but on top of that there is min/max to show scale and allow one touch min/max values.
➕ & ➖ still work as before. The "single unit" step will always be there + options to increment by multiples.
✔️ is just "OK" now.

So from usability standpoint there should be no reason to prefer the current version.
But of course it can be optional 🙂.

@thisiskeithb
Copy link
Member

It is fully backward compatible, that was my main goal.

There is no config option in this PR to disable these new changes.

The issue I had with it was that it was hard to touch the right spot

Have you tried tapping + holding + dragging the slider? This will allow you to set your desired temperature accurately.

So from usability standpoint there should be no reason to prefer the current version.

I prefer the old/current version because it's a cleaner UI for those screens (hence the ask to make this change optional).

@lukasradek
Copy link
Contributor Author

lukasradek commented Apr 8, 2023

There is no config option in this PR to disable these new changes.

Fully backward compatible feature-wise. However as mentioned, it can be easily adapted to be configurable. I am not overriding the original code (ui.encoderPosition mechanism for example) but building on top of it mostly.

Have you tried tapping + holding + dragging the slider? This will allow you to set your desired temperature accurately.

Sure, I tried and I am using it that way. But on 3D printers, touchscreens are usually low quaility (resistive, small,..)... meaning the finger doesn't slide that well, the screen / Marlin also isn't that responsive either (ie. vs. phones) and in case of hotend temp, there is 280 (or more) units crammed into like 6 centimeters of the slider (5 per milimeter).
So yes... it is usable, but I wouldn't call it user friendly or accurate on my screen.

I have the 3.5" (so you can be sure I will test it properly for those sizes 🙂) and my current process is.

  1. Open edit menu
  2. Tap (and hold) to see some reference value
  3. Drag in the general direction and stop somewhere close (+- 15 units)
  4. Fine tune using ➕➖
    (I found it is faster for me to adjust even 15 units by tapping rather than trying to nail it more closely with slider and then fine tune anyway)
  5. Confirm

The upgrades to the slider screen make it a bit easier and precise and with
keypad screen, you can

  1. Open edit menu
  2. Tap "digit count" times
  3. Confirm

In the same way setting min or max value just using the slider also isn't that easy since you would have to tap the exact edge of the touch area.

I prefer the old/current version because it's a cleaner UI for those screens (hence the ask to make this change optional).

I look at it as a tool, so usable is more important than clean for me... but that sure can be up to the user, how he configures it 🙂.
I am also gonna try to polish it a bit more.

@thinkyhead thinkyhead force-pushed the bugfix-2.1.x branch 3 times, most recently from de391db to 0f34163 Compare April 12, 2023 05:14
@jmz52
Copy link
Contributor

jmz52 commented May 4, 2023

In the same way setting min or max value just using the slider also isn't that easy since you would have to tap the exact edge of the touch area.

Actually setting min or max value is super easy as you do not need to tap at the exact edge location.
Min and max values are selected with a single swipe - touch the slider, drag it to the edge and release.
Works like a charm even on 2.4" screens.

@lukasradek
Copy link
Contributor Author

lukasradek commented May 4, 2023

First of all... is there a need to raise counterarguments. Are there any downsides to my solution or are we just argumenting for no reason? 🙂
And second... the touch poll rate during the swipe is not high enough to register fast swipes smoothly. So yes, I can slow swipe to the min/max, but no, I cannot reliably fast swipe. Either way... single tap si faster than both cases.

I have come from the default TronXY chitu firmware, which is a far cry from Marlin. But there were some things I really miss in COLOR_UI. One of them being the feature that you can click the edit item icon (e.g. heatbed) and it will reset to 0 or set to reasonable value (70 in case of heatbed). Simple and elegant way to quickly switch to practical values.

@thisiskeithb
Copy link
Member

First of all... is there a need to raise counterarguments. Are there any downsides to my solution or are we just argumenting for no reason? 🙂

Yes. I had a basic request to make your changes optional since the current temperature settings menu has a simplified UI and tapping + dragging works well, even on 2.4" screens as jmz52 stated.

@lukasradek
Copy link
Contributor Author

lukasradek commented May 4, 2023

But of course it can be optional 🙂.

And I have replied you will have it 😉.

@jmz52
Copy link
Contributor

jmz52 commented May 5, 2023

First of all... is there a need to raise counterarguments. Are there any downsides to my solution or are we just argumenting for no reason? 🙂

It was an explanation how the slider works. Slider was specifically designed in a way that allows reliably set min and max values by dragging slider over the edge.

As for feedback:

  • Touch screen for Color UI is optional. UI can be used with encoder alone, encoder and touch screen or touch screen only. You need to make sure that your new screen can correctly handle the encoder.
  • There are 3 different fonts available in Marlin, make sure your UI looks good with Unifont (smaller font that is not so flash hungry)
  • I am not sure that proposed precision is practical - are there a real-life use cases when someone need to set temperature with fraction of degree precision?

@EvilGremlin
Copy link
Contributor

For me, only issue with slider is that tick is a bit too fast, i always overshoot couple degrees while holding buttons.
Sub-degree precision might be useful on bioprinters i guess.

int8_t intEnd = 7;
uint8_t dig = 0;
uint32_t div = 1;
for (;intEnd >= 0 && (((float)intVal / div) >= 1 || ((float)decimal / div) >= 1);intEnd--) {
Copy link
Member

@thinkyhead thinkyhead May 11, 2023

Choose a reason for hiding this comment

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

Save a few cycles by skipping cast and division since…

  • ((float)intVal / div) >= 1 == (intVal >= div)
  • ((float)decimal / div) >= 1 == (decimal >= div)

@lukasradek
Copy link
Contributor Author

  • Touch screen for Color UI is optional. UI can be used with encoder alone, encoder and touch screen or touch screen only. You need to make sure that your new screen can correctly handle the encoder.

As I mentioned above.

It is fully backward compatible, that was my main goal.

Furthermore I guess this UI will be turned on by a flag in Configuration(_adv).h that will be inside ENABLED(TOUCH) block.

  • There are 3 different fonts available in Marlin, make sure your UI looks good with Unifont (smaller font that is not so flash hungry)

I do not anticipate a problem, but I will make sure it works.

  • I am not sure that proposed precision is practical - are there a real-life use cases when someone need to set temperature with fraction of degree precision?

There is no proposed change in precision. The precision is strictly given by the EditItem class by its value type.

@vlsi
Copy link
Contributor

vlsi commented Sep 4, 2023

Min and max values are selected with a single swipe - touch the slider, drag it to the edge and release

@jmz52 , It would be helpful to display min, and max values anyway. The min and max could be made clickable, so users could select "100% fan" in a single click rather than unreliable swiping.

Could you please list more cases when you think sliders are helpful?
I guess pre-defined buttons could completely replace sliders providing better experience.
By default the firmware could sugoest min and max buttons while they could be customized on the case by case basis.

For instance:

  • "0% fan", "50% fan", "100% fan" buttons could be reasonably big, so they are easy to hit
  • "hotend temperature" rarely needs to be configured for "max", however, having "pla", "petg", "recently used" and similar buttons would be helpful
  • "stepper current" rarely needs min or max. It might need adjustments in +- 0.1A or +-0.01A
  • "extruder steps per mm" does not benefit from slider at all. It is tuned based on the measurements, so a keyboard would be better there.

@lukasradek , have you considered implementing several +- buttons like in #26230 ? I guess it would make tuning easier: +-1, +-0.1, ... would be a single click only, and users won't need to remember if they are in "+-10" or "+-1" mode.

Have you considered adding "cancel" button, so users could cancel the edit without saving changes?

@lukasradek
Copy link
Contributor Author

Hello @vlsi ,

yes, I agree with most of your suggestions and they have been considered.
Some of them are being (have been) implemented, some are not because of the way how Edit Screens are implemented - it would require a lot of work in some cases (eg. the per-digit increment buttons).

@thinkyhead thinkyhead force-pushed the Edit-screen-with-more-controls branch 2 times, most recently from 1bfb3ab to 85b1bf9 Compare September 28, 2024 16:45
@thinkyhead
Copy link
Member

I did my best to adapt the code to the updated unified Color UI layout, but a few things have changed in our methods, so new keypad button code will be needed. The bonus is that the new code can apply to all screen sizes.

Note that for larger screen sizes we could implement G-code entry and other fancy key inputs.

Touch behavior could use some adjustment. Touches on screens that have persistent sensing should highlight a button/menu item, and then only activate when the touch ends. This could help prevent hitting nearby items by mistake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants