fix: Properly scale PREC2 sensor values using "ratio"

[frankiearzu]

Fixes #4021

History:
During the transition from 2.8.5 to 2.9.0, there was several fixes to DSM sensor values. The values itself are correct, but it exposes a poblem when using "ratio" to adjust the value of a sensor.

The "ECUR" (ESC Current) sensor change from been in "mAh" (prec=1) to "A" (prec=2). The change of prec exposed the scaling problem with ratio, The value in 2.8.5 got 10x bigger in 2.9.0 when using ratio.

Summary of changes:

• Simply commenting out code that doing the x10 when prec==2.
• The other problem was when prec==0, it was converting it to prec=1, effectively doing a / 10.

How ratio works is that the value is multiplied by ratio/255. So using a ratio of 255 means no change.. value of less than 255 scale down the number, and values greater than 255 makes the value bigger. This is not documented in the manual.. this was by looking how the code works.

[pfeerick: added for clarity] As a possible future enhancement... I think we should consider making it a percentage. so a value of 100% means same value, > 100% scales up, and < 100% scales it down. This will be more understandable to more people. There is probably a historical reason for the 255, but can't find an explanation.

Testing:
Used 3 different sensors with prec=0 (integer), prec=1 (one decimal point), and prec=2 (2 decimal points). Starts by using ratio 255.. should give the same value. lowering the number makes the sensor value smaller, and increasing it makes the value bigger.

[pfeerick]

@3djc Your thoughts?

[3djc]

There are some merit to this, but this will break everyone setup. I would suggest including this in a bigger rework of sensors arithmetic

[raphaelcoeffic]

We could rename the attribute internally o allow for proper conversion?

[3djc]

We could, but I would still recommend to do it well one time rather than several steps, each requiring its own migration step

[frankiearzu]

@3djc @raphaelcoeffic @pfeerick
Sorry about mixing discusion of a feature request here, i think is better to to just think of this specific fix for right now.
i can create a feature request to change it to "percent" that will impact logic and GUI.

This bug only happens when trying to adjust a sensor value with prec=2.. The code clearly do a 10x that is not needed.

By looking at the current code, most of the ones who uses prec=2 (.00) are (Heading, Altitude, Gs) that are unlikely to be adjusted, but current ("A") and voltage ("V") are usefull to adjust.

For the Battery voltages who has prec=2 (.00) and small value, in those case, is nearly imposible to use "radio" to adjust due to the 10x bug (no fine tuning), most people will only use "offset" to get the right voltage.

Try it yourself in 2.9.0 or previos 2.8.x with the basic RX voltage sensor (A1,A2) that is very common and likely to be prec=2. "ratio" is not usable as it is. since it jumps to much for fine tuning.

I think is a bug that is still woth to fixing.

[rdeanchurch]

Specifically ECUR is unusable by me at the moment. I'ld lke to see it fixed. Migration is not an issue if it is not usable for me and others the way it is.
Better to have to change twice or more than have an unusble feature which ends up affecting Watts, and more importantly, mAh/consumed battery..

[pfeerick]

@frankiearzu Can you be more specific on exactly what is needed to reproduce this? As I'm not seeing a shift in the ratio scaling between 2.8.5 and 2.9.1 (or main) with TX16S, MPM, DSMX and a LemonRX telemetry receiver. i.e. with a ratio of 295, changing the precision for the A2 custom voltage sensor simply changes the value from 62V -> 62.2V -> 62.24V, and when I switched from 2.8.5 to 2.9.1 there was no change in the value - it didn't jump 10x or anything. I think I know what you are saying should be happening, but I'm just not seeing it here, so I must be missing something...

[frankiearzu]

@pfeerick your RX voltage sensor at 62v is probably already scaled (way to high for RX voltage), should be 5-6v range.. Start with Ratio "0" that is no ratio.. you will get something in the range of 6.00v.. then change the ratio to 255. that should be the same value, but using the ratio buggy code, the value is 10x. A radio of 295 should give you just a bit higher.

Using the official 2.9.1. Tested with various DSM RX (AR631, Lemon-RX), but also happen with FrSky as long as the sensor is PREC2 (.00)

[rdeanchurch]

As far as I can see this was corrected in EdgeTx v2.9.2. ECur show the correct units and the value is nearly identical to what my wattmeter reads.

[frankiearzu]

@rdeanchurch .. the problem is using the "ratio" to adjust the value a bit up or down. the bug report confused it a bit with "ECUR", but is really with any sensor of prec2 (2 decimals .00) that is doing a 10x once you start using the "ratio".

A value of ratio = 255 should give you the same number. The formula is new_value=old_value *ratio/255
but there are some code that for an unknown reason, is doing that x10 there.

[pfeerick]

Yes I see that. But is universal. Not specific to prec2. And, I get near the "zero" value at 25 on tx16s. Which is different on b&w... That gets it at 2.5 (note the decimal).

So firstly the colorlcd and b&w ui needs to unified so that they match... I would suggest there is a fault in the colorlcd ui given the b&w ui is mostly unchanged.

Then, need to see the ramifications of this change on b&w also, as this is underlying telemetry code being changed.

[frankiearzu]

I think that code is shared for both color and black &white radios.
If you see the code that I removed. You can see where the 10x is.. the other part of the code is also not too clear.. I think for me works fine with sensors of PREC1 (.0), but for integers (PREC0) was also moving the decimal point by internally changing to PREC1 (didn’t do much research of this one).
Removing the code did solve both problems.
I really don’t see a point of trying to scale an integer number.. only the ones with decimal places.

Have a b&W radio.. will play more with it during the holidays.

[pfeerick]

Two things need to be considered. Either this is a very old bug (like eight years old since that is when this particular bit of code was touched last) or the fault is somewhere else - whether that be the colorlcd ui or something else.

That isn't to say this can't be improved.. we're no longer on AVR with limited ram, eeprom, flash, etc, so can improve on the idiosyncrasies in code done to optimize for that platforms limitations.

[frankiearzu]

@pfeerick Did a few tests on both TX16 (Color) and QX7 (B&W). Looks like the telemetry code that do the sensor math is shared.. But found an inconsistency in the UI.

On the TX16, Ratio is an integer. On the QX7 as well as Companion, is a float with 1 decimal (internally is still an integer, is just displayed with 1 decimal). So on the TX16 a ratio value of 255, in companion/QX7 shows as "25.5" (25,5 for EU) and the sensor value scales properly with the fix. This UI inconsistency is reported also in bug #2665.

The easiest to make all UI consistent will be to make the TX16 UI also a float (1 digit) and use 25.5 as reference.. Values less than 25.5 will scale down, and values over 25.5 will scale up. Is also backward compatible, since the model data is saved as interger, only the display shows as decimal-1.
Not very familiar with UI code but will give it a try to fix it.
The other with more changes will be make it all an INT but that will require changes in B&W UI and Companion (not familiar with that code either, but if that is the way to go, will try to fix or ask for help)

The BUG scaling behavior makes sense when looking of how the code was (Only PREC1 did not change)

[frankiearzu]

Screenshot from bug: #2665.

[pfeerick]

That matches up with what I said back at #4083 (comment), so glad it wasn't just me ;)

I believe the instance ID being off by one was fixed since that issue was reported, so it's only the ratio precision remaining still.

See if changing

auto edit = new NumberEdit(paramLines[P_RATIO], rect_t{}, 0, 30000, GET_SET_DEFAULT(sensor->custom.ratio));

to

auto edit = new NumberEdit(paramLines[P_RATIO], rect_t{}, 0, 30000, GET_SET_DEFAULT(sensor->custom.ratio), 0, PREC1);

at

edgetx/radio/src/gui/colorlcd/model_telemetry.cpp

Line 674 in fa12b27

auto edit = new NumberEdit(paramLines[P_RATIO], rect_t{}, 0, 30000, GET_SET_DEFAULT(sensor->custom.ratio));

fixes it.

I think it should do the trick, as it's really only a UI presentation error from the looks of it.

[frankiearzu]

Done fixing the UI... Thanks for the hint.. tested on my TX16 and shows now as PREC1. There is another reference to custom->ratio for a field "P_BLADES", but did not want to touch it, since it does not show in my UI.

[pfeerick]

That field being shown is dependent on the unit field being set to 'rpm'. It should probably be a integer rather than float number in that case, so should be right as it is.

[pfeerick]

Behaviour of ratio is now consistent between B&W and color UI... they are both shown as a decimal value, which is also consistent with how shown in Companion. Ratio scaling does not change when changing precision of value, so LGTM. Merging into main first, and then 2.9.3 after a delay for feedback.

Then we can discuss using a percentage in the future to get rid of this 13.2 == 0 nonsense... 🤭

[Spring7186]

Hello everyone, never an issue since updated from 2.9.2 (as all previous releases as well, starting from 2.7.0) to 2.9.3 in my Radiomaster Zorro, which now currently shows battery voltage value received by telemetry multiplied by 10x (for example, original 7.45V is becoming now 74.50V).
Note:

1. Trying to delete all sensors and rescan all again doesn't solve this issue;
2. Trying to modifiy parameters such "ratio", "precision", "offset" cause to "freeze" the value readings to last measured one (it stop working at all);
3. Downgrading to 2.9.2 obtain readings to works properly;
4. I always displayed these values setting two decimal points.
   Hope I was clear, thank you in advance for taking into consideration

[frankiearzu]

@Spring7186 What Protocol are you using?, What "ratio" value are you currently using?? If you set the "ratio" to 0 (or --), that will show the raw value coming from telemetry.

Should be the opposite, in 2.9.3 we REMOVED a multiplication 10x. I have talked to some people, and what they have reported is a voltage value 10x LOWER in 2.9.3 compared to 2.9.2 if they have used "ratio" value in 2.9.0. That is solved by adjusting the "ratio" again (usually by setting "ration" to 0, or "ratio" 10x the old value, for example, they had "ratio" of 1.3 in 2.9.2, and change it to 13.0 in 2.9.3 to get close to same result ).
If no "ratio" value (0), everything is the same, raw value from telemetry is not changed.

Some protocols already have some "ratio" pre-setup by default, All that i found was for 1 decimal point (the bug did not affected), the other 2-digits where RPM that needs adjusting anyway. Also could be that a protocol-telemetry specific code was adjusting due to the old bug. That's why need to know the sensor name and protocol to look at the code.

Can you do a screenshot of your sensor setting in 2.9.2 and 2.9.3? Pick one voltage sensor, and show us the data on the screen where "ratio" is. You don't need to re-discover sensors.. just change the "ratio".

The "freezing" is new, didn't notice that on my testing. it is it just a freezing of the GUI or the entire radio??

[Spring7186]

Dear Frankiearzu thank you again for your reply, here to you the screenshots as requested. Protocol is OMP and the freeze involve the telemetry only, not the radio.

[frankiearzu]

@Spring7186

I think OMP uses "frsky_d" telemetry protocol.. The sensor who matches your ID (00F1) is "A1" and by default is 1 decimal place. I can see in the code where they populate the ratio=13.2 by default.

I cannot find a sensor named "BAT1" in the code. Is that a name that you edited? Only "A1" and "A2" and "RxBt" is what I can find. Did you change the precision from 1 decimal to 2 in the GUI setting? Or was that how it came when you discover new sensors??

[frankiearzu]

UPDATE: I think i found the problem in the "frsky_d" code.

Since could not duplicated with DSM or FrSkyX, has to be something specific for FrSkyD.

Found an inconsistency on passing the prec when setting up the value with the call setTelemetryValue().. last param is prec.

This inconsistency of prec in the call and prec in the definition of the sensor affects later:
setTelemetryValue() calls getValue() who calls convertTelemetryValue using the prec parameter and the prec definition of the sensor. That will force a conversion from Prec0 to Prec1 that is a x10 inside the convertTelemetryValue()

Why is was working before in 2.9.2 (and before) ???

That code removed was fixing this 0 prec, and was overriding the Prec0 to be always Prec1 when using "ratio". Prec1 to Prec1 stays the same.

Did the the change so that the Prec of Volatage matches the definition of the sensor:

Can you try this firmware: Is 2.9.3 and the fix for OMP. I think that will eliminate the 10x that you are seeing. Still can't explain the freeze of telemetry.

https://github.com/frankiearzu/DSMTools/blob/main/EdgeTx_Firmware/zorro-edgetx-v2.9.3-OMP-Fix.bin

[Spring7186]

@Spring7186

I think OMP uses "frsky_d" telemetry protocol.. The sensor who matches your ID (00F1) is "A1" and by default is 1 decimal place. I can see in the code where they populate the ratio=13.2 by default.

I cannot find a sensor named "BAT1" in the code. Is that a name that you edited? Only "A1" and "A2" and "RxBt" is what I can find. Did you change the precision from 1 decimal to 2 in the GUI setting? Or was that how it came when you discover new sensors??

Yes, I modified tag in BAT1 (ex A1) and precision from 0.0 to 0.00. Could be cause any issue? Thank you

[Spring7186]

UPDATE: I think i found the problem in the "frsky_d" code.

Since could not duplicated with DSM or FrSkyX, has to be something specific for FrSkyD.

Found an inconsistency on passing the prec when setting up the value with the call setTelemetryValue().. last param is prec.

This inconsistency of prec in the call and prec in the definition of the sensor affects later:

setTelemetryValue() calls getValue() who calls convertTelemetryValue using the prec parameter and the prec definition of the sensor. That will force a conversion from Prec0 to Prec1 that is a x10 inside the convertTelemetryValue()

Why is was working before in 2.9.2 (and before) ???

That code removed was fixing this 0 prec, and was overriding the Prec0 to be always Prec1 when using "ratio". Prec1 to Prec1 stays the same.

Did the the change so that the Prec of Volatage matches the definition of the sensor:

Can you try this firmware: Is 2.9.3 and the fix for OMP. I think that will eliminate the 10x that you are seeing. Still can't explain the freeze of telemetry.

https://github.com/frankiearzu/DSMTools/blob/main/EdgeTx_Firmware/zorro-edgetx-v2.9.3-OMP-Fix.bin

Sure, I'll test it asap! Yes, it's very strange that the issue was happened only since 2.9.3. One question: I'll can upgrade future release normally, or I'll find only customized firmware for OMP protocol? I can assume no, it is just for confirmation.

[frankiearzu]

Once we confirm that the fix works, it will make it to the official releases in the future. Will check more protocols for this type of scenarios. Before i only checked for sensor definitions (who are correct)

[pfeerick]

Looks like this has an issue to track now... #4572