search

EdgeTX / edgetx

EdgeTX is the cutting edge open source firmware for your R/C radio
https://edgetx.org
GNU General Public License v2.0
1.58k stars 337 forks source link

Touch is accepted with the backlight off #420

Closed jmil-dev closed 2 years ago

jmil-dev commented 3 years ago

If the backlight is off a touch will turn the backlight on and be registered as an interaction with the UI.

Since the current touch screens are unreadable with the backlight off there is no way to know what you have touched.

Ideally a touch is required to turn the backlight on and then a second touch is needed to interact with the UI

gagarinlg commented 2 years ago

With PR #942 it is possible to disable the touchscreen. I would like to extend this mechanism to disable the touchscreen also for this issue. I already tried something, but since the touch start enables the screen and the touch end does the action, it did not work as intended. My idea for an implementation this:

three states for the touchscreen in libopenui:

special function from #942 sets to disabled or enabled backlight off sets the state to "delayed enable", which can be overridden by the special function backlight on sets the state to "enabled"

disabled blocks all events from being processed delayed enable waits for a touch end event and then goes to enabled enabled allows event provessing

Any opinions on this?

pfeerick commented 2 years ago

I was thinking more simply that the backlight should be turned on, and the touch event consumed?

gagarinlg commented 2 years ago

The current implementation does enable the backlight when the finger touches the screen, which is perfectly ok. But the event that is consumed by the pages/widgets is the touch end event, when the finger is removed. So when the backlight has been switched on, the touch end event would be processed. Which in effect, does not change the behaviour at all. The order would be, with the current implementation, because of the way backlight is handled:

backlight off -> touchscreen off touch start event -> backlight on -> touchscreen on touch end event -> processed by UI code

So the first touch end event after switching the backlight on has to be blocked. The backlight is not controlled by libopenui but somewhere else. Changing this would probably break things for non-touch radios, I presume. For the touch radios it might be the cleaner way, software design wise, but lots of work and also making the code harder to read, because of all the #if defined checks, to switch between classic backlight control and libopenui backlight control. So I thought it is easier to tell libopenui to block touch event handling until the next touch end to solve this. When implementing #942 I first tried blocking the touch events at the touch controller code, but then for every radio with a different touch sensor IC and the sim it is necessary to implement this again and again. So in my opinion it was the cleaner way to implement a touch event blocker in the ui code.

pfeerick commented 2 years ago

My reasoning was that touch is only part of the issue... as the same could be said for key input when the display is off... particularly enter. ;)

Waking the backlight up is the domain of resetBacklightTimeout()... and it is called by things such as the touch panel driver (before a touch event is raised) and trim and key inputs. However, because isBacklightEnabled() is rather conveniently just set to true on horus targets, rather than actually reporting the backlight state (🤦), it is not possible to do a check of the backlight state, and do a backlight enable instead of a raising a touch event...

gagarinlg commented 2 years ago

My plan was to modify the checkBacklight function in opentx.cpp, so that it returns the actual backlight state and then use this to decide whether the touch screen should be enabled or not, which should be accurate. The isBacklightEnabled() result then should not matter. Is there a good reason for this, or can the function changed to report the actual state? I also thought about changes for the keys, but this is something not just related to touch screen radios, but to all radios and is a significant change to the user interface. Also pressing keys accidentally is less likely to happen. I am willing to make the effort to also change the key behaviour, but we should discuss this with a larger audience, or at least make it configurable.

pfeerick commented 2 years ago

I would presume it was one of those "will get to someday" tasks... as it seems rather strange it is valid for most other targets, just nor horus targets, when the other backlight functions are implemented

I think it is fair to say for a colourlcd radio, that touch generally should not trigger an event if the backlight is off (other than to enable the backlight), and key inputs should not be accepted at warning screens/popups, etc if the backlight is off... but that should be already handled, or if it isn't, be using the forced backlight on mechanism.

The whole backlight mechanism in OTX/ETX is a complete mess though, especially with how the SF (via pot) interacts with it.

gagarinlg commented 2 years ago

If I understand the code correctly, warning indeed use forced acklight. Still one could argue, that a single touch on the screen should not confirm the warning, because it is to easy to accidentally touch the creen or a button.

Maybe I will take a look at all the backlight stuff and try to make a plan how to change it. Afterwards it is hopefully easier to implement a no touch/keys when backlight is off. Testing on non color/touch radios has to be done by someone else, though.

yds commented 2 years ago

With PR #942 it is possible to disable the touchscreen. I would like to extend this mechanism to disable the touchscreen also for this issue.

this has been an issue since before EdgeTX with the touch only Nirvana NV14. FWIW, since there's no possible way to interact with EdgeTX on Nirvana other than touch it would be nice to be able to wake the screen with something which does not register as an interaction with the UI. turning off the touch screen is not an option with this radio..

maybe have a corner of the screen (perhaps one or both of the bottom corners) where nothing ever registers as an interaction? but it wakes the screen from sleep and/or turns the backlight on.. :thinking:

then all we'd need is a little documentation to spread the word that there's a "safe space" on the screen to tap to wake it up without any interaction registering.

gagarinlg commented 2 years ago

This has been an issue since before EdgeTX with the touch only Nirvana NV14. FWIW, since there's no possible way to interact with EdgeTX on Nirvana other than touch it would be nice to be able to wake the screen with something which does not register as an interaction with the UI. turning off the touch screen is not an option with this radio..

maybe have a corner of the screen (perhaps one or both of the bottom corners) where nothing ever registers as an interaction? but it wakes the screen from sleep and/or turns the backlight on.. 🤔

How about using the power buttons?😉 Your idea sounds interesting, but like a lot of work. Maybe a. transparent grey overlay to mark the touch enable area, but oy shown, when touch is disabled. I will think about it.

yds commented 2 years ago

How about using the power buttons?😉

yes.. FlySky-OpenTX did wake the screen when both power buttons were pressed.. I'd go for just one power button waking the screen if that's possible.. right now EdgeTX shows the power down symbol when the power buttons are pressed on Nirvana NV14, but the screen goes back to sleep, leaving the screen awake until time out would be better.. and waking the screen with just one power button would be even better than having to press both of them..

pfeerick commented 2 years ago

Maybe a. transparent grey overlay to mark the touch enable area, but oy shown, when touch is disabled.

I think we're then back the same problem... If the backlight is off, how do you see that overlay? :laughing:

The power button could certainly be used on some radios as a backkigt timeout reset - I was surprised on the TX16S that it was only waking the backlight for the animation and then turning it off again on release, so that could be tweaked.

If you know the backlight is actually off (in other words, either query hardware state or track the actual state configured to be in as that is not currently done at all AFAICT), I think it should be a simple matter of hooking in the the two touch drivers, and doing an "if backlight off, resetbacklight timeout (thus turning it on) and don't fire touch event" to all-out touch to wake.

gagarinlg commented 2 years ago

Waking the backlight up is the domain of resetBacklightTimeout()... and it is called by things such as the touch panel driver (before a touch event is raised) and trim and key inputs. However, because isBacklightEnabled() is rather conveniently just set to true on horus targets, rather than actually reporting the backlight state (🤦), it is not possible to do a check of the backlight state, and do a backlight enable instead of a raising a touch event...

Implementing isBacklightEnabled on the horus based radios hsould be a matter of minutes, I just checked. We have to decied wheter we only want a completely disabled backlight to be considered off, or f.e. everything below 10%, or so.

Maybe a. transparent grey overlay to mark the touch enable area, but oy shown, when touch is disabled. I think we're then back the same problem... If the backlight is off, how do you see that overlay? 😆

well, we can just use it as a mark, to show the user where the wakeup area ist, it it has to be remebered, but I whink, I would prefer using the power buttons, if I would own such a radio.

If you know the backlight is actually off (in other words, either query hardware state or track the actual state configured to be in as that is not currently done at all AFAICT), I think it should be a simple matter of hooking in the the two touch drivers, and doing an "if backlight off, resetbacklight timeout (thus turning it on) and don't fire touch event" to all-out touch to wake.

I would also like to block key presses from being send to the UI. As for blocking in the touch drivers, that is so hardware implementation specific. If we find a solution at a higher abstraction level, it would just work for the next type of radio.

pfeerick commented 2 years ago

Implementing isBacklightEnabled on the horus based radios hsould be a matter of minutes, I just checked.

Indeed... that's why I am surprised it was not done, and hardcoded to true... :-/

I would only consider backlightlight off state - as anything else is user configurable and so up to them if they set the backlight low state to "too low to read". After all, the goal is to prevent input when it is impossible to see what you are pressing, isn't it? Which is because with a colourlcd radio, you cannot see anything on the screen with the backlight off, unless you have a bright flashlight and angle it off the display...

As for blocking in the touch drivers, that is so hardware implementation specific. If we find a solution at a higher abstraction level, it would just work for the next type of radio.

Since we can rely on the touch driver turning the backlight on, you could probably use the setTouchEnabled function to disable the touch panel if the backlight was off, and is being turned on. But then you would need to figure out how to then turn the touch panel back on after the backlight has been enabled (and the touch event processed and cleared).

MervDale commented 2 years ago

Just as a matter of interest I have the back light on my TX16S set up under GF on S2 . If you have the back light set to go off under the timer then operating S2 can bring it back without the need to touch the screen or any other switches, in fact you can set the backlight back with a number of options Just a thought.

pfeerick commented 2 years ago

It is definitely a thought, but it's a poor "workaround" at best ;) At the end of the day, we should not be needing volume or backlight special functions, and it should be properly configurable via the radio setup screen. And if the SF does remain, it should interact with the "on" slider (which it currently doesn't). Poor documentation has led to no-one actually knowing how the brightness SF was designed to be used - with the option set to "on" so that "off" is never used - except for anyone that read the initial pull request implementing the SF or who remembers it being added ;)

raphaelcoeffic commented 2 years ago

@pfeerick please educate us on this :-) a link would be helpful!

MervDale commented 2 years ago

Here is a trap for the unwary according to the OTX manual the Backlight Auto Off Timer should have a range of 5 -100 seconds.... not 0 to 100 seconds as the current versions of OTX and ETX have it set to. I can tell you this as it caught me out playing around with the Backlight options. the only way I could get my screen back on was through Companion!

pfeerick commented 2 years ago

@pfeerick please educate us on this :-) a link would be helpful!

And here I was thinking you would be in the "remembers it being added category" since it is actually a fairly new function (or remember another issue on this repo discussing it recently - now you'll have to find that one yourself :-P) 😁

https://github.com/opentx/opentx/pull/7680

gagarinlg commented 2 years ago

My plan was to fix this issue at the weekend, if family time and work would have allowed for it. As with the current state of the discussion and multiple obvious undesired behaviors, I will wait for a decision to be made. That said, should we make different issues for the different problems from this discussion? How is the normal procedure for decision making in this project? Does anyone know if there would be problems, when isBacklightEnabled does work properly on Horus radios, just from their head? I may just look into that as a first step to start fixing the issues.

MervDale commented 2 years ago

It seems to me that you can have the screen working as it should from what I can see from the original OTX manual, that is of course if ETX is programed the same way, correct me if I have this wrong

backlight OTX

raphaelcoeffic commented 2 years ago

There is a key difference for color displays: the backlight is basically always on. When it isn't, the screen is OFF.

@gagarinlg I think we can safely consider the backlight to be OFF when the current brightness is <= the ON brightness. I need to check what the SF does however....

raphaelcoeffic commented 2 years ago

Regarding the backlight SF, I think we should do this:

Alternatively, we can scrap the SF altogether and allow for configuring a "ON/OFF" or "forced ON" control (physical switch), and of course allow for configuring via a physical pot as listed in the last bullet point.

While we are at it: I think the backlight mode "OFF" should be removed from color lcd targets. There is no point in having it. With the backlight OFF all the time, the display is basically OFF.

pfeerick commented 2 years ago

While we are at it: I think the backlight mode "OFF" should be removed from color lcd targets. There is no point in having it. With the backlight OFF all the time, the display is basically OFF.

I think we were both reading this setting wrong... it's not off as in backlight off, but off as in delay timer off... but rather strangely, it then also triggers the backlight brightness to be set via the off slider. 😕

It's backlight delay ON (forced on, on slider), keys, controls, both (bounces between on and off brightness based on delay) and then OFF (forced on, off slider).

raphaelcoeffic commented 2 years ago

It's backlight delay ON (forced on, on slider), keys, controls, both (bounces between on and off brightness based on delay) and then OFF (forced on, off slider).

Ok, then let's call this OFF mode on color lcd "standby". Either way, I think there are too many settings and combinations.

What we need is this, IMHO:

The brightness is then regulated with:

Then we have exactly 2 states, regardless of how these are set/controlled:

gagarinlg commented 2 years ago

My backlight solution can be found here: [https://github.com/gagarinlg/edgetx] in the main branch. It does work on TX16s and should work on NV14, at least it compiles and it is not to many changes. Compilation for non color lcd/non touch radios is also OK. If this is an acceptable solution, I would create a pull request from it.

MervDale commented 2 years ago

Please don't take this as criticism but it seems to me that there would be more pressing issues that would benefit from programmers time.

To me that this does not seem to be a major issue in that I would say that the main concern of having the back light off is to conserve battery power when flying and reactivating it without affecting the touch screen.

This can all be done with the current OTX options as I see it. Please correct me f I am wrong.

I have mine set to 50 sec to off and back on to half power "approx" with keys so just a roll of the wheel on the TX16S brings it back on with the option of turning it up or down with a GF on S2.

Not being a programmer could you explain how you option works please.

gagarinlg commented 2 years ago

Feel free to say what you think. I very mich prefer an open discussion. As for the reasoning:

  1. it is a hobby, not a job, so I decide what I do with my time 😜
  2. I just started participating in this project, so I selected an issue with little to no impact on the real rc stuff, to get to know the software internals. I do dim my backlight faster, but that is a personal preference. The main concern is, that e. g. on the NV14, there is no way to activate the backlight without creating an input event, as there are no buttons. When you are in certain lighting conditions it can be very hard to discern what is on your screen and then it is helpful to not create an input event.
MervDale commented 2 years ago

By all means go for it, I have no issues with your option. I was just thinking out aloud, it seems Australian English doesn't convey the same thoughts as other versions, my bad.

gagarinlg commented 2 years ago

😂 There are a few distinct differences between german and australian English... As I said, I like an open discussion. So no offence taken.

raphaelcoeffic commented 2 years ago

Please don't take this as criticism but it seems to me that there would be more pressing issues that would benefit from programmers time.

To me that this does not seem to be a major issue in that I would say that the main concern of having the back light off is to conserve battery power when flying and reactivating it without affecting the touch screen.

This can all be done with the current OTX options as I see it. Please correct me f I am wrong.

I have mine set to 50 sec to off and back on to half power "approx" with keys so just a roll of the wheel on the TX16S brings it back on with the option of turning it up or down with a GF on S2.

Not being a programmer could you explain how you option works please.

What do you think of what is proposed here: https://github.com/EdgeTX/edgetx/issues/420#issuecomment-954643823 ?

gagarinlg commented 2 years ago

What do you think of what is proposed here: #420 (comment) ?

If I am right, this is essentially, what is there now, except for being able to set the backlight timeout to 0, which, in combination with setting the dimmed brightness to 0 leads to an unusable state. So I will change the minimum backlight timeout to 5. Also I will try to disable unused control elements, so when backlight is set to be always on, everxthing but the on brightness control element will be disabled.