[BUG] Can't calibrate touch screen

[X-Ryl669]

Bug Description

Since ea37161, if you select TOUCH_SCREEN_CALIBRATION without specifying the value for X, Y, and offset, you can't get out of the calibration screen. If you specify a X,Y and offset, you are still presented a calibration screen upon bootup.

What happens is the following:

1. In class TouchCalibration, the member static touch_calibration_t calibration; is initialized to 0 since there is no constructor calling reset or a static initializer.
2. The UI asks for the need_calibration() which always returns true since the values are 0
3. The calibration procedure is done
4. Let's say the user fails the calibration (I'm failing 80% of the time), we reach this code:

  else {
    calibration_state = CALIBRATION_FAIL;
    calibration_reset();
  }

5. calibration_reset() puts 0 in the X/Y and offsets, and the state is set to fail
6. My UI shows a button "Return" but I obviously can't click it since the touch point coordinates will be multiplied by 0 (what I have in calibration.X and calibration.Y after reset) in the raw X/Y to position X/Y conversion.

How to fix it

First, the static touch_calibration_t calibration should be initialized to { TOUCH_SCREEN_X, ... } in the cpp file (like it's done in reset). This will fix the calibration menu from appearing when values are set in configuration.
Second, upon failure of calibration:

1. Either default meaningful values should be saved instead of a reset to 0 (such as X = 1<<16, Y = 1<<16, OFFSET = 0). This is the most logical action to do, since this would allow to add the calibration menu to the setting menu of the printer. In the future, calibration should be saved in/loaded from EEPROM.
2. Either the state should be reverted to TOPLEFT to trigger calibration again until it succeed. Doing so means updating all UI to let the user retry (lot of work?) or simply adding a timer to revert to TOPLEFT after a fail
3. Either store the calibration success in its own member (called calibration_precise) and only use the state as a variable tracking "none, top-left, bottom left, bottom-right, top right" so that in case of failure of the calibration, still compute the calibration's coefficient (but don't mark it as precise) and use that anyway. A bad calibration is better than a non functioning printer.

Configuration Files

I'm using a MKS Robin Nano 35 board, the configuration is the default one with TOUCH_SCREEN_CALIBRATION enable but no value set up (for bug 1) and with default values set up for _X, _Y, _OFFSETs (for bug 2).

Expected behavior:

Calibration should work and not be asked at each boot.

Actual behavior:

Calibration menu is displayed at each boot (whether or not I have enabled default value), and if I fail to calibrate correctly, the printer is struck.

[rhapsodyv]

Please follow the issue template and attach your configs.

[rhapsodyv]

1. If you fail to calibrate on boot, you can send M995 to run calibration again or restart the printer. I don't see any issue here.

2. Are you storing the values on EEPROM after you boot with a successful calibration, either by M500 or using screen menus?
   I just did a test, and the issue you are reporting only happens when I do not store my settings after a successful calibration.

So I can't reproduce your issue.

The issue template is for that: I don't know what are your configs and I don't know what you are doing, because there're no steps described in how to reproduce the bug.

[X-Ryl669]

Here it is, although for this bug, they would be useless since it is an error in the code logic.
Marlin.zip

Sorry to be harsh, but I did explain the steps that the code is doing. I'm not doing anything, yet it happens on start everytime. I know I can bypass them via the serial link, but since I have a TFT screen, I'm not using the serial port for controlling the printer. And without a serial port, the printer is useless (it's struck and can't be unlocked, unless restarting the printer, which is a critical bug IMHO). Since I fail the calibration 4 times out of 5, it's a PITA.

To reply to your points:

1. Even if I did not fail to calibrate, the calibration screen appear at each boot (while I did set TOUCH_SCREEN_CALIBRATION and both TOUCH_X_CALIBRATION / Y and OFFSET). It should not happen in that case IMHO.
2. No, I don't store the value in EEPROM since, again, I don't have a serial port connected (I expect the users not to be aware of requiring to send a M500 after touch calibration, or, said differently, why doesn't it store the calibration by itself?)

I think there are many "magic" commands that are not clear and there is no comment in the code whatsoever to explain them.
In all cases, I still think the logic of the code is broken somehow, and as such should be fixed.

Take the EEPROM out of the equation here (and/or the serial port), and you'll see that the printer is useless with the code as-is.

[tpruvot]

add M995 in custom menus. else, there was a commit pushed today to slow down a bit the calibration procedure with DOGM.. if you use this TFT_CLASSIC_UI see PR #20454

[rhapsodyv]

Well... here we go:

Here it is, although for this bug, they would be useless since it is an error in the code logic.
Marlin.zip

1. We have 3 UI that support touch screen calibration. Without your configs, I can't know what you are using, to test it.
2. Without your step-by-step, I don't know if you are missing some needed step, as, for example, save the values in EEPROM.

It's not useless. It's needed for most of the cases. This is the reason we have an issue template and ask users to follow it.

To reply to your points:
Even if I did not fail to calibrate, the calibration screen appear at each boot (while I did set TOUCH_SCREEN_CALIBRATION > and both TOUCH_X_CALIBRATION / Y and OFFSET). It should not happen in that case IMHO.
No, I don't store the value in EEPROM since, again, I don't have a serial port connected (I expect the users not to be aware of requiring to send a M500 after touch calibration, or, said differently, why doesn't it store the calibration by itself?)

You are using LVGL UI, just go to "Settings -> Eeprom Set -> Store settings to EEPROM". It will save the calibration values to EEPROM, and you problem will be gone.

Since I fail the calibration 4 times out of 5, it's a PITA.

It's because of the precision factor we set. We can make it less precise, but I think don't worth.

I still think the logic of the code is broken somehow, and as such should be fixed.

The logic is not broken. You are doing wrong assumptions and wrong tests.

Take the EEPROM out of the equation here (and/or the serial port), and you'll see that the printer is useless with the code as-is.

If you don't have EEPROM to save calibration values, it will ask you every time to calibrate your screen if you don't have it set in your config!! Because without these values, the touch will not work... Adding "such as X = 1<<16, Y = 1<<16, OFFSET = 0" is useless.

I'm closing this, because it's not a but at all, as the user said that he isn't saying the calibration data to EEPROM.

[X-Ryl669]

You are using LVGL UI, just go to "Settings -> Eeprom Set -> Store settings to EEPROM". It will save the calibration values to EEPROM, and you problem will be gone.

How do you go to the menu if you can't click on anything ?

The logic is not broken. You are doing wrong assumptions and wrong tests.

I'm powering on the printer, and the printers locks up. There is a button "Return" on screen and clicking it does not work.
I don't know about your assumptions, but honestly, they are bad.

Please note that I'm pretty stubborn so I'm doing the calibration multiple time by restarting the printer, but many users will not.
I had the TOUCH_SCREEN_CALIBRATION setting on my previous build (2.0.6) and I haven't changed anything. Simply updating to 2.0.7 and it breaks now, at least my assumptions are that I should work.

Also, if you have some requirement, please write them somewhere (for example on screen "You must save the calibration to EEPROM now" if the code does not do it by itself).

In the end, it's not user friendly at all IMHO.

[rhapsodyv]

You are using LVGL UI, just go to "Settings -> Eeprom Set -> Store settings to EEPROM". It will save the calibration values to EEPROM, and you problem will be gone.

How do you go to the menu if you can't click on anything ?

How on earth you want that your touch screen works when touch calibration fails??

I said that you must save to EEPROM when you have a SUCCESSFUL calibration!

I had the TOUCH_SCREEN_CALIBRATION setting on my previous build (2.0.6) and I haven't changed anything. Simply updating to 2.0.7 and it breaks now, at least my assumptions are that I should work.

There's NO TOUCH_SCREEN_CALIBRATION FOR LVGL BEFORE that commit you pointed out. . Before that, only COLOR UI had touch calibration screen.

The only think that existed was: XPT calibration values. Now we changed those name. If you have valid configuration, just update the name of the configs and it will work.

Also, if you have some requirement, please write them somewhere (for example on screen "You must save the calibration to EEPROM now" if the code does not do it by itself).

In the end, it's not user friendly at all IMHO.

No marlin config does auto save, that I'm aware of. And a lot of other configurations don't say you need to save it after changing: z offset, abl, ubl, steps/mm.... None that I'm aware of. Because it's something very obvious: when you change some config, it should be stored somewhere... if not, it will be lost in the next boot...

But ok, we can make our documentation better, so we will avoid having to say users that when configs are changed, they need to be saved.

[sjasonsmith]

I think there are some good points made here. Unfortunately it turned into arguing rather than looking for opportunities to make Marlin better.

First, I'd like to address this response to requesting configs

Here it is, although for this bug, they would be useless since it is an error in the code logic.

Configuration files are never useless. There are basically infinite ways to configure Marlin. We don't ask for configuration files to find bugs in your configs, it is to help understand how you are using Marlin. I have suggested we automatically close issues without configuration files, @rhapsodyv was patient enough to ask you for them.

Second, I think is probably room for improvement in these routines. It seems they work as currently designed, but we can probably do better.

We should consider the experience of a user who has no serial connection no debugging skills. When they turn on their printer, what is their experience? We should be able to come up with some minor changes to the procedure to make this better.
Ideas for improvement might be:

1. Suggest on calibration screen to use a stylus instead of finger
2. Auto-retry if calibration fails
3. Prompt to store to EEPROM after calibration

I am not re-opening this issue. @rhapsodyv is the expert on all things TFT. I have never even plugged mine in. I respect his judgement on this topic. I hope that both of you can set the initial frustration with each other aside and focus on making Marlin better rather than "being right".

[rhapsodyv]

We can open a FR for those 3 points.

[X-Ryl669]

How on earth you want that your touch screen works when touch calibration fails??
I said that you must save to EEPROM when you have a SUCCESSFUL calibration!

Yes, you are right. The issue is when there is no valid EEPROM calibration data yet, but there are calibration data in Configuration.h built up.

You'll get a "calibration screen" that you didn't have with previous version every printer boot (unless you save the EEPROM, but since it's hard to get right, you'll have it at least 5 times) although the calibration in Configuration.h is perfectly working, it's just not used at all.

Either one should remove the definition TOUCH_SCREEN_X etc.. in Configuration.h for those (but it's sad, since in 99% of the case, the default value are correct), or should fix the code to fallback to those upon failing to calibrate.

I understood your point, but as a user, it's not user friendly. I'll hack something now, and ask Victor to comment if he thinks it's better.

[X-Ryl669]

For me, just having a static initialization for static touch_calibration_t calibration set to the Configuration.h's user provided values would solve half of the issue. If the user provides default value, then the touch screen calibration menu will not pop up and it's correct. If the user wants the calibration to pop up, for now, she can remove the pre-calibration in her conf file.

In the future, maybe a menu in the setting could be used.

In order to solve the second half of the issue, would be to retrigger the calibration if it fails. I think it perfectly feasible since the UI reacts to the current calibration step, so instead of going to CALIBRATION_FAIL state, just going back to CALIBRATION_TOPLEFT would probably work without touching a single line of UI code. We could add a counter and if it fails for ... 5 times ?... we go to CALIBRATION_FAIL definitively.

What do you think?

[sjasonsmith]

Those values should be initialized in the following code. It is not static initialization, but should still run before anything is usable.

void Touch::init() {
  TERN_(TOUCH_SCREEN_CALIBRATION, touch_calibration.calibration_reset());
  reset();
  io.Init();
  enable();
}

[X-Ryl669]

Strange. It does not work for me.

[sjasonsmith]

You may need to re-initialize your EEPROM. Perhaps it has zero values saved in it.

[X-Ryl669]

Probably a clue. I did not change the EEPROM while upgrading the version. I don't see anything in the code validating the data it reads from EEPROM, except for the EEPROM version. What is the init order ? EEPROM.load then Touch::init() or the opposite ?

[X-Ryl669]

There is something I don't understand. MarlinCore contains:

SETUP_RUN(settings.first_load());   // Load data from EEPROM if available (or use defaults)
                                      // This also updates variables in the planner, elsewhere

  #if HAS_ETHERNET
    SETUP_RUN(ethernet.init());
  #endif

  #if HAS_TOUCH_BUTTONS
    SETUP_RUN(touch.init());
  #endif

If the settings are loaded before touch::init and touch::init calls unconditionally touch_calibration.calib_reset that's using the configuration from Configuration.h, then EEPROM values are never used (good or bad), right ?
How come the calibration screen appears then ?

[rhapsodyv]

  static void calibration_reset() { calibration = { TOUCH_CALIBRATION_X, TOUCH_CALIBRATION_Y, TOUCH_OFFSET_X, TOUCH_OFFSET_Y, TOUCH_ORIENTATION }; }
  static bool need_calibration() { return !calibration.offset_x && !calibration.offset_y && !calibration.x && !calibration.y; }

1. The code reset to the user config, not to zero. Zero is only used to calibrate the screen.
2. The touch calibration screen will show on boot ONLY if ALL values are zero.
3. On calibration FAILED, the code calls calibration_reset(), which will set the user configured values!! So it don't keep the printer in a wrong state. Touch keep working.

Tests I did:

1. TOUCH_SCREEN_CALIBRATION + without touch calibration configs: touch screen calibration showed on boot, I calibrated, and saved to eeprom. No more touch calibration screen were showed on boot. If I reset EEPROM with M502, touch calibration screen is showed on boot again.

2. TOUCH_SCREEN_CALIBRATION + touch calibration configs: touch screen calibration DIDN'T showed on boot. If reset EEPROM with M502, the touch calibration screen keep don't showing on boot, as it uses default configs. Them I sent M995 and intentionally failed to calibrate. The touch kept working, because it loaded my config values.

I don't know what is happening with your touch. I guess you have some dirty on EEPROM, or have changed the code.. or maybe a bad rebase. I don't know, but I can't find any bug or issue with touch calibration, besides the improvements @sjasonsmith suggested.

[X-Ryl669]

Ok, it's clear now, there is this:

#if ENABLED(TOUCH_SCREEN) && !HAS_GRAPHICAL_TFT
  #undef TOUCH_SCREEN
  #if !HAS_TFT_LVGL_UI
    #define HAS_TOUCH_BUTTONS 1
  #endif
#endif

So HAS_TOUCH_BUTTONS is not defined, so touch.init is not called and thus the calibration state is never initialized:

(gdb) p TouchCalibration::calibration      
$9 = {x = 0, y = 0, offset_x = 0, offset_y = 0, orientation = 0 '\000'}

[X-Ryl669]

I'm using non modified bugfix-2.0.x branch right now

[X-Ryl669]

Eh eh. Look here:

Dev/Marlin/Marlin/src/lcd/extui/lib/mks_ui $ rg "touch;"
tft_lvgl_configuration.cpp
48:XPT2046 touch;

So it seems mks_ui does not use Touch class but the old XPT2046 class.

[rhapsodyv]

HAS_TOUCH_BUTTONS is specific to CLASSIC_UI. It's enabled the touch buttons bar bellow the screen. It's only used there.

[X-Ryl669]

Ah. Right, so how is the touch_calibration initialized then in that case?

[rhapsodyv]

For LVGL? Only on load EEPROM_SETTINGS. It will reset defaults or load the values from EEPROM.

[X-Ryl669]

But it does not (see the GDB output above)
I've checked XPT2046 code, and it does not call touch_calibration::reset, see:

void XPT2046::Init() {
  SET_INPUT(TOUCH_MISO_PIN);
  SET_OUTPUT(TOUCH_MOSI_PIN);
  SET_OUTPUT(TOUCH_SCK_PIN);
  OUT_WRITE(TOUCH_CS_PIN, HIGH);

  #if PIN_EXISTS(TOUCH_INT)
    // Optional Pendrive interrupt pin
    SET_INPUT(TOUCH_INT_PIN);
  #endif

  TERN_(TOUCH_BUTTONS_HW_SPI, touch_spi_init(SPI_SPEED_6));

  // Read once to enable pendrive status pin
  getRawData(XPT2046_X);
}

[rhapsodyv]

Look for TOUCH_SCREEN_CALIBRATION in Marlin/src/module/settings.cpp.

[rhapsodyv]

Did you reset your eeprom? All points that you have zeroes on your EEPROM.

[X-Ryl669]

No, and I can't since I can't pass to the main menu... Argh...

[X-Ryl669]

It should load the values from Configuration.h if they are not found.

[X-Ryl669]

What about changing this line:

touch_calibration_t TouchCalibration::calibration;

to this:

touch_calibration_t TouchCalibration::calibration = { TOUCH_CALIBRATION_X, TOUCH_CALIBRATION_Y, TOUCH_OFFSET_X, TOUCH_OFFSET_Y, TOUCH_ORIENTATION };

At least I'll have some values from my config file.

[rhapsodyv]

If you fail to calibrate, it will load the values from your config.

To reset using serial, you can: M502 + M500.

To reset automatically on boot, just increment the EEPROM_VERSION on Marlin/src/module/settings.cpp

[X-Ryl669]

I think I've made some progress:
This code in Conditionals_LCD.h:

// XPT2046_** Compatibility
#if !(defined(TOUCH_CALIBRATION_X) || defined(TOUCH_CALIBRATION_Y) || defined(TOUCH_OFFSET_X) || defined(TOUCH_OFFSET_Y) || defined(TOUCH_ORIENTATION))
  #if defined(XPT2046_X_CALIBRATION) && defined(XPT2046_Y_CALIBRATION) && defined(XPT2046_X_OFFSET) && defined(XPT2046_Y_OFFSET)
    #define TOUCH_CALIBRATION_X  XPT2046_X_CALIBRATION
    #define TOUCH_CALIBRATION_Y  XPT2046_Y_CALIBRATION
    #define TOUCH_OFFSET_X       XPT2046_X_OFFSET
    #define TOUCH_OFFSET_Y       XPT2046_Y_OFFSET
    #define TOUCH_ORIENTATION    TOUCH_LANDSCAPE
  #else
    #define TOUCH_CALIBRATION_X  0
    #define TOUCH_CALIBRATION_Y  0
    #define TOUCH_OFFSET_X       0
    #define TOUCH_OFFSET_Y       0
    #define TOUCH_ORIENTATION    TOUCH_ORIENTATION_NONE
#error here // <== Added by me
  #endif
#endif

breaks compilation. Thus it's clearing my Configuration.h value. Looking at them, I think I've spotted the issue:

#define TOUCH_SCREEN
#if ENABLED(TOUCH_SCREEN)
  #define BUTTON_DELAY_EDIT  75 // (ms) Button repeat delay for edit screens
  #define BUTTON_DELAY_MENU 200 // (ms) Button repeat delay for menus

  #define TOUCH_SCREEN_CALIBRATION

  #define TOUCH_X_CALIBRATION 12316
  #define TOUCH_Y_CALIBRATION -8981
  #define TOUCH_X_OFFSET        -43
  #define TOUCH_Y_OFFSET        257
#endif

It's TOUCH_CALIBRATION_X and not TOUCH_X_CALIBRATION !

[X-Ryl669]

Indeed, it works once fixed with my change (see patch above)

[X-Ryl669]

If you fail to calibrate, it will load the values from your config.

You are right.
But the values from my config were wrong.

[X-Ryl669]

Now I understand the code's logic why it should have worked with a failed calibration (and why I wasn't able to reach the menu since mine were all 0). In the good case, you can still use the default calib.

In my case (or if someone does not provide a TOUCH_CALIBRATION_X value), it's not possible to retry calibrating since you can't click on the button.

[X-Ryl669]

What about this change:

index e4ad8f215..973b0e528 100644
--- a/Marlin/src/lcd/tft_io/touch_calibration.cpp
+++ b/Marlin/src/lcd/tft_io/touch_calibration.cpp
@@ -44,6 +44,7 @@ void TouchCalibration::validate_calibration() {
   else {
     calibration_state = CALIBRATION_FAIL;
     calibration_reset();
+    if (need_calibration()) calibration_state = CALIBRATION_TOP_LEFT;
   }
 
   if (calibration_state == CALIBRATION_SUCCESS) {

[X-Ryl669]

Another bug: If you select "Revert settings to factory defaults" in the EEPROM menu, and confirm (I guess it's erasing EEPROM), the touch screen does not work anymore and checking the calibration value gives this in GDB:

p TouchCalibration::calibration                                         
$12 = {x = 0, y = 0, offset_x = 0, offset_y = 0, orientation = 0 '\000'}

So it's nuked and the touch screen becomes unusable.

[X-Ryl669]

I think it is still a major bug. It should not clear the current calibration while erasing EEPROM else you can't do anything in the menu anymore.

[sjasonsmith]

It sounds like you are still using configuration files without defaults properly set in them. This should restore your default values.

[X-Ryl669]

If it's an error not to have default properties in the Configuration file, then it should be reported as so. Currently, without such properties, you can have situations where the printer appears locked:

1. Upon upgrade of Marlin (so the EEPROM is erased) and first boot if you fail to calibrate (you can reboot the printer to resume)
2. Or if selecting "Restore to factory default" in the EEPROM menu (for most user, they'll report that the printer is broken)

If you do have default properties in Conf file, the case 2 still happens, since the EEPROM restore will load a 0 based calibration values immediately making your touch screen unusable. In that case, the default are not loaded back in the calibration value until you reboot and are presented the touch screen calibration menu again.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.