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

[LUA] Touch events missing in action #1426

Closed JimB40 closed 2 years ago

JimB40 commented 2 years ago

Testing new TouchUI lib I've observed som erratic behavoiur so started to make some tests.

When MCU is working harder EVTTOUCH XXX sequence starts to mis-behave Don't know how it's done (events queue?) but when there is FIRST without TAP or BREAK following, it's hard to code anything else than simple tap.

Vid showing llost events https://youtu.be/eknqViEHlc4

Depending on app state I've observed also

Here is LUA test script (use throttle to control iterations / increase workload): TEtest.lua.zip

General information

gagarinlg commented 2 years ago

There is no touch event queue. The touch drivers write to a struct and this is polled by the UI. When the UI is not quick enough, events get lost. Before the last changes, it was even worse as there was just one shared struct between the touch drivers and libopenui.

JimB40 commented 2 years ago

One more bug only on NV14 EVT_TOUCH_BREAK is not generated at all if EVT_TOUCH_SLIDE was generated previously.

To reproduce:

@pfeerick are you capable of checking this bug? On TX16S it works. Maybe some typo in code? Would be good to have it done in 2.6 to publish TBS AL for NV14 :)

gagarinlg commented 2 years ago

Sadly the touch drivers are completely different between NV14 and TX16S. I am looking into it.

rotorman commented 2 years ago

I do not have NV14 to test, but IMHO one could replace https://github.com/EdgeTX/edgetx/blob/8150b00823ca0a37ade72c41bb2fa35cbe31e594/radio/src/targets/nv14/touch_driver.cpp#L479-L500 with https://github.com/EdgeTX/edgetx/blob/8150b00823ca0a37ade72c41bb2fa35cbe31e594/radio/src/targets/horus/tp_gt911.cpp#L766-L799 (plus minor adjustments and additions to make it work).

gagarinlg commented 2 years ago

You are probably right. While looking into this I already fixed some problems and I hope that I did not break something else. I'll continue tomorrow

jfrickmann commented 2 years ago

Lua does buffer key and touch events, because the Lua task does not necessarily run every time the GUI does. Currently, the buffer size is set to 2, and I can easily increase it.

Just keep the following two things in mind:

Having said all of that, how about I double the buffer size to 4?

gagarinlg commented 2 years ago

Please try how it behaves with my changes from today. If it works, we should not change anything, as the touch code is ☚ī¸.

jfrickmann commented 2 years ago

I assume that you are working on the NV14 / EVT_TOUCH_BREAK issue?

That is different from the Lua buffer size, which was about events getting lost when he slows down Lua with a demanding script.

So I think that we are good 😄

gagarinlg commented 2 years ago

🤔đŸ¤Ļ Sorry

jfrickmann commented 2 years ago

NP 😎 Better be diligent and ask one extra time to avoid misunderstandings!

Please check #1442 !

rotorman commented 2 years ago

@JimB40 I tried to match the touch handling of NV14 code 1-to-1 with that of TX16S. As I have no NV14 to test, I was not brave enough to put it directly in EdgeTX repo, and used my fork instead: https://github.com/rotorman/edgetx/tree/nv14_touch For convenience I include a pre-built NV14 firmware binary (zipped due to attachment rules of GitHub) - could you please try if this works better? Thx! NV14_ETX_touchhandling.zip

JimB40 commented 2 years ago

Hey Guys. Checked enclosed binary. Events are completly screwed to the point touches in ETX UI don't work properly. Checking via LUA script FIRST and then BREAK comes once every 10 times. Rest of times SLIDE comes instead of FIRST/BREAK No TAP shows at all.

gagarinlg commented 2 years ago

Can you try PR #1434 please?

JimB40 commented 2 years ago

@jfrickmann checked #1442 Works much better with low and mid range CPU load. It gets events slowly but doesn't loose or skip them As you stated there will be always point when it will start to misbehave. So with CPU big load when I tap quickly several times it always slowly gets 5 events (I think 1st is not buffered). So it can be like FIRST BREAK FIRST BREAK FIRST But I agree that it will be hard to overcome it as system is very slow while input is quick.

JimB40 commented 2 years ago

@gagarinlg checked #1434 With low CPU load seems to work 'almost' correctly. like during 5 mins of sliding I spotted only once missing BREAK after SLIDE. But then with average-mid CPU load starts to behave like previously not catching BREAK after SLIDE. But it might be this buffer size. Does your PR include #1442 4 events buffer?

gagarinlg commented 2 years ago

No, it does not include the increased buffer and also some changes in libopenui are needed to get all bugfixes. Sliding is probably not completely working without the libopenui changes mentioned in the PR.

pfeerick commented 2 years ago

Ok, so it sounds like all three PRs will be needed, and then we can re-evaluate. I'll try and get them merged in the next nightly.

JimB40 commented 2 years ago

IRL test on NV14 with TBS AL https://youtu.be/Ec7vHZmHBOQ

JimB40 commented 2 years ago

@pfeerick Tested yor build from #1434 zip on NV14 Whith very low CPU load (up to 10 draw Text iterations) - success rate to get BREAK after SLIDE. is 100%. Above that it's like 70%

On TX16S to make it misbehave when CPU load must be quite heavy. Has to be around 90+ iterations. Below that it's slow but still working quite well

pfeerick commented 2 years ago

Indeed... that was what I was alluding to re: iNav widget running at the same time - this is a resource constrained system and you are sharing resources. This isn't single-task! ;) But it seems strange the NV14 seems more... flakey.

gagarinlg commented 2 years ago

I totally agree regarding NV14, the difference seems larger than the greater resolution should create

rotorman commented 2 years ago

Thanks for testing Robert, and sorry for wasting your time. I have been comparing my "work" with the horus branch tp_gt911 code and cannot see a change, so I am a bit stumped that it does not perform the same way. I do believe you when you say it does not work, would just like to learn why...

Can anyone see a difference between well working https://github.com/EdgeTX/edgetx/blob/dcbdde2b74baa3398306d06021b7bf7a65885efa/radio/src/targets/horus/tp_gt911.cpp#L721-L810

and the non-working events:

https://github.com/rotorman/edgetx/blob/6531f823eeb124766069070e9b2af41d891e4051/radio/src/targets/nv14/touch_driver.cpp#L469-L535

If I skip the required differences between the readouts of GT911 and FT6236 touch ICs, and ignore the fact that the variables for X and Y touch coordinates are named differently, I get exactly the same code, especially on the touch event handling part. Hmm... grafik

gagarinlg commented 2 years ago

I think it is not worth the effort. Lvgl only expects toucj/no touch events and all sliding and tapping detection is done within lvgl.

pfeerick commented 2 years ago

Ok, so should we go ahead with this as it is for 2.6, and hopefully fix it good as part of LVGL?

JimB40 commented 2 years ago

Does it mean in 2.6 LUA touch on NV14 will be 50% reliable?

gagarinlg commented 2 years ago

From libopenui all event are now forwarded to Lua. It is not the touch driver anymore. Now it probably is the higher CPU load. Ristos modifications would be the cleaner way, but they will not fix the problem. Dis increasing the buffer size help?

JimB40 commented 2 years ago

As stated yesterday. Tested @pfeerick build with increased buffer and those fixes Displayng 30 lines of text in a run() loop makes that BREAK does not show after SLIDE 30%-40% time. (On NV) On TX16S this starts to happen with 250+ lines of text displayed

pfeerick commented 2 years ago

I think before we can get any further with this we will need a standalone lua script that can be run on both TX16S or NV14 without any other dependencies which exhibits this behaviour... then it will be possible to start debugging this and/or make changes or determine exactly what is going on.

JimB40 commented 2 years ago

What you mean by dependencies? Is my test script not good? You can replace lcd.drawText in load-simulation loop with any thing else like creating array. And compare on TX16S and NV14

pfeerick commented 2 years ago

Er, um... I forgot it was there? :O Yes, that should be fine.

pfeerick commented 2 years ago

Just revisiting this issue with the current main (c7ca1cc) using the script given in the initial comment. From some brief testing, it seems consistent on NV14 and TX16 now, and the events seem to be firing consistently in order. What I am seeing though is when doing touch and slide, it is firing FIRST => SLIDE => TAP. At no point when I tap, double tap, slide, etc does it ever trigger 'BREAK'.

raphaelcoeffic commented 2 years ago

Ok, then let's close for now until somebody complains again.