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.49k stars 313 forks source link

Simulator RxBt+/- strange behaviour/Error #5214

Open derelict opened 5 days ago

derelict commented 5 days ago

Is there an existing issue for this problem?

What part of EdgeTX is the focus of this bug?

Simulator

Current Behavior

Strange Values for RxBT+/- and current

Expected Behavior

RxBT+ and RxBT- holding/showing correct values

Steps To Reproduce

See Video/GIF

Version

2.10.1

Transmitter

Other (Please specify below)

Operating System (OS)

Windows

OS Version

No response

Anything else?

companion_YumUjhFpqV

pfeerick commented 5 days ago

Can reproduce two issues on 2.10.1 and recent main simulator with colorlcd

The latter issue is common to both colorlcd and B&W, not sure about the former. And don't have a radio handy so can't confirm whether simulator or firmware specific. VFAS and Curr values appeared to be working correctly on a quick test.

philmoz commented 5 days ago

This looks to be by design - if the battery level goes above the max recorded value, then the battery was changed / charged and the old min value is no longer valid:

  else if (newVal > valueMax) {
    valueMax = newVal;
    if (sensor.unit == UNIT_VOLTS) {
      valueMin = newVal; // the batt was changed
    }
  }
pfeerick commented 5 days ago

Hm... has been like that for eight plus years also... last change was by Bertrand :hand_over_mouth: https://github.com/EdgeTX/edgetx/blob/22114a67fafb70223cd5ad385d1f162700d9e264/radio/src/telemetry/telemetry_sensors.cpp#L253-L265

Can't say I agree with that logic though... since it only resets given sensors if the sensor unit is UNIT_VOLTS? IMO better to reset the telemetry properly since all telemetry data is now invalid if the battery was changed, which can be done via logical switch and reset SF.

That explains one behaviour... min following the current value if the current value is higher than the sensor session max... does it also explain the rollover at 13.2v?

3djc commented 5 days ago

Disagree on the full telemetry reset, if you do a gliding afternoon, you want to keep your best alt, not the one on last battery. But bat- really makes sense for current batt only

pfeerick commented 5 days ago

So why not have a Lua script keep track of that specialised scenario, rather than it be wrong and different behaviour for just that one suffix type? Or use a LS + GV?

---------- Forwarded message --------- From: 3djc @.> Date: Mon, 24 June 2024, 4:09 pm Subject: Re: [EdgeTX/edgetx] Simulator RxBt+/- strange behaviour/Error (Issue #5214) To: EdgeTX/edgetx @.> Cc: Peter Feerick @.>, Comment < @.>

Disagree on the full telemetry reset, if you do a gliding afternoon, you want to keep your best alt, not the one on last battery. But bat- really makes sense for current batt only

— Reply to this email directly, view it on GitHub https://github.com/EdgeTX/edgetx/issues/5214#issuecomment-2185686700, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABJ66KOR5IVWR4J2C2AQ6ODZI6ZZNAVCNFSM6AAAAABJYXJ3NKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBVGY4DMNZQGA . You are receiving this because you commented.Message ID: @.***>

3djc commented 5 days ago

The thing is who would not want to use this feature ? Yes, it does not make a whole lot of sense on simu, but it bloody does on the radio itself. It has been up and running for years, not sure it should be removed at all.

What modeler is actually going to benefit from removal of this feature ?

derelict commented 5 days ago

i more or less agree with all of you :-) ... just my two cents: i think it's not up to the sensor/telemetry/radio to determine or "guess" when a battery has been changed ... for me ... EVERY Sensor should reflect TRUE Current, + and - Values until it is reset by other means. Just a new max Voltage does not necessarily mean "a Battery has been changed"

3djc commented 5 days ago

How exactly do lipo voltage rise above their initial values without been changed ? Now this change would mean a lot of modeler would have to go to quite a bit of complexity (there is no easy way to do that for end users, the only somewhat easy been using GV, but there already isn't enough of those) on every model to detect battery change and trigger a lipo sensor reset. Thats a lot of downside, for a very theoretical benefit. You could argue that current implementation DOES indeed give you bat- for your current battery, not some previous one

derelict commented 5 days ago
(newVal > valueMax)

what if the new battery has a voltage below max of the previous battery (and therefore not matching newVal > valueMax) ? you will end up with valueMin of the previous battery anyway ... again .... ALL Sensors should report true Current, + and - ... until completely reset on purpose ... that's what "Reset Telemetry" is for ...

pfeerick commented 4 days ago

One thing that I think is being overlooked here is this is not exclusive to battery... but any sensor with UNIT_VOLTS. Since this is a sensor level behaviour, perhaps it would be better for this to be an option at the sensor level? Another consideration being if some (thus potentially all if so desired) sensors should be able to auto-reset on telemetry loss.

derelict commented 4 days ago

Another consideration being if some (thus potentially all if so desired) sensors should be able to auto-reset on telemetry loss.

And/Or a LUA Function to trigger "Reset Telemetry" (if not available already)

3djc commented 4 days ago

One thing that I think is being overlooked here is this is not exclusive to battery... but any sensor with UNIT_VOLTS. Since this is a sensor level behaviour, perhaps it would be better for this to be an option at the sensor level? Another consideration being if some (thus potentially all if so desired) sensors should be able to auto-reset on telemetry loss.

Sounds reasonable as long as it defaults to current behaviour

AJHAJHAJH commented 4 days ago

I agree that "EVERY Sensor should reflect TRUE Current, + and - Values until it is reset by other means"

One addition I would like to see is a Soft Reset Telemetry special function which simply sets the min/max values to the latest current value.

I use reset telemetry at the start of every flight, triggered at the same time as starting the flight timer so that I can review the min/max values at the end of the flight.

Under 2.10.1, this results in a distracting 'Telemetry Connected' call out just as the glider leaves my hand. Often this is accompanied by a distracting 'Receiver Battery Low' call out because the Reset Telemetry special function has reset the Receiver Voltage to zero and no telemetry value has been received before the logical switch for low Voltage is evaluated.

derelict commented 4 days ago

not to hijack my own thread ... but maybe my widget could be of use for you now or in the future:

https://github.com/derelict/TxBatTele

( suggestions welcome )

because for your exact "problem" about the call out's i let telemetry/values settle for a few seconds before starting yelling at you :-)