fix(lua): Do not pass key event that triggered telemetry view to lua

[bug400]

Fixes (Taranis only) #3253
After switching to the telemetry view, the LUA key event buffer could still contain the key event that was used to switch to the telemetry screen. See below an example for script evt1.lua
evt1.zip
on a Taranis Q X7 ACCESS:
with patch:
Key event received 1 4097

without patch:
Key event received 1 99
Key event received 1 131 (long page)
Key event received 1 4097
This behavior is unrequested and does neither occur in opentx nor in elder edgetx releases.

Summary of changes:
The LUA key event buffer is cleared at the beginning of resumeLUA if the radio just switched to the telemetry view.

[pfeerick]

Rebased onto correct branch. Please take note of the changes requested above.

[bug400]

Unfortunately the EVT_ENTRY event comes in after the LONG_PAGE event had arrived in the resumeLua subprogram. Alternatively it would be necessary to walk through the queue every time when pulling an event to see if an EVT_ENTRY event exists.

[raphaelcoeffic]

Unfortunately the EVT_ENTRY event comes in after the LONG_PAGE event had arrived in the resumeLua subprogram. Alternatively it would be necessary to walk through the queue every time when pulling an event to see if an EVT_ENTRY event exists.

Ok, maybe I should have a look. There is probably a simpler solution to this issue. The present PR feels like a hack to hide something else.

[bug400]

The origin of the problem seems to be in guiMain (main.cpp). The previous implementation did not guarantee that keyboard events are not sent to luaTask, if neiter a telemetry script nor a standalone LUA script is active. Successfully tested on radio (X7 ACCESS) and simulator for the 2.9 branch and on simulator only for the main branch.

[raphaelcoeffic]

@philmoz do you have an idea what's going on?

[bug400]

Explenation in more detail:
After switching to the telemetry view, the LUA event buffer contains the events:
99
131 (long enter)
4097 (EVT_ENTRY)
The events before EVT_ENTRY don't belong there and may cause errors in LUA keyboard event handlers.

My first PR solved this issue by clearing the event queue in interface.cpp just after a LUA telemetry screen became visible.

As @raphaelcoeffic correctly guessed, this PR only hided the cause of the problem which is in my opinion in guiMain:

Original code:

 bool handleGui(event_t event) {
   bool refreshNeeded;
 #if defined(LUA)
  refreshNeeded = luaTask(event, true);
  if (menuHandlers[menuLevel] == menuViewTelemetry &&
      TELEMETRY_SCREEN_TYPE(s_frsky_view) == TELEMETRY_SCREEN_TYPE_SCRIPT) {
      menuHandlers[menuLevel](event);
  }
  else if (scriptInternalData[0].reference != SCRIPT_STANDALONE)
 #endif
// No foreground Lua script is running - clear the screen show normal menu

In this code events are passed to luaTask even if neither telemetry nor standalone screens are active. Thus I changed the code in this PR to:

bool handleGui(event_t event) {
  bool refreshNeeded; 

#if defined(LUA)
// LUA telemetry foreground script active
  if (menuHandlers[menuLevel] == menuViewTelemetry &&
      TELEMETRY_SCREEN_TYPE(s_frsky_view) == TELEMETRY_SCREEN_TYPE_SCRIPT) {
      refreshNeeded = luaTask(event, true);
      menuHandlers[menuLevel](event);
  }
// standalone foreground script active
  else if (scriptInternalData[0].reference == SCRIPT_STANDALONE)
      refreshNeeded = luaTask(event, true);
  else 
 #endif
 // No foreground Lua script is running - clear the screen show normal menu

[philmoz]

I think you've identified the cause of the problem; but I'm not entirely happy with the solution.
Moving the luaTask call might have other unintended side effects as it does things even when there are no Lua scripts running.

This is the only case where a non-zero event value is passed to luaTask (color radios call luaPushEvent separately).

I think it would be better to test if a telemetry script or standalone script is running and call luaPushEvent, then call luaTask as is; but with a 0 event value.
Untested code:

bool handleGui(event_t event) {
  bool refreshNeeded;
#if defined(LUA)
  bool isTelemView = menuHandlers[menuLevel] == menuViewTelemetry &&
                     TELEMETRY_SCREEN_TYPE(s_frsky_view) == TELEMETRY_SCREEN_TYPE_SCRIPT;
  bool isStandalone = scriptInternalData[0].reference == SCRIPT_STANDALONE;
  if (isTelemView || isStandalone)
    luaPushEvent(event);
  refreshNeeded = luaTask(0, true);
  if (isTelemView) {
      menuHandlers[menuLevel](event);
  }
  else if (!isStandalone)
#endif
// No foreground Lua script is running - clear the screen show normal menu
  {
    lcdClear();
    menuHandlers[menuLevel](event);
    drawStatusLine();
    refreshNeeded = true;
  }
  return refreshNeeded;
}

You will also need to add this to the start of main.cpp:

#if defined(LUA)
  #include "lua/lua_event.h"
#endif

[bug400]

You are absolultely right that moving luaTask must be considered very carefully.
I inserted traces at the beginning of luaTask and perMain which shows (neither a telemtry nor a standalone script is active):

2,35s: perMain called
2,35s: (luaTask evt 0 allowLcdUsage 0
2,35s: (luaTask evt 0 allowLcdUsage 1
2,41s: perMain called
2,41s: (luaTask evt 0 allowLcdUsage 0
2,41s: (luaTask evt 98 allowLcdUsage 1
2,46s: perMain called
2,46s: (luaTask evt 0 allowLcdUsage 0
2,46s: (luaTask evt 0 allowLcdUsage 1
2,51s: perMain called
2,51s: (luaTask evt 0 allowLcdUsage 0
2,51s: (luaTask evt 0 allowLcdUsage 1

luaTask with allowLcdUsage == false is called anyway in each iteration of perMain. Thus luaTask is always called twice, even if there is no foreground script active. Concerning my PR we have to check whether a move of luaTask with allowLcdUsage==true has side effects. Unfortunately I was not able to understand the logic in resumeLua.

Concerning your version, I propose a modification:

#if defined(LUA)
  bool isTelemView = menuHandlers[menuLevel] == menuViewTelemetry &&
                     TELEMETRY_SCREEN_TYPE(s_frsky_view) == TELEMETRY_SCREEN_TYPE_SCRIPT;
  bool isStandalone = scriptInternalData[0].reference == SCRIPT_STANDALONE;
  if ((isTelemView || isStandalone))
     refreshNeeded = luaTask(event, true);
  else
     refreshNeeded = luaTask(0, true);
  if (isTelemView)
      menuHandlers[menuLevel](event);
  else if (scriptInternalData[0].reference != SCRIPT_STANDALONE)
#endif
// No foreground Lua script is running - clear the screen show normal menu

which was tested successfully in simulator.

If luaPushEvent is called in HandleGui as you proposed, the event parameter of function luaTask becomes obsolete. And it is better to call the low level subroutine luaPushEvent in luaTask.

[philmoz]

If luaPushEvent is called in HandleGui as you proposed, the event parameter of function luaTask becomes obsolete. And it is better to call the low level subroutine luaPushEvent in luaTask.

The color LCD code always calls luaPushEvent outside of the luaTask function. My recommendation it to make the B&W code match this and remove the event parameter from luaTask.

[bug400]

Done. Checked successfully with branch main on XQ7 ACCESS (emulator) and on branch 2.9 on XQ7 ACCESS (emulator + radio).

[pfeerick]

There don't appear to be any negative side effects of this on TX16S, X9D+ or Zorro when tried with standalone lua like ELRS / MPM config Lua, or telemetry screen widges like yappu / iNav telemetry or colorlcd lua widgets.