edgetx
edgetx copied to clipboard
EdgeTX
feat: Support GVs in calculated telemetry sources
Fixes #2092
Summary of changes:
Allows calculated telemetry sources (specifically add, average, min, max and multiply) to use GVars as sources.
The multiply source can also divide by GVars, not just multiply.
This is useful for scaling telemetry values in ways that ratio does not support due to limited precision.
Additionally, GVars may be dynamically changed, further expanding the scaling capabilities.
Example: Auto-detecting the number of cells and scaling a voltage sensor (VFAS, blheli32 telemetry) to produce average cell voltage.
Current state:
- ~~Missing gui for colorlcd.~~
- Gui works on simulated X9LITE.
- Tested and works on real X9D+.
See also: https://github.com/opentx/opentx/issues/8436 https://github.com/opentx/opentx/issues/6293 https://github.com/opentx/opentx/pull/3593
Colorlcd looks good in simulator, it would be good to have someone test it for real.
Main thing missing now is Companion
@elecpower - please could you step in? Thx :)
I can do the Companion side when you get radio side approval from the main devs.
Thanks for the offer @elecpower, that is very helpful :)
I have since tested this in an actual flight on X9D+. It works great! I finally have human understandable voltage callouts for my 3s lipo!
The radio side code is ready as far as i am concerned. It would be great to hear some feedback on it.
Ubuntu 20.04 Tested TX16S sim and observed the source drop down list has negative and positive entries mixed. Should be negatives followed by '---' followed by mirrored positives. Tested X9D+ and X7 sims and source drop down does not list GVs. Tested with both a new model so no sensors and a converted TX16S with sensors. The lists on the sims are as expected but without the GVs obviously. I have GVARS defined and checked by inserting a TRACE line below line 282 in radio/src/gui/212x64/model_telemetry_sensor.cpp
Source drop down on X9D+? It has a field where you go through sensors with + and - buttons. You have to Long-Press to get to the GVars. This is the same behaviour as input or mixer weights use.
Am I misunderstanding something?
Source drop down on X9D+? It has a field where you go through sensors with + and - buttons. You have to Long-Press to get to the GVars. This is the same behaviour as input or mixer weights use.
Am I misunderstanding something?
My bad forgot about the long press as I do 99.9% of my config in Companion and didn't even notice log press in code. So retested ok for B&W. Just leaves list order issue for color.
Based on the model file from X9D+ sim and this likely explains the TX16S list order, there is an inconsistency with the YAML values for GVs. Sensors range from negative max radio sensor capability eg -60 to positive max sensor capability eg +60 with no sensor = zero. GVs on the other hand have this pattern: +GV1 stored as -128 -GV1 stored as +127 +GV2 stored as -127 -GV2 stored as +126 Sign is inverted and not numerically mirrored as sensors. I assume the existing memory allocation of int8_t is the reason for the reverse increment Though +/- should have the same number especially since we are using YAML now and to match sensors. There is a risk that should the max radio sensors capability be increased and/or max radio gvars increased (there is a push to do this) we could have an overlap. Unlikely due to the current ranges but possible.
Companion coded to use yaml mapping -GV1 -> -127 and GV1 -> 127 -GV2 -> -126 and GV2 -> 126 etc
I have used the existing mapping functions in gvars.h and gvars.cpp.
As such the mapping is the same as is used by weights and offsets.
I don't know why the pattern was chosen, but my guess is that it was convenient to implement. Should i abandon the existing mapping and implement the new one you suggested?
If we go down the weights and offsets approach then for consistency the yaml read and write for both the radio and Companion will need to be changed for the telemetry calc fields as from memory the textual reference eg GV1 is stored in yaml files if not a numeric value for weights and offsets. So either we continue to follow the past methods or start a new one for this case. From the radio coding side I would expect better to stick with the devil we know and change the yaml. One for @raphaelcoeffic to arbitrate on.
Is there any progress, I would like to use this feature. Reqlly useful and could save me a lot of time
2 days ago I needed this feature for one of my projects... this makes a lot of things easier (or maybe I do not know another way in edgetx how to live without this feature).
@philmoz Any chance you can rebase and resolve the conflicts that will arise on the colorlcd side? Also taking this comment about the display of values into account. https://github.com/EdgeTX/edgetx/pull/2096#issuecomment-1203226844
I think https://github.com/EdgeTX/edgetx/pull/2096#issuecomment-1204505814 is possible material for a separate cleanup PR.
Is there anything i can help with?
Can you sync the main branch in your fork to get it up to date.
Rebased to 39b21b677 and fixed the color LCD code (including the sort order). Should rebase cleanly to later PR's.
Have not fixed the B&W issue.
This is not using the new source select menu - that will require more extensive changes (and can probably wait).
Please test that the color LCD functionality is working as expected.
Have not fixed the B&W issue.
There isn't one AFAIK... on B&W, you long press enter in weight/offset fields for GVs, and that is working as expected. The only outstanding issue is related to yaml storage, hence why material for a different PR.
I've only had a quick glance after flashing to TX16S and X9D+ but it looks somewhat like the colorlcd functionality is as expected. I'll comment further in the next day or so after some actual use of it.
@elecpower Sorry to distract from #4406, but regarding this PR, I finally realised what the issue you was referring to was here... i.e. Companion and radio firmware are at odds with how to index GVs (at least for telemetry sensor purposes)? Thus rendering it impossible to use Companion in conjunction with this feature at present. Although it seems if it is configured exclusively on the radio that Companion won't mangle the settings if you leave that field alone? I'm so tempted to merge this on that premise (with a caution in the release notes), but I'll behave! 😆
How hard would it be to make it so Companion matches the firmware? And then move on from there? Perhaps in the future there should be a gv() type available? Or did you mean make a change here, specific to this option (and potentially work outwards from there)?
@raphaelcoeffic Can you have a look at this, particularly Neil's two earlier comments at: https://github.com/EdgeTX/edgetx/pull/2096#issuecomment-1204505814
before (as read from TX16S)
after (set to "correct" value by Companion)
for reference: 2096-calc_gv_error.etx.zip
@pfeerick @raphaelcoeffic IMO this PR should implement one of the methods as shown in the screen shots below so it is crystal clear whether the value is a numeric or a gvar. I am not in favour of yaml containing magic numbers which may cause confusion and can be broken by future features requiring data conversion. There have been requests to increase the number of gvars and this would definitely break the use of +/- 127.
What the firmware and Companion do with it internally is up to each as long as they interpret identically. Companion rawsource and rawswitch are prime examples of different internal handling to firmware.
The code to do the yaml exists so not a big task to implement in this PR.
The yaml parsing is an if then else nightmare now. Why introduce another?
Indeed... that is really what I want to avoid.
How it is done in limitData and srcRaw -> curve (i.e. GV#/-GV#) IMO is the most appropriate then in this case, since either the positive or negative GV could be used. However, I'll butt out now until further comments. ;)
On Thu, Jan 4, 2024 at 8:04 PM Neil Horne @.***> wrote:
The yaml parsing in a if then else nightmare now. Why introduce another?
— Reply to this email directly, view it on GitHub https://github.com/EdgeTX/edgetx/pull/2096#issuecomment-1876824609, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABJ66KJRD47MRJ3GH3JNHY3YMZ5DNAVCNFSM5Z2YAPAKU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOBXGY4DENBWGA4Q . You are receiving this because you were mentioned.Message ID: @.***>