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

Fix the bug that some sensor values are displayed incorrectly #3386

Merged
merged 7 commits into from
Apr 19, 2023

Conversation

Xy2019
Copy link
Contributor

@Xy2019 Xy2019 commented Mar 22, 2023

Fixes #3196

Summary of changes:

@pfeerick pfeerick added bug 🪲 Something isn't working telemetry 📶 labels Mar 22, 2023
@pfeerick pfeerick added this to the 2.8.2 milestone Mar 22, 2023
@pfeerick pfeerick self-assigned this Mar 22, 2023
radio/src/telemetry/flysky_ibus.cpp Show resolved Hide resolved
radio/src/pulses/afhds3.cpp Show resolved Hide resolved
@richardclli
Copy link
Collaborator

I think it is good to go, since Flysky knows best about how to decode their telemetry data.

@pfeerick
Copy link
Member

pfeerick commented Mar 26, 2023

I think this is mostly good to go, but can you just check a couple of final details first though. For each radio and when changing firmware I did sensor rediscovery so not have issue with changes to sensor IDs or handling, etc. I'm not so concerned with the MPM changes as that is suggestive there is probably something wrong in that handling, but at the same time this PR is breaking stuff there.

For AFHDS3 (with a FTr12B, FS-CAT01 and FS-CVT01), on the EL18 (ignore the names being repeated on the right side, that is a bug has been fixed), I get the following, where Tmp1 is an invalid value.

Regarding table of telemetry issues in #3196, should RNse unit be dBm or dB?

For my own reference, is % a valid unit for RQly?

image

For AFHDS2A (with a FS-iA6B, FS-CAT01 and FS-CVT01),

  • on the NV14, I get the following, with A1 present and correct unit, Alt is completely wrong, and Pres seems to be the correct value
    image

  • and finally, on the TX16S and MPM, I get the following (ignore the highlighting of TRSS) - A1 is missing and there is a now 0100 sensor , perhaps with wrong unit since it was 5.20V when A1 was present? Tmp2 is completely wrong (is that even supposed to be here - it was there before this PR and seemed to be a valid value for ambient temperature), Pres is completely wrong, Alt is negative.
    image
    image

@Xy2019
Copy link
Contributor Author

Xy2019 commented Mar 27, 2023

1.The temperature error of the FS-CAT01 sensor is too large (or completely wrong), and the data analysis of TX is no problem (if possible, we will not let the temperature value be displayed).
2.The unit of RNse (Noise) is dBm, the same as RSSI.This is my mistake:worried:.
3.The signal quality range of RQly is 0-100, which is a value converted by RSSI. We hope that the unit of RQly is UNIT_RAW.

@pfeerick pfeerick modified the milestones: 2.8.2, 2.8.3 Mar 30, 2023
@richardclli
Copy link
Collaborator

richardclli commented Apr 12, 2023

@pfeerick I helped to do the following changes:

  1. RQly use %
  2. RNSe use dbm
  3. 0100 problem solved

Alt problem seems scaling problem, maybe the sensors used in AFHDS3 and AFHDS2 receivers should use different scaling, however, not sure if this can be done with a common telemetry coding. I asked Flysky to check again.

Please see if there are anything else.

@richardclli
Copy link
Collaborator

richardclli commented Apr 14, 2023

I checked PL18 and other references, I think RSNR should be db and not dbm

@richardclli
Copy link
Collaborator

@pfeerick Please test again.

@pfeerick
Copy link
Member

Argh! Just pulled this up to leave feedback and it's changed again! lol

@pfeerick
Copy link
Member

pfeerick commented Apr 15, 2023

Ok, with latest state of this PR:

On the NV14, RQly seems to be following RSNR value, and so is wrong.

On the EL18, Tmp1 was around -15.0C, when the actual ambient temperature is around 24C, so I don't think the 40C temperature offset applies to AFHDS3. Can you validate and commit this if it works fine for you? It has put my temperature reading back into acceptable range (i.e. one thermometer says it's 24.3C and the transmitter is saying 26.3C now).

diff --git a/radio/src/telemetry/flysky_ibus.cpp b/radio/src/telemetry/flysky_ibus.cpp
index 3b8c7e199..8176f3406 100644
--- a/radio/src/telemetry/flysky_ibus.cpp
+++ b/radio/src/telemetry/flysky_ibus.cpp
@@ -280,8 +280,7 @@ void processFlySkyAFHDS3Sensor(const uint8_t * packet, uint8_t len )
   {
     if (sensor->type != type) continue;
 
-    if (sensor->unit == UNIT_CELSIUS) value -= 400; // Temperature sensors have 40 degree offset
-    else if (sensor->unit == UNIT_VOLTS) value = (int16_t) value; // Voltage types are unsigned 16bit integers
+    if (sensor->unit == UNIT_VOLTS) value = (int16_t) value; // Voltage types are unsigned 16bit integers
 
     setFlyskyTelemetryValue(type, id, value, sensor->unit, sensor->precision);
     return;

On the TX16S /w MPM, Pres is wrong, Alt is wrong, Tmp1 is reported as Tmp2 and is wrong. Other telemetry seems fine though. I'm on the fence as to how important this is atm, as it can be flagged as a known issue and work in progress with flysky - i.e. stick with an earlier 2.8.x build if using MPM with AFHDS2A.

I am curious about Alt(itude) generally though. I'm pretty sure the previous behaviour was coded so that it would be relative to takeoff, i.e. 0 and then go up or down. whereas now for me it starts off at about 48m and goes up in 0.5m increments. Is it supposed to be an approximation relative to sea level or something?

@richardclli richardclli modified the milestones: 2.8.3, 2.9 Apr 19, 2023
@pfeerick pfeerick merged commit 8b40fb7 into EdgeTX:main Apr 19, 2023
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🪲 Something isn't working telemetry 📶
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EL18 bind with INr6-HS,Telemetry data problem
3 participants