feat(lua): Add getSourceValue

[jfrickmann]

Replaces #2175 - something was messed up with the base branch

[CapnBry]

Argh, I appologize! I didn't follow through on actually looking up and trying to understand the code for isFresh() and just assumed it was the condition I wanted. Today I learned...

• isFresh() - true if the sensor was updated in the last 320ms. This is the dots on the telemetry page that show briefly after an item ispdated.
• isOld() - true if the sensor has been lost. This is the bracketing of a telemetry item in B&W, or red highlighting on color screens

So I believe what we actually want is !isOld() and not isFresh(). I tested it with this change and everything checks out now.

diff --git a/radio/src/lua/api_general.cpp b/radio/src/lua/api_general.cpp
index 85322cff7..4647118db 100644
--- a/radio/src/lua/api_general.cpp
+++ b/radio/src/lua/api_general.cpp
@@ -784,7 +784,7 @@ static int luaGetSourceValue(lua_State * L)
             lua_pushinteger(L, value);
           break;
       }
-      lua_pushboolean(L, telemetryItems[qr.quot].isFresh());
+      lua_pushboolean(L, !telemetryItems[qr.quot].isOld());
     }
     else { // telemetry is not available
       lua_pushnil(L);

Tested
-- Case sensitive only match
-- getSourceInfo returns nil if not found, or table desc/unit/id/name
-- getSourceInfo works with both ID and string
-- getSourceValue works with both ID and string
-- getSourceValue/getSourceInfo works with non-telemetry items (tested thr)
-- getSourceValue returns nil before item has a value, allowing a default to be presented to the user
-- getSourceValue returns false for second parameter before item has a value
-- getSourceValue returns value when value is present
-- getSourceValue returns true for second parameter when telemetry streaming
-- getSourceValue returns previous value after telemetry lost
-- getSourceValue returns previous value after sensor lost
-- getSourceValue returns false for second parameter when telemetry lost
-- getSourceValue returns false for second parameter if sensor lost
-- getSourceValue returns true for second parameter when telemetry recovered

I know it was brought up in the discussion about returning more than a simple true / false for this value, since isFresh and isOld are both useful bits of information. One could not reproduce the Model -> Telemetry list without knowing what the state is, and for ExpressLRS testing it would be beneficial to be able to programmatically time the update rates. I admit that's a bit of a specialized use case though. I just thought I'd bring it up for consideration one last time before this becomes the new API.

Second Return Value	Meaning
nil	Sensor does not exist
SOURCE_STATUS_OLD = false	isOld() i.e. more than 20s since last update, or no telemetry streaming, or has never been updated
SOURCE_STATUS_NOMINAL = 0	Not old or fresh, just valid telemetry that wasn't just updated
SOURCE_STATUS_FRESH = 1	isFresh() i.e. sensor updated in last 320ms, or is mixer item or switch or standard thing

I'm leaning against this change just because it makes working with it possibly more complicated.

Compiler Warning

There's a compiler warning in the unit as well, which wasn't added by this but I thought I'd bring that up as well so maybe it can get fixed for 2.8 (as this was added for 2.8 #2279)

/src/radio/src/lua/api_general.cpp:2593:11: warning: comparison of unsigned expression in '>= 0' is always true [-Wtype-limits]
 2593 |   if (idx >= 0 && idx < MAX_OUTPUT_CHANNELS) {

[pfeerick]

There's a compiler warning in the unit as well, which wasn't added by this but I thought I'd bring that up as well so maybe it can get fixed for 2.8 (as this was added for 2.8 #2279)

/src/radio/src/lua/api_general.cpp:2593:11: warning: comparison of unsigned expression in '>= 0' is always true [-Wtype-limits] 2593 | if (idx >= 0 && idx < MAX_OUTPUT_CHANNELS) {

Thanks... mixsrc_t is actually a uint32_t so can never be < 0 🤦 . Can fix this before merging.

One could not reproduce the Model -> Telemetry list

I think this is the key aspect - since this is something we are trying to achieve with extending the Lua APIs... so I have nothing against this if it makes sense to Jesper, et al. It is probably better to be added here, then as part of yet another function? Since as I understand it now the second parameter essentially only tells you if telemetry is active or not at the moment, and you don't know what the health state of the data/link is.

[jfrickmann]

I have now replaced isFresh() by !isOld() as requested. Instead of making the second return value complicated, we could also add a third return value with an integer giving a detailed status. If you hash out exactly what you want, then I will be delighted to stick it in there 😄

[CapnBry]

I don't think there's any need for another return value. If you look at the list I have above, both nil and false evaluate to false in a comparison, and the others are status numbers.

If you go by the way the code is currently in this PR, there's nothing to gain from adding another return parameter to juggle. There's nothing that says the status has to be a specific type. You can even use a SOURCE_STATUS_ERROR or something for the nil value in the docs. This doesn't have to be defined in the EdgeTX source because any value that doesn't exist is nil already, so code like this all works

local val, status = getSourceValue(id)
-- Symbolic constants don't care what the type is and the code looks the same
if (status == SOURCE_STATUS_ERROR) then
  val = 'NotDefined'
else if (status == SOURCE_STATUS_OLD) then
  val = val .. ' OLD'
else if (status == SOURCE_STATUS_FRESH) then
  val = val .. ' FRESH'
else if (status == SOURCE_STATUS_NOMINAL) then
  val = val -- no decoration, just for demonstration
end

-- But also works as a straight boolean for simple things, turning old or non-existent items in warning color
lcd.drawText(0, 0, val, status and COLOR_THEME_PRIMARY1 or COLOR_THEME_WARNING)

The immediate response might be "Well what happens when we add other statuses at some point that could be consistent with the old/fresh/nominal/non-existent status?!". I mean these are the same statuses that telemetry items have had for like 5 years now so let's not get crazy thinking suddenly items will have all sorts of new properties that will be able to be expressed as part of this proposed new integer returned. 😄

The code is insanely simple with just the two return values, both on the Lua side and the C side (although it would probably be good to refactor all the telemetryItems[qr.quot] into a variable). Lua is an amazing language and we should use its features instead of inflating the number of returned values for no additional benefit.

diff --git a/radio/src/lua/api_general.cpp b/radio/src/lua/api_general.cpp
index 85322cff7..f510a78d6 100644
--- a/radio/src/lua/api_general.cpp
+++ b/radio/src/lua/api_general.cpp
@@ -745,9 +745,8 @@ static int luaGetSourceValue(lua_State * L)

   if (!valid)
   {
-    lua_pushnil(L);
-    lua_pushboolean(L, false);
-    return 2;
+    // Return nil, nil (SOURCE_STATUS_ERROR) implicitly
+    return 0;
   }

   if (src >= MIXSRC_FIRST_TELEM && src <= MIXSRC_LAST_TELEM) {
@@ -770,7 +769,7 @@ static int luaGetSourceValue(lua_State * L)
             // Return nil if there are no cells
             if (telemetryItems[qr.quot].cells.count == 0) {
               lua_pushnil(L);
-              lua_pushboolean(L, false);
+              lua_pushboolean(L, SOURCE_STATUS_OLD);
               return 2;
             }
             luaPushCells(L, telemetrySensor, telemetryItems[qr.quot]);
@@ -784,20 +783,25 @@ static int luaGetSourceValue(lua_State * L)
             lua_pushinteger(L, value);
           break;
       }
-      lua_pushboolean(L, telemetryItems[qr.quot].isFresh());
+      if (telemetryItems[qr.quot].isFresh())
+        lua_pushinteger(L, SOURCE_STATUS_FRESH);
+      else if (telemetryItems[qr.quot].isOld())
+        lua_pushboolean(L, SOURCE_STATUS_OLD);
+      else
+        lua_pushinteger(L, SOURCE_STATUS_NOMINAL);
     }
     else { // telemetry is not available
       lua_pushnil(L);
-      lua_pushboolean(L, false);
+      lua_pushboolean(L, SOURCE_STATUS_OLD);
     }
   }
   else if (src == MIXSRC_TX_VOLTAGE) {
     lua_pushnumber(L, float(value) * 0.1f);
-    lua_pushboolean(L, true);
+    lua_pushinteger(L, SOURCE_STATUS_NOMINAL);
   }
   else {
     lua_pushinteger(L, value);
-    lua_pushboolean(L, true);
+    lua_pushinteger(L, SOURCE_STATUS_NOMINAL);
   }
   return 2;
 }

[jfrickmann]

With all due respect, this is not a simple solution. First of all, you have to add these new constants to the C++ code, and then you also have to export the constant values and names to Lua. This means that you are now creating constants and name spaces just for Lua that do not get used in the underlying EdgeTX system. This is directly contrary to how I believe that things should be done, which is to have the Lua API be based on the underlying API w/o adding new things to it.
This is, of course, just my personal opinion, and since I do not have any personal interest in this PR, I will give you whatever you want. As long as you feel confident that it has been well deliberated, and others who want this are cool with it.
So shall I go ahead and add these constants and create the API as suggested above?

[CapnBry]

Oh no offense taken at all, your point makes a lot of sense. I didn't include the C or Lua defines in my diff to just keep it clear about how they would work, but I had them in my local repo.

You're right about it not being a great idea to create new definitions that would only be used for the Lua interface, so I think just leave it what you've got currently. It solves the current issue of the values "disappearing" when telemetry is lost and requiring the Lua cache all the last values if it doesn't want its display to turn into a bunch of zeros suddenly, and resolves the ambiguity of the 0 currently being returned as a possibly valid value.

EDIT: One minor thing, when pushing the table for getSourceInfo there's code to push a nil units field if it doesn't have units. However, Lua associative tables do not store nil values so this code can be removed-- retVal.units will be nil with or without it.

[jfrickmann]

Great! I have never been accused of being diplomatic; I find it easier to just lay it out as I see it - thanks for understanding 🍻

Since the underlying API has several different functions isFresh and isOld etc. it is not a problem to push them as return values for telemetry - please let me know if you want it.

Good catch with the redundant code in getFieldInfo - I will clean that up

[pfeerick]

So I take it this is ready as it is, and access to isFresh and isOld will be added separately?

[CapnBry]

So I take it this is ready as it is, and access to isFresh and isOld will be added separately?

I think this decision should be made before merging this so that the API of this function is not continuously changing and requiring the Lua code to figure out and work around what capabilities are available. You could just slap another boolean return value on there, but that seems like a waste considering only Telemetry items have this property so there's a sprinkling of adding a fixed "true" to every other item.

I do not have any further opinions on the subject though, since I just seem to be holding everything up with my analysis.

[pfeerick]

Indeed you are, but it is an entirely worthy analysis, so you are forgiven for that ;) API changes need discussion so all possible variations and situations are considered, rather than just the original intended and potentially single use case. It does seem like a waste, but it also gives access to that last bit of info, so IMO is an acceptable evil.

[jfrickmann]

How about we make the second return value into a table, and then we can return isOld, isFresh etc. as table fields?

[CapnBry]

Returning a table has more of a performance hit to both Lua memory and script size than just returning an additional boolean and is a slightly more cumbersome to work with (having to remember the exact string name of the value in the table). My opinion is that it should just return value, !isOld, isFresh as 3 return values, and everything except Telemetry items return true for the 2nd and 3rd items.

If the requested item is not found I think you can just return nil for all 3? That is, have the if (!valid) check just return 0 instead of two pushes and a return 2. nil evaluates as false and allows the Lua script to differentiate between a value that hasn't been received yet and one that is never going to have a value because it doesn't exist.

[jfrickmann]

OK, you got it!

[CapnBry]

Tested, all pass except the one I'm not sure you're going to do
-- Case sensitive only match
-- getSourceInfo returns nil if not found, or table desc/unit/id/name
-- getSourceInfo works with both ID and string
-- getSourceValue works with both ID and string, returning the same values
-- getSourceValue/getSourceInfo works with non-telemetry items (tested thr)
-- getSourceValue returns nil before item has a value, allowing a default to be presented to the user
-- getSourceValue returns value when value is present
-- getSourceValue returns previous value after telemetry lost
-- getSourceValue returns previous value after sensor lost
-- getSourceValue returns false for second parameter before item has a value
-- getSourceValue returns true for second parameter when telemetry streaming
-- getSourceValue returns false for second parameter when telemetry lost
-- getSourceValue returns false for second parameter if sensor lost
-- getSourceValue returns true for second parameter when telemetry recovered
-- getSourceValue returns true for third parameter when for a brief instant after item is updated / otherwise false ASTERISK
-- getSourceValue returns true for third parameter for non-telemetry items always (tested thr)

-- getSourceValue returns nil for all values if the item does not exist: FAIL
Not sure if this last one was supposed to be implemented or not, currently returns nil, false, false, and the C code can just return 0

ASTERISK Just to note the first time a telemetry item is receiver, the third parameter (isFresh) returns true for a long time (5 seconds?). This matches the EdgeTX display on Model -> Telemetry. This only seems to happen if it has been longer than a certain amount of time since telemetry was lost? As if I unplug and replug the flight controller in just a couple of seconds it behaves as expected (isFresh = true for just an instant) but somewhere around 20s of lost power isFresh = true for ~5s when telemetry_streaming goes true (regardless of if the queried value was actually updated).

This is the code I have been using, but sort of hacking it as needed to test everything above.

local LH = 14
local xoff
local Y

local function echo(msg)
  lcd.drawText(xoff, Y, msg)
  Y = Y + LH
end

local function run(event, ts)
  lcd.clear()
  if ts then
    lcd.drawText(0, 0, string.format("%ux%u", ts.x, ts.y))
  elseif event ~= 0 then
    lcd.drawText(0, 0, tostring(event))
  end
  
  local strTItem = 'RxBt'
  Y = LH
  xoff = 0
  
  local info = getSourceInfo(strTItem)
  if info == nil then
    echo(strTItem .. ' is nil')
  else
    for k, v in pairs(info) do
      echo('(' .. k .. ')=[' .. tostring(v) .. ']')
    end

    if info.id then
      local val = table.pack(getSourceValue(info.id))
      --echo('Value table item count ' .. tostring(#val))
      for k, v in pairs(val) do
        echo('val(' .. k .. ') is [' .. tostring(v) .. '] ' .. type(v))
      end
      if val[3] then
        val[1] = val[1] .. ' *'
      end
      lcd.drawText(LCD_W/2, 0, tostring(val[1]), val[2] and COLOR_THEME_PRIMARY1 or COLOR_THEME_WARNING)
      
      echo('')
      local info2 = getSourceInfo(info.id)
      echo(info2 and 'getSourceInfo(id) PASS' or 'getSourceInfo(id) FAIL')

      local val2 = table.pack(getSourceValue(strTItem))
      -- echo('Value2 table item count ' .. tostring(#val))
      local allMatch = true
      for k, v in pairs(val2) do
        if v ~= val[k] then
          allMatch = false
        end
      end
      echo(allMatch and 'getSourceValue(str) PASS' or 'getSourceValue(str) FAIL')
    end
  end

  echo('')
  echo('-- MIX SOURCE --')
  info = getSourceInfo('thr')
  if info == nil then
    echo('Thr is nil')
  else
    echo ('Throttle item found, id ' .. tostring(info.id))
    
    local val, valid, fresh = getSourceValue('thr')
    echo('Throttle ' .. tostring(val) .. (valid and ' VALID' or ' NEVER') .. (valid and ' FRESH' or ' STALE'))
  end
  
  local val3 = table.pack(getSourceValue('pugh'))
  for k, v in pairs(val3) do
    echo('val3(' .. k .. ') is [' .. tostring(v) .. '] ' .. type(v))
  end

  return 0
end

return { run=run }

[pfeerick]

@jfrickmann Your thoughts?

-- getSourceValue returns nil for all values if the item does not exist: FAIL
Not sure if this last one was supposed to be implemented or not, currently returns nil, false, false, and the C code can just return 0

[jfrickmann]

returns nil for all values if the item does not exist

OK, changed that. Please test again.

[CapnBry]

OK, changed that. Please test again.

Working. However you don't need to push 3 nils, you can just return 0 without pushing anything and the caller automatically gets as many nils returned as they expect.

Source docs also need updating:

-@retval value current source value (number).
+@retval value current source value (number), or last known telemetry item value.

-value is nil for non-existing sources and all non-allowed sensors while FAI MODE is active
+value is nil for non-existing sources. all non-allowed sensors while FAI MODE is active, or if a telemetry item value has never been received

-@retval status is false for telemetry sources when the telemetry stream is not received, or value is nil. Otherwise, it is true
+@retval isCurrent is true for telemetry sources that are within the "Sensor Lost" duration and telemetry is streaming . Always true for non-telemetry items. 
+
+@retval isFresh is true for telemetry sources which have been recently updated and telemetry is streaming. Always true for non-telemetry items.

[jfrickmann]

OK, you got it

[pfeerick]

Thank you all! :)