Cleanup KEY handling code. (Development discussion).

[philmoz]

Over time the code for handling KEY events (ENTER, EXIT, PAGE, etc) has become somewhat messy.

I'd like to propose some changes and get feedback.
Barring bugs these should not affect the functionality of the radio firmware; but may have some impact to Lua scripts (see below).

Some terminology first. There are four types of events generated when a key is pressed on the radio:

• FIRST - when the key first pressed.
• BREAK - when the key is released.
• LONG - when the key is held down for a bit.
• REPT - when the key is held down for a bit longer (this can be sent multiple times).

1. A short key press event (BREAK) is always generated after a long press of any key.

• This has resulted in both the firmware code and Lua scripts being dotted with calls to killEvents() to cancel the BREAK event so it will not inadvertently trigger an unexpected action.
• I am proposing to change the low level key handling code so that the BREAK event is never generated if a LONG event has been sent. Then remove all the killEvents() calls in the code that are no longer needed.

2. Change action triggering from using FIRST to using BREAK.

• There is a lot of inconsistency in how actions are trigged in the code. Some actions trigger as soon as the key is pressed (FIRST), while others happen when the key is released (BREAK).
• I am proposing to change triggering from FIRST to BREAK where possible. There are some cases where FIRST makes sense and these should remain.

3. Change LONG(PAGEDN) to BREAK(PAGEUP) for color radios with only a PAGEDN key.

• Currently color radios that have only a single PAGEDN key treat the long press of this key as equivalent to pressing the PAGEUP key on radios that have both. This requires multiple different test in the code to handle all the cases.
• I am proposing to change this in the low level key handler such that a LONG(PAGEDN) event is converted to a BREAK(PAGEUP) event so the higher level code can be simplified.

4. Change the PAGE key definition for B&W radios to PAGEDN in the code.

• Color radios with a single page key use the PAGEDN definition while B&W radios use the PAGE definition. Again this results in lots of special cases and tests in the code to handle the differences.
• I am proposing to replace PAGE with PAGEDN in the code to make things more consistent. This also means item 3 above would then apply to B&W radios with only one page key.

Impacts to Lua scripts.

• Change 1 will remove the BREAK event after a LONG event.
• For radios with a single PAGEDN key, change 3 will:
  • replace the EVT_PAGEDN_LONG event with EVT_PAGEUP_BREAK
  • remove the EVT_PAGEDN_REPT event
• For radios with a single PAGE key, change 4 will:
  • replace the EVT_PAGE_FIRST event with EVT_PAGEDN_FIRST
  • replace the EVT_PAGE_BREAK event with EVT_PAGEDN_BREAK
  • replace the EVT_PAGE_LONG event with EVT_PAGEUP_BREAK
  • remove the EVT_PAGE_REPT event

Because the LONG press event is converted to a BREAK event it is not possible to have REPT events for radios with a single PAGE/PAGEDN key. This is also true in the current firmware.
For radios with both PAGEUP and PAGEDN keys there is no change - all four event types are available for both keys.

[pfeerick]

Given the most... visible... impact will be to Lua scripts (as the rest is mostly internal cleanup), it would be good to get feedback from some of the Lua script devs on this proposal as summarised specifically for Lua at the bottom (these are some who come to mind offhand): @lshems @JimB40 @offer-shmuely @frankiearzu @yaapu @stronnag @CapnBry @klutvott123

[philmoz]

@JimB40 Understand what you are saying; but I'm not proposing to change any of that, other than suppressing the BREAK event after a LONG/REPT event.

The main concern is if the changes to the PAGEDN / PAGE events will impact existing scripts.

[JimB40]

Suppressing BREAK if LONG or LONG, REPT was generated should have no impact on existing scripts.

EVT_VIRTUAL_PREV_PAGE is usually aliasing KEY_PAGE_LONG or KEY_PAGEUP_LONG and KEY_PAGE_BREAK is usually killed with killEvents() in existing LUA scripts.
EVT_VIRTUAL_NEXT_PAGE is usually aliasing KEY_PAGE_BREAK or KEY_PAEGUP_BREAK

Will review some existing open-source lua code but at first look it should be okay

[JimB40]

BTW you can find quite interesting code

What is EVT_ROT_BREAK? :)

[pfeerick]

looks like it was added a while back... not exactly sure why though ... opentx/opentx#3791

[philmoz]

EVT_ROT_xxx is an alias for EVT_ENTER_xxx for radios with a rotary encoder.

[JimB40]

[pfeerick]

d. EVT_PLUS_xxx, EVT_MINUS_xxx -> OK
only on FrSky X9D

This is actually on four ETX supported radios... the X9D, X9D+. Commando8 and T8... as well as being exposed through the trim hats on EL/PL18.

I would suggest you don't go outside of the scope of the original post... some of what you are suggesting are breaking changes that should come in at 3.0, not now.

[philmoz]

That's all well and good; but this discussion is not about re-designing the Lua event API.

I would appreciate if you could just focus on the proposed changes to the existing Lua EVT names.

[JimB40]

Okay here you have focus:

1. No impact on LUA scripts.

2. AFAIUI this regards C++ UI handling not how LUA API events are generated.

• With reservation mentioned in EdgeTX 3.0 Events handlig #4840 (EdgeTX Ui/UX speed) no impact on LUA scripts

3. this regards C++ UI handling not how LUA API events are generated. - no impact
   There are no radios with single PAGEDN key
   a. single key labeled PAGE - FrSky X7, X9D(2019) Jumper T16, T18 etc
   b. single key labeled Page Up/Dn FrSky X10/X12
   c. two keys labeled PAGE> & PAGE< - most Radiomaster radios
   How variables are named inside C++ code it's up to you. I'd not use name PAGEDN if there is no such a hardware key.

4. this will break compatibility with most of existing LUA scripts for B&W targets with single page KEY
   Example from iNav code
   

• again considering future cleanup EdgeTX 3.0 Events handlig #4840 for hardware (key)/interface (virtual) events. I would not change hardware key name PAGE to non existent PAGEDN

Not sure about final conept so to confirm.

Possible events queue now:

1. FIRST,BREAK
2. FIRST,LONG,BREAK
   3.FIRST,LONG,REPT,BREAK
   4.FIRST,LONG,REPT,REPT,REPT,...,BREAK

Events queue after change

1. FIRST, BREAK
2. FIRST,LONG
3. FIRST,LONG,REPT
4. FIRST,LONG,REPT,REPT,REPT,...

Because the LONG press event is converted to a BREAK event it is not possible to have REPT events for radios with a single PAGE/PAGEDN key. This is also true in the current firmware.

Not sure what you mean here. Please give example of radio with single PAGE key where EVT_PAGE_REPT is not possible.

X7 2.9 SIM, Lua Dev Tools, PAGE key pressed for 2sec then released

[philmoz]

3. this regards C++ UI handling not how LUA API events are generated. - no impact
   There are no radios with single PAGEDN key

T16, T18 and X10 are all defined as using PAGEDN/PAGEUP - i.e. the Lua event names are EVT_PAGEDN_xxx and EVT_PAGEUP_xxx (regardless of what is printed on the radio button).

4. this will break compatibility with most of existing LUA scripts for B&W targets with single page KEY

Ok, so the event names have to remain the same.

Because the LONG press event is converted to a BREAK event it is not possible to have REPT events for radios with a single PAGE/PAGEDN key. This is also true in the current firmware.

As I said - current firmware (as in main branch & 2.10). PR #4833 removes the BREAK(PAGE) after LONG(PAGE) for B&W radios with a single PAGE key (to match the issue fixed in PR #2668 for color radios). This also kills the subsequent REPT events.

[JimB40]

T16, T18 and X10 are all defined as using PAGEDN/PAGEUP - i.e. the Lua event names are EVT_PAGEDN_xxx and EVT_PAGEUP_xxx (regardless of what is printed on the radio button).

So this be should be changed to EVT_PAGE_xxx. if not now in 3.0
LUA event constant names can be different from C++ variables
@mha1 coded this way when we did pr for nv14 hats

Because the LONG press event is converted to a BREAK event it is not possible to have REPT events for radios with a single PAGE/PAGEDN key. This is also true in the current firmware.

As I said - current firmware (as in main branch & 2.10). PR #4833 removes the BREAK(PAGE) after LONG(PAGE) for B&W radios with a single PAGE key (to match the issue fixed in PR #2668 for color radios). This also kills the subsequent REPT events.

Pretty sure fixing one thing shouldn't be reason to break another.
There is quite complete event model

• except something
• except something
• except something

Got used to it. I can live without REPT event on PAGE key.

[offer-shmuely]

I do not see any issue on the script I have.
and we will find some, we can fix them easily (although the user will to know that he need to upgrade the scripts as well)

however I do thing that API behavior changes, should be on major version only.
especially when 3.0 is so close (isn't it ;-) )