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.59k stars 338 forks source link

Change of telemetry precision effects value V2 in logical switches #4619

Open elekSENDER opened 8 months ago

elekSENDER commented 8 months ago

Is there an existing issue for this problem?

What part of EdgeTX is the focus of this bug?

Transmitter firmware

Current Behavior

A change in telemetry precision causes an unwanted automatic shift of the comma in logical switches at the value V2.

initial state: screen-2024-01-23-191254 screen-2024-01-23-191316

change precision: screen-2024-01-23-191400 go to logical switches: screen-2024-01-23-191557

and again change precision: screen-2024-01-23-191624 go to logical switches: screen-2024-01-23-191653

Expected Behavior

no change of V2 in logical switches after adjusting telemetry precision

Steps To Reproduce

look at screenshots

Version

2.9.3

Transmitter

FrSky X10 / X10S (ACCST), RadioMaster TX16S / TX16SMK2

Operating System (OS)

No response

OS Version

No response

Anything else?

No response

frankiearzu commented 8 months ago

Same behavior in 2.9.2 and 2.9.0

All the sensor values are internally stored as integers. The PREC affects the display only of the sensor, by multiplying/dividing by a power of 10 from the original PREC. For example, it the sensor at discovery is 5.43 (543 PREC2), if we change the GUI to PREC1 it will do a division by 10. i.e, 5.4 (PREC1) to display it.. The value of the sensor stored has always been 543 PREC2.

In the case of the logical switches, looks like the GUI display for the constant inherits the display PREC from the Sensor. Since the original value started with 7.00 (internally 700 PREC2), When the sensor is changes to PREC1, it will be displayed as 70.0 since the constants internally did not change (internally is still 700).

This probably is not easy to fix, since it could affect more things besides the GUI, but also in the evaluation of expressions for LS.

One way to solve the problem is to store the PREC of the original sensor (not the display PREC) together with the value at the time the LS was created, and do the conversion for display from the original PREC to the new PREC for display and expression evaluation. (backward compatible).

Another solution would be to store ALL the values in a fixed PREC, lets say PREC2, and convert to the display PREC just for display and evaluation (not backward compatible).

Could affect Companion too.

3djc commented 8 months ago

I would say works as designed and intended. Obviously changing the PREC of a sensor will require a review of associated everything, but I would think that's to be expected (and there would be now way of knowing if you want the value updated or not anyway, both are equally plausible.)

pfeerick commented 8 months ago

I agree that changing the sensor precision naturally requires you to update/validate it again anywhere it is referenced. I'm not so sure that I agree about the "value being updated" ... i.e. when you set it to 5V as the comparison value in the logical switch, I'm not sure you would expect that to change when the precision of the sensor changes from 5 -> 5.0 -> 5.00 ... but if this is the established behaviour all this time, it's probably too late to change it without some kludge layer in-between.

3djc commented 8 months ago

The issue is your 5v in prec0 is a value of 5 from sensor, 5.0 is 50, 5.00 is 500 (using the same sensor). Changing prec effectively divides.

pfeerick commented 8 months ago

Yes... but in reality, should that configured sensor precision (as far as the value displayed to the user) not then have been honoured, even if it meant a higher or lower value internally (which is what is used in the calculations). After all, as the end user, all you care about is where the decimal place is, right?

elekSENDER commented 8 months ago

Just for your information and understanding, I would like to tell you the story behind it:

The BEC voltage alarm is very important for me on my helis. This type of alarm is “land asap and then see what happened”, if the BEC voltage drops below a certain value it means something is very wrong. Due to a change in my BEC supply, I wanted to see the 2nd digit after the decimal point when displaying the BEC voltage. So I did what is described in the EdgeTX User Guide under Telemetry/Sensor Configuration Options:

Screenshot 2024-02-12 at 11-39-08 Sensor Configuration Options - EdgeTX User Manual

With this change to a display parameter, I would never have expected that somewhere hidden in logical switches a value parameter would be automatically changed from 7V to 0.7V. This means that in reality I have disabled a critical alarm. I found this by accident and was really surprised about it.

I understand that it is obviously difficult to solve this "issue" and perhaps it is not worth working on for various reasons.

But I don't agree that this should be undocumented "intentional behavior". ;-) At least a note in the EdgeTX User Manual could give a chance to not overlook this essential connection between these parameters.

3djc commented 8 months ago

Ok, you cannot display value that isn't there, and we are not hiding anything or rounding any value.

If the sensor is not in prec2 by default, it means the sensor is not been sent with 2 decimal precision. Trying to force that isn't magicaly going to increase its precision.

This setting cannot be used without changing sensor side too, and is mostly there for those developing their own sensor. It is not what you think it is

pfeerick commented 8 months ago

It is not what you think it is

Is that perhaps because "precision" is the wrong descriptor here? i.e. precision != order of magnitude.

3djc commented 8 months ago

I won't disagree with that !

frankiearzu commented 8 months ago

Just adding comment to

image

At least in sensor, precision also describes how the value is coming from the telemetry feed. In telemetry, everything is an integer.. so a value of 7.12 really comes as 712 PREC2, or 1.3 really comes as 13 PREC1.

At display, we can choose any precision we want, that trigger a conversion from the source precision to the to the display precision. In the case of the sensor, we always have the reference to the source precision, that's why it works well in the GUI, but anywhere else where we type constants.. the original precision of the constant is not stored. That is the case with LS expression constants.

Last night I was looking to see if the LS GUI and Expression evaluation has access to the "source" precision, or only the display precision of the sensor. The source precision are stored in the telemetry protocol specific internal tables, and not in the runtime data shared externally.

I haven't dig more, but probably there are other areas who might have similar issue with constants.