Support for FrSky 8s Lipo Sensor FLVS ADV

[mha1]

To support https://www.frsky-rc.com/product/flvs-adv/. FLVS ADV transfers up to 8 cell voltages for Cels sensor

Summary of changes:

• extension to allow a maximum of 8 instead of 6 cells for Cels sensor (e.g. LUA will deliver a table with max 8 entries)

[pfeerick]

I take it you have one of these so it's not just a "wild stab in the dark should work" PR? 😁

[mha1]

I take it you have one of these so it's not just a "wild stab in the dark should work" PR? 😁

I got in contact with a unhappy guy owning one. So I sent him a private build with the changes along with a quick and dirty LUA widget to display the raw data getValue("Cels") returns because the current version of BattCheck caps at 6 cells (Offer is working on this). Here's what my quick and dirty widget looked liked for a 8s Lipo connected to the FLVS ADV:

But Companion seems to need some attention. The telemetry simulator is still showing only 6 cells:

[mha1]

@pfeerick Stabbing the beast in the neck: I extended the telemetry simulator to 8 cells. @offer-shmuely I'd very much appreciate if you could give this a try with BattCheck. This is what my hacked V.05 looks like

[pfeerick]

Any concerns @elecpower?

[elecpower]

Any concerns @elecpower?

Just a few @pfeerick

Corrupts all existing models in bin, yml, otx and etx formats that use Cell Delta or Highest settings due to the index shift ie Delta becomes Cell 7 and Highest becomes Cell 8. Therefore version specific (so no double ups in future releases) data conversion required for Companion and all Radios.

Unsure but it may throw out usage in lua scripts if index based.

Companion generated yml has new indexes but radios cannot handle.

[mha1]

@elecpower @pfeerick Thanks Neil for looking into this. I agree this might cause some extra work to redo custom sensor setups for a limited number of users. But the real problem behind this is the indexed based telemetry which leads to breaking "sensor name" to "sensor index" consistency even for rediscovery of telemetry sensors with added sensor hardware. Peter and I discussed this some time ago. I hope I may quote Peter: "At the end of the day, I think we want to scrap the whole transmitter generated unique sensor id system that is currently in use, as it prevents the user from having model templates that have references to sensors, or means you have to edit all widgets once a re-discovery has taken place". I might add "or support hardware changes/extension" as in this case. My opinion was and is I'm all for switching to referencing sensors by unique names.

And thanks for pointing out a missing item in my PR. Your screen shot shows that the edit sensor dialog only shows cells 1 to 6. It should also show 1 to 8. I will look into this (if I succeed in finding the code segment for this dialog).

[pfeerick]

STR_VCELLINDEX => TR_VCELLINDEX is the translation string, and a search for that should pin down the relevant model_telemetry_sensor.cpp lines that also need tweaking ;)

[mha1]

yep, found it, working on it

[pfeerick]

Oh thank god... more magic numbers removed! 😆

[mha1]

This PR is growing. Any more places to look out for?

[pfeerick]

Probably just:

Corrupts all existing models in bin, yml, otx and etx formats that use Cell Delta or Highest settings due to the index shift ie Delta becomes Cell 7 and Highest becomes Cell 8. Therefore version specific (so no double ups in future releases) data conversion required for Companion and all Radios.

whereby anything pre 2.8 needs to be shifted by two to make it valid again. Hopefully, 2.9 will bring a new telemetry backend that does away with this lunacy... although that doesn't actually help here anyway... this is a sensor parameter, not the sensor index.

[elecpower]

Another reason for yml to use text references rather than internal indexes eg CELL1 CELLD then less chance for the need for any settings file conversions

[offer-shmuely]

Yaml with text references to sensor will be most welcome! Is there a way to use default in lua script as string instead of magic numbers

[mha1]

Yaml with text references to sensor will be most welcome! Is there a way to use default in lua script as string instead of magic numbers

you mean like the index of Cels if it is defined?

[offer-shmuely]

Take a look here, Line 39, 40

https://github.com/EdgeTX/edgetx-sdcard/blob/master/sdcard/c480x272/WIDGETS/Flights/main.lua#L39

I like to out there a string

{ "switch" , SOURCE, ”SF” },

[mha1]

Take a look here, Line 39, 40

https://github.com/EdgeTX/edgetx-sdcard/blob/master/sdcard/c480x272/WIDGETS/Flights/main.lua#L39

I like to out there a string

{ "switch" , SOURCE, ”SF” },

Try experimenting with something like this:

local switchIndex = getSwitchIndex('SF')
local ch3Index = getSourceIndex('CH3')

local options = {
  { "switch"             , SOURCE, switchIndex    },  
  { "motor_channel"      , SOURCE, ch3Index    },  
  { "min_flight_duration", VALUE , default_flight_starting_duration, 2, 120},
  --{ "enable_sounds"    , BOOL  , 1      },  -- enable sound on adding succ flight, and on end of flight
  { "text_color"         , COLOR , YELLOW },
  { "debug"              , BOOL  , 0      }   -- show status on screen
}

[mha1]

@pfeerick are we doing this?

[pfeerick]

Depends on what Neils view is on with regard to highest and delta indexes... It's all good to talk about what would be nice, but the fact remains those last two will be broken on an import... So if ver not 2.8, bump by two seems in order.

[mha1]

It's a static and constant offset by 2. Should be a low risk conversion. There will be more 8s sensors out there. The benefit outweighs the pain I guess.

[mha1]

@elecpower Hi Neil, what's the word on this?

[elecpower]

IMO needs conversions otherwise: a) spawns unnecessary Issues that have to be responded to b) puts users offside with the product when a relatively simple conversion

[mha1]

@elecpower Thanks. I see three options:

1. don't implement upgrade -> users irritated with not being able to use new hardware -> bad
2. implement 8s upgrade without conversion -> irritates some users -> bad
3. implement 8s upgrade with conversion -> not good, but not as bad as 1 and 2.

My vote: 3 - any volunteers for the conversion part?

[offer-shmuely]

Just an additional perspective user can still use the new hardware, but only up to 6s 98% of the user will not use it so 1 & 2, so it is not bad for them. On option 3, 100% of the user will need to upgrade, so it bad for 100% while helping for 2%

Maybe this merge can wait util there will be another conversion?

[elecpower]

Maybe this merge can wait util there will be another conversion?

The conversion is version specific so needs to be done at the same time otherwise once the yml files are stamped 2.8 you cannot reliably determine whether the individual model settings are pre or post the change.

[pfeerick]

My preference has been for opt 3 the whole time, I was just hoping you'd figure that out without me jumping up and down like a crazy person saying "upgrades making unnecessary work for/confused users == bad" :-P I'm not sure why num 3 would be "not good" other than "more work" ... it's the best opt as it allows for 6S users to upgrade without needing to change/verify settings. num 2 would mean anyone with a 6S setup using the last two options (highest and delta) would need to reconfigure, since the settings since that would now be cells 7 and 8 ;)

It needs conversion in two places = on the radio firmware, and in Companion. Making it version dependent, we can avoid the whole "are they using index 7 & 8 or not" mess in determining whether to convert or not. Hopefully, we can do this for 2.8, rather than it having to wait for 2.9. I'll see what I can look at, but it may have to be a Neil think as far as pointers on where to look - the only conversion stuff I've done so far has been conversion for parameter name changes, rather than having to tie to fw versions.

[elecpower]

If your desire is for 2.8 then quicker for me to do Companion side for both bin and yml than try and tutor someone especially modding opentxinterface. Though where and how will be evident in changes and happy to answer questions afterwards.

[pfeerick]

That would be greatly appreciated. And as you say, the changes themselves should help explain it. This is something I've been trying to do with some PRs... make them not just fixes or feature implementations but also references for future implementation ;)

[elecpower]

Sorry for taking so long but I had to create a basic semantic version helper. I found a fully featured Semantic Version 2.0 compliant API https://github.com/Neargye/semver but it was written for C++17 and above. Retrograding to C++11 almost required a rewrite so gave up after a list of compiler errors a mile long. Tested with otx and yml files. My work here is done so now up to someone to do the radio side.

[raphaelcoeffic]

Gentlemen, I must apologize for being late to the party. What is actually the issue? How is the old sensor vs. new one serialised in YAML currently? Or what is the issue exactly?

[mha1]

@raphaelcoeffic Thanks for asking. I believe there is no open issue. Just a work required on the radio side for model data conversions.

To summarize the history of this: the PR extends the FrSky Lipo sensor handling to deal with 8 cells to accommodate for the new FrSky FLVS ADV 8 cell Lipo sensor by basically adding cells 7 and 8 to the existing code:

enum {
  TELEM_CELL_INDEX_LOWEST,
  TELEM_CELL_INDEX_1,
  TELEM_CELL_INDEX_2,
  TELEM_CELL_INDEX_3,
  TELEM_CELL_INDEX_4,
  TELEM_CELL_INDEX_5,
  TELEM_CELL_INDEX_6,
  TELEM_CELL_INDEX_7,
  TELEM_CELL_INDEX_8,
  TELEM_CELL_INDEX_HIGHEST,
  TELEM_CELL_INDEX_DELTA,
  TELEM_CELL_INDEX_LAST = TELEM_CELL_INDEX_DELTA
};

All tested ok so far, but this addition also shifts the index for retrieving data for the cell with the highest voltage and highest delta. This will break model setups using TELEM_CELL_INDEX_HIGHEST and TELEM_CELL_INDEX_DELTA in custom sensors. A conversion is required which Neal kindly implemented for companion. Same procedure is now required for the radio.