Closed JimB40 closed 2 years ago
Yeah it would be great if it returned nil for values that don't exist.
It also would be sweet if getValue() returned two values, the value and true if the sensor value is valid. It is horribly annoying for the value returned to be 0 when telemetry_streaming is false, leading to the Lua having to cache the last value if it wants to keep showing things like the last GPS position or link information like LQ / SNR / etc.
local lq, lq_valid = getValue("RQly")
lcd.drawText(0, 0, "LQ: " .. tostring(lq) .. "%", lq_valid and 0 or INVERS)
-- instead of
last_lq = 0 -- global cache of last value
local lq = getValue("RQly")
if lq == 0 then
lcd.drawText(0, 0, "LQ: " .. tostring(last_lq) .. "%", INVERS)
else
lcd.drawText(0, 0, "LQ: " .. tostring(lq) .. "%", 0)
last_lq = lq
end
Agree @CapnBry .
Choosing to check if source exists one of valid values (0) is evident bug. Regarding telemetry sources we have three states
For case 1 it shoud definetely return 'nil'
For case 2 & 3 to detect if Telemerty stream is available you can use new in EdgeTX LUA function getSwitchValue()
local tele_switch_ID = getSwitchIndex("TELE")
local tele_ON = getSwitchValue(tele_switch_ID)
@jfrickmann what you say?
Totally agree on case 1. This is breaking behavior but golly it is hard to know what you're working with when "0" is returned for so many cases.
I'm not a fan of having to carry around the "TELE" value anywhere I need to know if the value is valid, and that only works at the TELEMETRY_STREAMING level, not the "Sensor lost" level. Having to cache another ID and passing the tele_ON
value to all your rendering functions is an extra step that just isn't needed.
Lua is all about multiple return values. Just return if the sensor value is current when calling getValue(). The developer can then choose between behaviors using the TELE high-level value or the per-sensor local value, would_not_be_in_brackets_in_tlm_view = getValue(X)
. The getValue implementation has this information available so it is a two line change (one lua push and change the return value to 2) that offers more functionality and convenience to the Lua developer.
I'm not a fan of having to carry around the "TELE" value anywhere I need to know if the value is valid, and that only works at the TELEMETRY_STREAMING level, not the "Sensor lost" level. Having to cache another ID and passing the
tele_ON
value to all your rendering functions is an extra step that just isn't needed.Agree. Forgot that telemetry stream might be available but particular sensor might be lost. So seems your proposal is better.
btw last elrs fw 3.0.1 doesn’t sent RSNR (always zero) and i had quite a long hunt what’s going on :)
btw last elrs fw 3.0.1 doesn’t sent RSNR (always zero) and i had quite a long hunt what’s going on :)
It for sure does send RSNR (we rely on this for dynamic power), but it is always 0 in FLRC mode because there is no SNR available in that mode (so dynamic power falls back to RSSI). To keep this on topic, hit me up on our discord if you are experiencing something different.
Actually, @lshems has been complaining about this issue for years 😄
I agree that the most logical API would return nil
if there is no value for whatever reason. But I also agree that we should not break the existing API because I know from personal experience how much grief that can get you...
So I support adding a boolean return value to indicate if the value is present. But it is not quite as simple as two lines of code, because getValue(...)
can return a:
UNIT_TEXT
But in every case, it only returns one argument (a table is just one argument) so adding a second argument as a valid
boolean value for the various cases should not be too hard.
If you all agree, then I will get it done.
Regardless of the type of the value, the second return parameter is agnostic of the parameter type, being simply:
(src < MIXSRC_FIRST_TELEM || src > MIXSRC_LAST_TELEM) || (TELEMETRY_STREAMING() && telemetryItems[X].isAvailable())
However I agree that simplifying it to one line is more confusing than it is worth. 😀
I don't think it should keep returning 0 though when the sensor or telemetry is lost. This leads to having to cache every last value to be able to keep displaying it. The second return value declaring if the "0" is a valid value or not is not nearly as helpful. If getValue()
won't be changed for compatibility reasons, I'd suggest a new method be added which returns the current value regardless of TLM_STREAMING && isAvailable
. That "0" is utterly useless for the user view, so developers have to fall back on copying the data to keep the display running, and adds more code and storage to the script for every item that needs to be displayed (see my example above). The same EdgeTX code can be used, just that luaGetValueAndPush
would need to take a parameter to know if it needs to force pushing a 0 or the actual usable value in case the item isn't current.
Just my point of view:
A measurement is what it is, a measurement. You should always transfer the measurements to your processing system, and let the processing system do what it needs. That way, you always have the maximum available info for the processing system. Knowing there is no measurement is also info.
So now decide if you want a function to provide the measurements and an other to provide processed measurements.
The current is providing processed values.
Add a new function to provide raw measurements from sensors, best even without scaling etc, which should be provided as info as well, to be used in the processing system.
Comparable to the canon RAW picture format and the standard processed jpeg format we all know.
@lshems , I have a Sony camera where RAW files are lossy compressed 🤦
But we could add a getSensorValue
function that simply throws whatever it is back in a table including boolean values for streaming
and isAvailable
. This would not have to deal with all of the other cases for switches, knobs, Tx voltage etc. like getValue
.
@CapnBry , the basic getValue
function being called from luaGetValueAndPush
will return a zero if it does not find the source. It is implemented in mixer.cpp. So when you get a zero in the first place, it can either be that the source actually returns a zero (e.g. 3-pos switch is centered) or it doesn't know the source. Then we have to test if it is UNIT_CELLS
, and if the count is zero, because here luaPushCells
will push a zero. So there are a few more layers to it.
I do not want to be the dev who changes something like this, and hence becomes the person who has to deal with all of the grief and complaining that follows - just sayin' 😉
Path of least resistance - new function that adds the proposed new capability, and depreciate this to be removed in the next major version. When that will be is anyones guess, so there's plenty of "we gave you plenty of time to read the tea leaves" time ;)
Anything else (i.e. changing existing established behaviour), well, the person who wants that is the person who all the complaints from users will be sent to, and will be required to respond to them - "delete" will not be an option! 😝
My 2c on functionality based on what has been said (as I've not look at the code) is it should return nil
if not present, last value if not "online", and there should be a flag to say whether it's cached or live. Which fits in perfectly with the getTeleValue()
name...
Ah see that makes more sense now about how everything returns 0 currently for things that don't exist. I assumed every ID was valid and only the telemetry sensors IDs had the option of not existing or being offline so yeah I get how it is a larger change. Thanks for the education.
I think @pfeerick 's 2 cents is exactly the functionality I've been wanting for years of writing Lua telem scripts and tools. That provides so much more information to be able to display a message if missing sensors need to be discovered, no need to cache last values, and knowing if the value is certified fresh.
So just to be clear - this discussion is all about sensor values, but getValue
returns not only sensor values but also other things such as e.g. sticks, sliders, knobs and switches.
So we will not touch getValue
. Instead, we will add a new API function that only returns sensor values, and which will do whatever you can agree on in this thread, and which fits within the existing EdgeTX telemetry framework. A few questions:
value
(number, table or nil), isAvailable
, isStreaming
, and isOld
?isAvailable = false
or nil?isAvailable = false
?isStreaming= false
?getSensorValue
?@jfrickmann
Just FIY. I've refreshed for EdgeTX spreadsheet summarizing switches,sources & key events that shows implementation for different radios https://docs.google.com/spreadsheets/d/11gRpQhv6LNhfq85o46ClZguxVQ8xOlNwmqPvndQvntk/edit?usp=sharing
As I remember @raphaelcoeffic stated that adding new function consumes memory resources that is particualrly problem for B&W target
There is already LUA function model.getSensor(sensorID) that returns various information about source type that is telemetry sensor. Since name "sensor" is used for "source" that is intended for telemetry values and telemetry sensors vary depending on model I'd consider folowing this path with model.getSensorValue(sensorID)
Thing to solve is that sourceID and sensorID have diferent numbering scheme. If you look at implementation in spreadsheet sources being telemetry sensors are the last ones. They start with different sourceID as every radio has different set of physical inputs. So for X9D first telemetry sensor has sourceID 232 but for X10 or TX16S it will be 251. Then sources are populated in groups of 3 for every sensor (value, max value, min value). BTW I think max allowed sesnsors number is 60 as there are 60 fields for telemetry sensors in Companion.
On the other hand sensorID starts from 0.
for X9D (first telemetry sensor) sensorID == 0 equals sourceID = 232
for X9D (second telemetry sensor) sensorID == 1 equals sourceID = 235
for X10 (first telemetry sensor) sensorID == 0 equals sourceID = 251
for X10 (second telemetry sensor) sensorID == 1 equals sourceID = 254 etc.
Propsal model.getSensorValue(sensor)
Then we should be able to code like @CapnBry propsed
local lq, lq_valid = model.getSensorValue("RQly")
lcd.drawText(0, 0, "LQ: " .. (lq and tostring(lq) .. "%" or "not discovered", lq_valid and 0 or INVERS)
or list all telemetry sensors
local sID = 0
while model.getSensor(sID) do
local sValue, sValid = model.getSensorValue(sID)
local sName = model.getSensor(sensorID).name
lcd.drawText(0, 0, sName..": " .. ( type(sValue) ~= 'table' and tostring(sValue) or 'multi' ), sValid and 0 or INVERS)
sID = sID + 1
end
valueMin & valueMax are optionail to include in implementation as we can always poll via getValue(sensorName..'-') or getValue(sensorName..'+') but including it we have all sensor data in one pass
What you think?
I think the proposed solution is perfect and provides all the information anyone would need about a telem sensor with the least code in the Lua.
Is the sensorId still the ID you get from getFieldInfo('XXX').id
, or would the IDs need to be pulled from getSensor(string) instead now?
I like that proposal. Instead of having model.getSensorValue
take either a string or sensor index argument, I would prefer to add a function model.getSensorId
to get the sensor index from the name, just to encourage script writers to get the sensor index once, and use that instead of the name, for the sake of efficiency.
I can also add an iterator function sensors
similar to sources
and switches
if you want it.
I know that Peter voiced some concern about the available memory in BW radios, but I don't know how close to the limit we are today.
😄 No memory problems on B&W for any of my telemetry scripts, but boy that ELRS tools script wrecks the whole place with its monolithic nature.
getFieldInfo currently returns IDs for telem items so I am not sure a new function is needed? I mean it certainly is more correct to have getSensorId and getSensorValue that match, but if getSensorId is just a getFieldInfo that only works for a subset of strings IDs, it seems redundant to have two.
There are two meta-indices in ETX. One is sources, where everything that can produce a value (e.g. sticks and telemetry sensors) is indexed, and the other is switches where everything that can produce a boolean (e.g. switch positions and logical swithes) is indexed. And there are direct indexes e.g. one index for logical switches and another for telemetry sensors. What we will do here, is use the direct index for telemetry sensors instead of the meta-index for sources. As JimB40 explained above, these will be different from each another. For the BW radio memory, I think that the issue is flash memory, as we keep adding code, it will eventually overflow so the FW would no longer be able to fit.
FW that won't fit the flash. Sounds like our dynamic code loading in LUA scripts to work around a similar issue. Ever thought of externalising parts of non critical firmware and load onl up that what is needed when it is needed for to ge noncritical functions?
In essence every user screen could be kept on the card, as only one is visible and needs to be loaded at any time....
We are working on freeing up some flash. When the hardware driver update to the HAL is done we can enable LTO which frees some flash.
What we will do here, is use the direct index for telemetry sensors instead of the meta-index for sources. As JimB40 explained above, these will be different from each another.
Gotcha. I was aware that the indexes varied based on the hardware, but didn't know what the getSensorValue would not use the virtual (shifted) indexes. Makes sense then.
We are working on freeing up some flash. When the hardware driver update to the HAL is done we can enable LTO which frees some flash.
You guys are awesome! If I understand it correctly, you will update the hardware driver code in the Hardware Abstraction Layer of the code, and when that is done, you can enable the -FLTO compile option to do Link-Time Optimization, and this will enable the compiler to discard some unused segments from the final image, hence freeing up some flash memory?
In that case, we will just keep adding new features until it is full again 🤣
Exactly. Currently we have two different sets of drivers in the firmware, which makes it impossible to use LTO without a lot of warnings.
FW that won't fit the flash. Sounds like our dynamic code loading in LUA scripts to work around a similar issue. Ever thought of externalising parts of non critical firmware and load onl up that what is needed when it is needed for to ge noncritical functions?
In essence every user screen could be kept on the card, as only one is visible and needs to be loaded at any time....
Yes, I did think of it 😉 But I have neither the skills nor the time to do such a thing! But in principle, it would be cool to almost completely eliminate the GUI, add a Lua API that could do anything you need, and then write a complete GUI in Lua with dynamic loading of screens 🤓
I would like to add another UI "mode' whrere a set of lvgl UI elements is available for Lua and Lua creates the screen from those elements. Touch and scrolling would automatically work and reduce the Lua complexity by a lot
ETX 3.0!
I know that Peter voiced some concern about the available memory in BW radios, but I don't know how close to the limit we are today.
IIRC it's both flash and RAM, but we regained some of that back with img compression. It was more a gentle "don't go crazy with adding a dozen new functions"... a couple are fine, and ones like this are dearly needed.
FW that won't fit the flash. Sounds like our dynamic code loading in LUA scripts to work around a similar issue. Ever thought of externalising parts of non critical firmware and load onl up that what is needed when it is needed for to ge noncritical functions?
In essence every user screen could be kept on the card, as only one is visible and needs to be loaded at any time....
It's not quite that simple... and not as reliable IMO at this point in time as it means if there is a SD card issue you are hosed. And we are seeing those still every now and then, especially for the B&W targets. So it should stay in flash until we can get any of those are are still firmware related resolved. Plus it takes time for the user education as to ensuring using decent SD cards.
I think a long term goal is to flesh out the LUA API more though so that it can become a viable UI alternative, but it's a backburner project as there's a lot more foundational stuff that needs to be worked on first.
I love these balanced and open discussions on where to go when. Refreshing.
I think some will know why I post this.
I love these balanced and open discussions on where to go when. Refreshing.
I think some will know why I post this.
Do I want to know?
If you don't you don't need to. It's just a compliment then. :)
Is the sensorId still the ID you get from
getFieldInfo('XXX').id
, or would the IDs need to be pulled from getSensor(string) instead now?
@CapnBry Nope, as I wrote getFieldInfo('XXX').id
is sourceID that is different for the same sensor (like RSSI) for every radio type. On the other hand sensorID used in model.getSensor() implements index unification for telemerty sensors. It starts with 0 and ends with last discovered sensor. So far to check if user discovered telemetry sensor (and how many) I have hard-coded in LUA (for every radio) sourceID allocated for first telemetry sensor and incerement this value looking for nil to get last one.
sensorID unifies it across radio targets.
BTW @jfrickmann I think we should change it to start from 1 as LUA tables have this unique feature to start tables with index 1. So having index 0 makes ipairs use imposible.
model.getSensor() is not yet used widely so it shouldn't be a problem. In Companion first telemetry sensor is marked with index 1.
I like that proposal. Instead of having model.getSensorValue take either a string or sensor index argument, I would prefer to add a function model.getSensorId to get the sensor index from the name, just to encourage script writers to get the sensor index once, and use that instead of the name, for the sake of efficiency.
I know about efiiciency and was thinking about that. I've mimicked getValue() that can take both.
Don't know how you code but if my code is not effeciency sentensive I prefer to code like Bryan to maintain better code readability
local rcLinkSensor = model.getSensorValue("RSSI")
looks better for me than
local rcLinkSensor = model.getSensorValue(6)
Moreover if user redicover telemetry sensors they may appear in different order. So index 6 is no more pointing RSSI.
I will keep names lookup table in LUA anyway :)
Just let's keep naming scheme you already started with sources and switches that is model.getSensorIndex(sensorName)
I can also add an iterator function sensors similar to sources and switches if you want it.
Haven't noticed it. Is there a function that returns table of valid sources or switches?
The entire efficiency thing about getting the index first is completely irrelevant. It's something that has been said and nobody contested the validity.
It will never be of relevance in any normal lua scripts.
Last but not least if we clarify things.
What about aliases getValue() -> getSourceValue getFieldInfo -> getSourceInfo getSensor -> getSensorInfo
So we have:
for switches getSwitchIndex ( switchName ) getSwitchName ( switchIndex ) getSwitchValue ( switchIndex )
for sources getSourceInfo ( sourceIndex | sourceName ) getSourceValue ( sourceIndex | sourceName )
for sensors model.getSensorInfo ( sensorIndex | sensorName ) model.getSensorValue ( sensorIndex | sensorName )
You know I'm API naming junkie, so if you don't implement I won't kill myself :)
@CapnBry Nope, as I wrote
getFieldInfo('XXX').id
is sourceID that is different for the same sensor (like RSSI) for every radio type.
I know you said this, but it doesn't matter, because getFieldInfo can take a string and return the ID you use for subsequent calls. Nobody has ever hard-coded "RQly is 197" into their Lua because it would have a hard time of ever working on anyone else's handset where the sensors are in a different order or start at a different base number even, as you said.
The only point where getFieldInfo doesn't work is if you want to enumerate just all the telemetry sensors. Nobody should do this though right? Like no developer should scroll through the entire list "to see what sensors you have" so knowing the correct base ID doesn't matter. If you need to know if RQly is in the list, then you getFieldInfo('RQly').id
and don't care what the number it gives you back is-- it is assumed to be opaque and unique to that single execution lifetime. That's why I was saying getSensorId isn't really needed, because the ID that is returned always needs to be retrieved and cached on every execution so the developer should not know or care that RQly is 3 this run because it might be 19 next run.
But yeah if the getSensorValue uses different indexes, of course it needs a getSensorInfo to get the correct index to pass to it. I'm just not sure why there needs to be different functions at all for different classes of values. Keep getFieldInfo the same, only add getValue2 (with a better name) that still takes the whole range of IDs and returns the extra return values (and nil if it doesn't exist). I'm just not understanding why not using meta-indexes is a requirement?
Also to enumerate the sensor list (although nobody should ever be doing this, what are you looking for? Just query what you're looking for) you can't go through starting at 0 and going looking for nil to be returned, because the list can have gaps if the user has deleted sensors, you gotta go through the whole range and just skip the nils. Again, nobody should be doing this?
Don't forget there are language dependant default sensor names.
Yes, I would like to get a list of all available sensor id's.
A list of available sensors does make sense, if you want to provide a list of available sensors to the user. For that purpose you need to iterate through all sensors and get the their names.
@JimB40 , I agree in principle with everything you said about base index and function naming.
But, as I have argued before, I think that we should leave it as is, because a) I would rather spend my time flying planes and adding new features b) Writers of other scrips e.g. iNav and Yappu feel the same way c) Regular users do not appreciate when they upgrade and their scripts suddenly breaks because we have changed the API.
As for the zero-base indexing, it is stupid, but I feel it is better to at least be consistently stupid, so people don't have to remember which API functions use base 0 and which use base 1. So let's keep the stupid tradition alive 🤣
As for the naming, we can of course re-name and keep the old ones around, as you and Peter have proposed before. My only concern with that, is if we want to consume precious BW radio memory on that, and how much confusion it creates with the script writer community.
The only point where getFieldInfo doesn't work is if you want to enumerate just all the telemetry sensors. Nobody should do this though right? Like no developer should scroll through the entire list "to see what sensors you have" so knowing the correct base ID doesn't matter. If you need to know if RQly is in the list, then you
getFieldInfo('RQly').id
and don't care what the number it gives you back is-- it is assumed to be opaque and unique to that single execution lifetime. That's why I was saying getSensorId isn't really needed, because the ID that is returned always needs to be retrieved and cached on every execution so the developer should not know or care that RQly is 3 this run because it might be 19 next run.
You should always be careful assuming that you know what people want and don't want to do... What if I am making a script for live graphing sensor data, and I want to let the user choose the sensor from a list?
It is better to get the id once, and then use it going forward. Because every time you use the name, the C++ code does a linear search through all items with string comparison until it finds a matching name. And this can get CPU heavy if you do that on every call - especially on a list with many items like sources. For telemetry sensors, it literally searches hundreds of names by string comparison before it even gets to the sensors - every time! https://github.com/EdgeTX/edgetx/blob/911aec40b954a9db6128ebeb702136771913c303/radio/src/lua/api_general.cpp#L399-L481
But yeah if the getSensorValue uses different indexes, of course it needs a getSensorInfo to get the correct index to pass to it. I'm just not sure why there needs to be different functions at all for different classes of values. Keep getFieldInfo the same, only add getValue2 (with a better name) that still takes the whole range of IDs and returns the extra return values (and nil if it doesn't exist). I'm just not understanding why not using meta-indexes is a requirement?
This discussion is all about telemetry sensors, and getValue
and getFieldInfo
are about general sources of values; not just telemetry sensors. So if we want a new API function specifically for sensors, then we should stay within the name space for sensors to avoid the confusion of what to do if you call the function on another source, e.g. a stick input. Therefore sensor index instead of the general source meta-index. Just like the other model
functions getCurve
, getGlobalVariable
, getLogicalSwitch
etc.
Generally speaking, maintaining OpenTX compatibility won't work in the long run anyways. Maybe, as a mid to long term project, plan a modified UI API plus a general cleanup of the LUA API and then, for a specific Release, let us implement those changes. If this is not done in time, it gets harder and harder to clean up the existing mess, because more features are added and to keep it consistent old, maybe wrong, decissions have to be kept alive. Maybe 3.0 is a good release name for this, because it breaks API compatibility but has a cleaner interface and allows a better integration of future improvements. When we provide software with the changed API from early on in the development phase all script developers have enough time to adapt and we can get a nice, simultaneous, release for EdgeTX and the major scripts.
It is better to get the id once,
We're in agreement. I'm pretty sure I've said in every message I've posted that the developer needs to cache the ids and use the id only for calls to getValue.
What if I am making a script for live graphing sensor data, and I want to let the user choose the sensor from a list?
What if you want to put any other value on the graph too? Now you need specialized code for getting one kind of value versus another kind of value. Now your ID cache needs to know the name, ID, and type of value to use the correct getXXXValue variant.
If you want just a list of telemetry sensors then a pair of constants could be added that represent MIXSRC_FIRST_TELEM
and MIXSRC_LAST_TELEM
which would let you enumerate the list in a hardware-agnostic manner. You don't need a method that takes flat indexes instead of meta-indexes, you just need to know where the meta ranges are. I would think that is preferable to having a different function for every type of value you want to pull: getOutputValue
, getInputValue
, getMixerValue
, getSensorValue
, getSwitchValue
, getSliderValue
, getBacklightValue
, each with their own getXXXId
function too.
In the world of beautiful programming, I'd agree with having everything having their own specialized function and clean implementations working with data items of specific classes, but I don't even have the table library on B&W. Adding a dozen more pseudo-classes and functions to the Lua isn't moving in the right direction for these constrained environments when the same results could be achieved with a few constants and a getValue that uses the same code but has different return values for a much lower cost.
But also I am not writing it so none of this is my decision. I just think if any changes are being made that having telem sensors not return "0" the second they go offline or never existed in the first place would be a massive improvement which is sounds like is on the table, so I'll bow out of this discussion and leave how that happens to the implementer. 😁
Generally speaking, maintaining OpenTX compatibility won't work in the long run anyways. Maybe, as a mid to long term project, plan a modified UI API plus a general cleanup of the LUA API and then, for a specific Release, let us implement those changes. If this is not done in time, it gets harder and harder to clean up the existing mess, because more features are added and to keep it consistent old, maybe wrong, decissions have to be kept alive. Maybe 3.0 is a good release name for this, because it breaks API compatibility but has a cleaner interface and allows a better integration of future improvements. When we provide software with the changed API from early on in the development phase all script developers have enough time to adapt and we can get a nice, simultaneous, release for EdgeTX and the major scripts.
Funny to see one function change trigger such a nice discussion.
My take on it: edge TX has a viable scripters ecosystem, let's improve it. Opentx is dead for (script) developers, because it cannot keep up the pace of new radios anymore, let alone functional improvements.
Let's say thanks to Opentx, Cherish it's legacy, and go on without looking back.
Well actualy I do list all telemetry sensors :)
I had numerous inquires "why it doesn't work" so made simple module to show RAW telemetry data and which one sensors are needed for every RC link type to get all Toolbox features working properly.
List of sensor names for CRSF, Ghost, ELRS etc are harcoded in LUA. For now if sensor is not discovered or not getting value (I'm checking against 0) line is greyed-out. Once 'nil" and live are introduced I will be able to add 'not present' and 'sensor lost'.
It speeds up troubleshooting with users. To give you example. One of them created new model for CRSF duplicating one with D16. switched RC link type in model settings from D16 to CRSF but forgot to delete D16 sensors and discover new ones for CRSF. So app was showing kind of false data as both RC links share 'RxBt' only for D16 is voltage reported from RX and for CRSF is battery voltage feported from FC.
For quite a long time I'd rather gather feedback from users and ask for LUA feature basing on that, than ask 'imaginary' feature because I'd like to have one.
@jfrickmann Regarding indexing from 1 that's even funny if you look at https://github.com/search?l=Lua&p=1&q=model.getSensor&type=Code you will see that except my LUA-DEV-TOOLS and yours "JFutil.lua" only "FrSky SBEC.lua "FrSky GaSuite.lua" scripts are using model.getSensor function. Funny part is that we coded it properly from 0 while they coded it from 1 ommitting check for first sensor. Seems like RTFM doesn't work :)
I've also created a script to pre-load sensors in a fixed order, existing or not.
See https://repository.justfly.solutions/index.php?view=product&id=109:fixedsensororder
It is used to make things more like the opentx 2.1 way of working
Okay so recap we go easier path:
adding getSourceValue( sourceID | sourceName ) it returns list (value, status) value
adding getSourceInfo( sourceName) simple alias pointing getFieldInfo()
existing getValue( sourceID | sourceName ) and getFieldInfo( sourceName ) are left intact for legacy
then in ETX 3.0 or something we can phase out getValue and getFieldInfo
Checking all discovered telemetry sensors can be done with existing function
local avaliableSensors = {}
for sensorID = 0,59 do
local sensorData = model.getSensor(sensorID)
avaliableSensors[#avaliableSensors+1] = sensorData
-- storing sourceID for sensor
avaliableSensors[#avaliableSensors].sourceID = getFieldInfo( avaliableSensors[#avaliableSensors].name )
end
-- displaying all available sensors with values
for i, sensorData in ipairs(availableSensors) do
print( i, sensorData.name, getValue( sensorData.name ) )
end
-- displaying all available sensors with values via sourceID for speed
for i, sensorData in ipairs(availableSensors) do
print( i, sensorData.name, getValue( sensorData.sourceID ) ) -- via ID
end
BTW new getSourceInfo() function can use status for other source types. As ann example status=true for Logic Switch can indicate there is actually logic for this LS. But for now I don't want to complicate implemetation. Taking away your fly time @jfrickmann ;)
Agreed?
Maybe status could be an enum, to transport more information?
Maybe status could be an enum, to transport more information?
What other status values do you have in mind?
Is there general consensus for JimB40's proposal? @CapnBry @pfeerick @lshems ?
What other status values do you have in mind?
How about Invalid sensor - his sensor/id does not exists Valid - all good No data - no data available Maybe more, I did not use telemetry much yet.
The idea allows to extend the sensor states you can have without breaking compatibility, when we introduce more sensor states, because "valid" can stay the only state where the data is valid, all other states are error states. Worst would be an "unknown error" and updates scripts can show detailed information about the type of error. Also bools are hard to read, as true and false are not very descriptive.
Invalid source returns value = nil
and no/old data returns status = false
according to the proposal above. If something changes in the future, we could just add yet another return value, e.g. state
, with more info?
I'd be against the second return value being an enum instead of true or false/nil because it just makes code longer. The first return value value
is nil if the sensor doesn't exist, so that's "invalid sensor". The second return value status
True = valid all good. False/nil old or telemetry not streaming. If you make it a number or something else it makes everything into a long compare and what other status would you have?
local val, valid = getSourceValue(ids['RQly'])
if valid then XXXX end
-- compared to (ugh)
if valid == SOURCE_VALID then XXX end
-- inlines are even worse
lcd.drawText(0, 0, value, valid and 0 or INVERSE)
-- versus
lcd.drawText(0, 0, value, (valid == SOURCE_OLD or valid == SOURCE_INVALID) or INVERSE))
So the proposed sounds good. If you want to get more information like the Logic Switch has Logic, that could be in the table returned by getSourceInfo if that's needed since it is config versus runtime value.
For now you may be right, but, as I said, when a bool isn't enough anymore, every script has to be updated, again. So I think in the long run it saves work. Also, slightly longer lines don't hurt me. You can have a line break within your function call, if needed. As I am not doing anything with Lua in the radios and as I will not implement this, I can just offer my opinion. What you do with it, is your decision, not mine.
Be aware that in LUA any valua other than nil or false evaluates as true.
So " if 'a' then" is equal to " if true then ".
So you can combine the proposals into a three state model, with substates
Nil, false and true,. Where true may be any not nil and not false value. It could even be a function.
To keep it simple, return nil, false or string, with string being one of the states you need extra. Like new, last, estimate, whatever you guys can thing of as useful states.
getValue() regarding telemetry sensors is quite cumbersome as it returns 0 when:
value current source value (number). Zero is returned for: non-existing sources for all telemetry source when the telemetry stream is not received far all non allowed sensors while FAI MODE is active
As telemetry sensor is treated as any other available radio field, getting value of not discovered sensor will return 0. That's wrong IMHO as getting value of not discovered sensor (non-existing source) should return nil Now prior to getting sensor's value we have to check if sensor exists otherwise those two states are indistinguishable.
I don't know if we should change it (read: magic legacy) but then tele.getSensorValue() with proper implementation may solve problem
this refers #777 and #778