Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

telemetry voltage has wrong factor with Multiprotocol OMP #4572

Closed
1 task done
rockclimber128 opened this issue Jan 26, 2024 · 9 comments · Fixed by #4583
Closed
1 task done

telemetry voltage has wrong factor with Multiprotocol OMP #4572

rockclimber128 opened this issue Jan 26, 2024 · 9 comments · Fixed by #4583
Labels
bug/regression ↩️ A new version of EdgeTX broke something

Comments

@rockclimber128
Copy link

rockclimber128 commented Jan 26, 2024

Is there an existing issue for this problem?

  • I have searched the existing issues

What part of EdgeTX is the focus of this bug?

Transmitter firmware

Current Behavior

The transmitter is bound to an OMP M2 V2 Helicopter and the telemetry is working in general.
But both telemetry values for Battery Voltage A1 and A2 showing wrong values. The values are too high by a factor of 10.
The preset Ratio is set to 13,2.
As example the value is shown as 112,0V instead of 11,2V
As workaround you could use a Ratio of 1,3 but then you lose accuracy.

Expected Behavior

I've cross checked with my second transmitter running opentx 2.3.15 and the same multiprotocol module. There it is working as expected and the values are correct.

Steps To Reproduce

It seems to affect only the OMP protocol.
I've cross checked with EdgeTX 2.9.3 and a different Receiver and protocol with the multiprotocol module (R84 FrSky X LBT(EU)) There it is working also as expected and the values are correct.

Version

2.9.3

Transmitter

FrSky X9D+

Operating System (OS)

Windows

OS Version

No response

Anything else?

No response

@rockclimber128 rockclimber128 added bug 🪲 Something isn't working triage Bug report awaiting review / sorting labels Jan 26, 2024
@theCommaLlama
Copy link

theCommaLlama commented Jan 26, 2024

I am also experiencing this with a Radiolink R8F with an MT12

@pfeerick pfeerick added bug/regression ↩️ A new version of EdgeTX broke something and removed bug 🪲 Something isn't working triage Bug report awaiting review / sorting labels Jan 27, 2024
@frankiearzu
Copy link
Contributor

frankiearzu commented Jan 27, 2024

What caused the problem:
Fix for #4083
In 2.9.3, I removed a piece of code that seems out of place, and was affecting the scaling (x10 for Prec2, /10 for Prec0, Prec1 was OK) in other protocols (DSM, frsky_X).. by removing the code it fixed that protocols. Now it broke frky_D telemetry protocol (OMP M1 and probably other who use it).

@Spring7186 reported that his telemetry refresh stopped refreshing or "froze" refreshing telemetry after trying to change the "ratio" to adjust the value.

Temporary work around while we do the fix: change ratio from "13.2" to "1.3".

I think i found the problem in the "frsky_d" code, but don't have a OMP receiver to test it.

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.

image
image

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) ???
image
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. Also the A1..A4 sensors are pre-populated with "ratio=132" for frsky_D.
This code is shared for ALL protocols, so a workaround specific for a OMP/frsk_d protocol should not be here.

Did the the change so that the Prec of Volatage matches the definition of the sensor.. Looking at other protocols, they use the parameter Prec=255 to mean "use the sensor definition" value, will try that too.
image

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 refreshing.

Firmware with this change:
https://github.com/frankiearzu/DSMTools/tree/main/EdgeTx_Firmware

ZORRO: zorro-edgetx-v2.9.3-OMP-Fix.bin
X9D+: x9d+-edgetx-v2.9.3-OMP-Fix.bin
X9D+ 2019 Rev: x9d+2019-edgetx-v2.9.3-OMP-Fix.bin

@Spring7186
Copy link

Spring7186 commented Jan 27, 2024

Dear Francisco, finally I found some time to do some tests, the voltage measurements are now indicated correctly! On next firmware's releases should I do something, or simply launch that installations? Thank you very much again!
image
image
image

@jtaylor2
Copy link
Contributor

After updating to 2.9.3 all of my Frsky D8 voltage telemetry is incorrect. D16 seems to be OK.
A 3 cell battery at 11.3 shows 113.0 on a sensor with a ratio of 17.4. I didn't spend a lot of time, but couldn't find a combination of ratio and precision that gave the correct value. I have quite a few planes with D8 receivers and rely on voltage telemetry so downgraded to 2.9.2.

@frankiearzu
Copy link
Contributor

frankiearzu commented Jan 28, 2024

@rockclimber128 can you try the firmware for X9D+ (or X9D+ Revision 2019) from here:
https://github.com/frankiearzu/DSMTools/tree/main/EdgeTx_Firmware

Can you let me know if that fix the problem.. Don't have an OMP/frsky_d to test the fix, but another person on a ZORRO reported that this change fixes the problem. Have one FrSky D8 RX in the way to test myself.

@jtaylor2: What TX are you using?? if you change the "ratio" field from 17.4 to 1.7 will be close as a temporary fix.

Once we validate the fix, this change will go into the next release (2.9.4 or 2.10)

@jtaylor2
Copy link
Contributor

jtaylor2 commented Jan 28, 2024

I'm using a Radiomaster tx16s for edgeTX (my x9d is still on openTX). I compile my own code on openSUSE so if you give me a diff or point me to a PR I can test a fix.

Changing the ratio as a temporary fix is not practical for several reasons. Changing 20 or 30 A1, A2 (is A3 and A4 also affected?) sensors is no fun and the error is too great. For example:

A 3s battery at 11.3 volts on 2.9.2 A2 ratio 17.4 reads 11.3 volts - aprox. a 30% state of charge
                              2.9.3 A2 ratio  1.7 reads 11.0 volts - aprox. a 10% state of charge

A1 reads 5.3v with a 13.2 ratio on 2.9.2 and 5.0v with a 1.3 ratio on 2.9.3

As this bug, if not noticed, disables low flight pack and receiver pack voltage warnings it is in my opinion a pretty serious bug for a dot release and there should definitely be a 2.9.4 or perhaps even 2.9.3 taken down (or at least a warning put in the 2.9.3 release notes).

@frankiearzu
Copy link
Contributor

@jtaylor2
For the A1..A2, the fix is /radio/src/telemetry/frsky_d.c, line 74-75, The last parameter was Prec=0, but should be Prec=1 (1 decimal place) to match the sensor definition a few lines below.

image

A3..A4 i think are not affected, since the "ratio" is not populated by default ("ratio=0" or --) ... but in 2.9.2, if you set a "ratio" different than 0 on ANY Prec2 (2 decimal places sensors) on ANY protocol, was erroneously doing a times 10, that makes them really hard to tune (that was what we intended to fix originally in 2.9.3).

Do you have that sensor for A3..A4 to to test? if you, i think the default "ratio" is 0.

On the TX16, there was also a display bug in 2.9.2 (and before), "ratio" should have 1 decimal place to be consistent with Companion and B&W ratios. So what is displayed as "ratio=255" (TX16 2.9.2) is displayed as "25.5" (TX16 2.9.3). Is just the display, the internal value is stored without the decimal place in both versions, and the internal math is the same.

Just uploaded a RM TX16S version of the fix if you want to try it:
https://github.com/frankiearzu/DSMTools/tree/main/EdgeTx_Firmware

A few people has confirmed that it works, so will start working on the PR

@rockclimber128
Copy link
Author

Dear Frankie,
i just flashed the firmware "x9d+-edgetx-v2.9.3-OMP-Fix.bin". It fixed the problem. Many thanks for the quick solution to the problem!
I have been using opentx until now. I have been watching Edgetx for some time, now is the time to change!
Thanks again!

@jtaylor2
Copy link
Contributor

@frankiearzu I made the changes you indicated to frsky_d.cpp and can verify that this fixes the problem with the D8 A1 and A2 sensors. As to the A3, A4 sensors I don't think I have any installed, but I got to thinking that they are Sport and not D8 anyway.

-      setTelemetryValue(proto, D_A1_ID, 0, 0, packet[1], UNIT_VOLTS, 0);
-      setTelemetryValue(proto, D_A2_ID, 0, 0, packet[2], UNIT_VOLTS, 0);
+      setTelemetryValue(proto, D_A1_ID, 0, 0, packet[1], UNIT_VOLTS, 1);
+      setTelemetryValue(proto, D_A2_ID, 0, 0, packet[2], UNIT_VOLTS, 1);

As a side note, I don't use Spektrum receivers but several of my friends do (and ask me to help them) so I really appreciate what you're doing with forward programming and Spektrum telemetry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/regression ↩️ A new version of EdgeTX broke something
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants