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.61k stars 340 forks source link

COLOUR SETTINGS IN WIDGET #2197

Closed DG9BFC closed 2 years ago

DG9BFC commented 2 years ago

Is there an existing issue for this problem?

What part of EdgeTX is the focus of this bug?

Transmitter firmware

Current Behavior

colour settings in the widgets are somehow mixed up (or say some not working) i can not set the widget colour in the top row ... instead two of the widget on main screen set also the colour on the top row colour of link quality (RQLY) in main screen also sets colour of rx batt and tx batt in top row colour of RSNR in main screen sets colour of RSNR and RQLY in top row if i try to change colour of the widget in the top row nothing changes

Expected Behavior

the colour in top row should be set in its widget alone and not grabbed from main colour setting

Steps To Reproduce

try to set the colours in the top widgets ... does not work change the colour in main also changes colour in top row

Version

Other (Please specify below)

Transmitter

Radiomaster TX16S

Anything else?

photo_2022-08-06_11-52-19 photo_2022-08-06_12-15-03

using 2.8.0

DG9BFC commented 2 years ago

another bug found if i select one of the main widgets to show fullscreen its colour is then also set to tx and rx battery (widget on top left as gauge) so if i have select one widget in main to be "red" and select it to show fullscreen also the battery symbols (top left widgets) show that colour .. select another widget (and show fullscreen) sets then the two widgets to the colour of that new selected widget

pfeerick commented 2 years ago

Is this the BattCheck (lua) widget that you're using? in the top left - as I would suspect it has a bug with the colour it's using.

Also, what nightly are you using, as that is not a recent version (i.e. what are the build date and commit listed on the radio settings -> version page?). The builds for the last few weeks are using LVGL, and the UI looks a little different...

image image

pfeerick commented 2 years ago

And yes, it looks like the bug is over in the BattCheck lua widget - it's using the shared CUSTOM_COLOR colour index rather than keeping track of it's colour internally, which is why this problem is happening. I'll see if I can massage it to track colours properly itself, and then you'll be able to update the widget on your SD card to fix the isssue.

pfeerick commented 2 years ago

Once I've finished testing / double checking the changes in https://github.com/EdgeTX/edgetx-sdcard/pull/70 you should be able to update your both the BattCheck and BattAnalog widgets as they were both failing to set the CUSTOM_COLOR index to the color they needed before using them when the widget was in a small zone, hence why other widgets using the same shared color variable would alter the color.

DG9BFC commented 2 years ago

i do find three batt widgets and all seems to have some faults BattAnalog (changes colour in top row like described) BattCheck (does not work at all .. not tx voltage and no rx voltage) BattCheckAn (can not be saved ... after power cycle its gone) so ... all three seem a bit buggy ..... sidenote ... the values how much voltage is what percentage needed some tuning so i made some changes in the voltage table ... yes i am sure that i made no faults there ;-) ... only replaced some numbers .... the version used here is 2.8.0 (c495606f) ... compiled by "sandor dm4ds" ... date 04-04-2022

that may explain the different layout you see

options: bluetooth, crossfire, ghost, afhds3, internalgps, internalmulti, internlaccess, multimodule, luac, ppmus, externalaccessmod

....................

so battery scripts seem to be a bit buggy :-)

if i just have to replace these three widgets (drop on sd card) .. yes thats easy .. so if you have something to try ... drop me a line greetz sigi dg9bfc

pfeerick commented 2 years ago

BattCheckAn was a version of BattAnalog that I'm not sure was included in a release SD card pack, so must have picked that up along the way. I think BattCheck is meant to be used with a multi-cell source like the Frsky FLVSS sensor... so probably won't be any use to you unless you have one of those.

i.e. this is BattCheck

image

vs BattAnalog

image

As far as the voltage table... you'd have to take that up with with the author, since as he indicated it is based on commercial lipo sensor readings... 😆 But this is the beauty of it being a lua script... you can just got right in and change it to suit your needs ;)

Ah, I guess you need the externalaccessmod or bluetooth so can't use a nightly firmware?

Anyway... the change is very simple... so if you've already been in to edit the file, it's probably easiest to copy the change in... namely just past this bit into the refreshZoneTiny function as shown ...

  if wgt.isDataAvailable then
    lcd.setColor(CUSTOM_COLOR, wgt.options.Color)
  else
    lcd.setColor(CUSTOM_COLOR, GREY)
  end

image

Otherwise, this is the current sd card pack including those changes - you can replace both the BattAnalog and BattCheck widgets (and delete BattCheckAn if that's present as it shouldn't be there). c480x272.zip

DG9BFC commented 2 years ago

that worked superb :-) i copied those lines in the files and it worked exactly like it should (set user selected color if data available otherwise set grey) i edited also the values in these two script ... i did not eant to see a "red" battery with a voltage of 7.4V ... i use round cells so its save to use them down to 3v ... and they only drop that low at the very end (and then there is still a rest left) thanks for your help (that was fast service hi hi) do you need my modded files with the new values?!? ... now the widget shows the same as the builtin battery indicator i maybe will modify them even more when i have time to do a "controlled discharge" with my digital charger and take some measurements (voltage drop over time with constant 400mA current) anyway ... thanks for your help

pfeerick commented 2 years ago

Perfect... independent confirmation that I haven't broken stuff is always appreciated! 😁

lol... yeah, when you're using lithium ion cells the percentages for lipo aren't quite right. I be curious to know what the values are, but I don't know if we should change them as some calibration of sorts was obviously done by the widget author - Offer Shmuely ;)

DG9BFC commented 2 years ago

i could send you my modded files (changed voltage table) ... its maybe not perfect yet cause i first would have to make a discharge over time and measure voltage drop to have exacter values ... but its much better as we had it just give me an email addy and i drop them in a mail

DG9BFC commented 2 years ago

in my view that issue could be closed cause it is solved (or seems so) just add the changes permanent in the widgets IT WORKS FINE HERE NOW

DG9BFC commented 2 years ago

uupps ... correction .. there still is a fault i can not switch the widget to fullscreen anymore shows an error (see first picture below) minimize (switch back and the error is also shown in main (see second picture) only reboot solves it

IMG_20220814_192618 IMG_20220814_192429 e

DG9BFC commented 2 years ago

i wanted to switch to fullscreen to easier read the value (doing a discharge test over time to get better values for the scale) grrrrr ... i thought we (you) have it all but it seems we (you) have something overseen hmpf ... i am sure you find it but that means another few lines to edit

pfeerick commented 2 years ago

I need a proper reproducible test case first - it might do that for you, but I'm not seeing it ;)

Which widget specificically are we talking about here - BattCheck or BattAnalog? What sensor is configured to read? Can you get it to fail with this error if you set it to Batt (i.e. the internal Tx Batt), or does it need to be set to a sensor?

Did you change any other lines other than the ones we mentioned above, or the myArrayPercentList list? If so, it might be best if you paste the version you have here so I can see the changes.

DG9BFC commented 2 years ago

i did only add above lines like you have descibed for me (and changed the voltage table a bit but that should be no problem cause i only change some numbers inside the table) you changed only color for when sitting in top row (refresh zone tiny) .. but i guess we also have to look for when switched to fullscreen ... BATT_Widgets.zip

DG9BFC commented 2 years ago

sidenote ... when i use the round lipo cells down to 3v there is still around 25 or 30 % charge left ... so i guess i could go down to 2.8v i use now 2x 21700 in parallel (10Ah) and if discharged to 3v i can only squeeze 6500mAh in the pack ... means if you use it down to 3v you still have a few hours of usage left ... i call that more then safe

DG9BFC commented 2 years ago

and ... yes it is the internal sensor (tx battery)

gagarinlg commented 2 years ago

That depends on the specific cell design and may vary between cells from different manufacturers. And with cheap cells you do not know if all cells are from the same manufacturer. You need the data sheets for the specific cells to decide which voltage is the correct shutoff or warning voltage. Also it depends on the electronics, because there is a minimum operation voltage. And third, the voltage depends also on the current you draw and the age of the cell. You may be able to discharge the cells further with a charger or tester and still not be able to operate the transmitter safely. So better be on the safe side.

DG9BFC commented 2 years ago

yes sure it depends on the cells . Thats why i wrote that i use round 21700 cells (Samsung SDI 50E) ... but even if i do not use them down to 2.8 and stay on the safe side with 3.0v i have many many hours of use i that big packs .. and if it tells me the pack is empty i have say 20 to 25% left ... should be enough to "drive home safely" (or in case of a drone or wing to look for a landing spot) i hope that i did not get "rebranded used cells" but a few charge discharge cycles showed that they are ok (AND THEY LOOKED BRAND NEW!) the fault in the widget is a different thingy (can not switch to fullscreen, see above) and i hope that "pfeerick" can find it its the same for the internal (tx batt) sensor and the receiver (rx batt) sensor both show that "error in refresh bad argument..." fault

DG9BFC commented 2 years ago

AHH i found something ... i have only two models in the trx .. my real quad and a simulator (switched off rf module) all settings (besides rf module off) are identical

if i boot in simulator and switch to real i can use fullscreen if i boot in real quad and switch to simulator i can use fullscreen if i boot in sim and direct select fullscreen shows above fault if i boot in real quad and direct select fullscreen it shows above fault

so ... it seems as i need to do a model change first and then no fault is seen

those tests were made without receiver powered on (same faults does happen if receiver is on but then i have also the rx battery voltage on screen) both widgets (tx and rx battery) show that fault when selecting fullscreen

another small cosmetic fault: if the telemetry is lost when rx batt is set to fullscreen then only the text is in grey blinking but the battery is still shown in green (full) but ok that is just cosmetics :-)

main thing is that i can only select fullscreen if i before made a model change (from sim to real or from real to sim) AND tried the fulscreen before change of model or to be real precise ...

1. boot (does not matter if model 1 or 2 selected) .. say you boot in model 1 model 1 ... try to use fullscreen > error select model 2 ... fullscreen works

2. boot again now with model 2 selecting fullscreen >error select model 1 ... fullscreen works

3. boot again with model 1 select model 2 and back to model 1 > error in fullscreen

so its not just a select new model thingy .. you have to see that fault and change model to refresh the screen (boot new) and THEN the fault does not happen anymore

yes a bit puzzling ... grin

pfeerick commented 2 years ago

Regarding the cell voltages... it's more that we're talking different cell chemistries there... the table is scaled for lithium polymer batteries, not lithium ion, so doesn't take into account their lower discharge voltage. This is where an option might be good to have for that widget, to use two different tables, since some TX battery packs will be li-po, and others will be li-ion. But the minimum voltage of the handset (and RF module, if using an external module) is the other important factor.

I did actually make a few more changes in the PR linked to this issue - https://github.com/EdgeTX/edgetx-sdcard/pull/70 - but the lines mentioned were relevant to this issue. I don't think the other changes would affect this issue. Thanks for uploading the zip, I just want to double check there isn't something else weird going on... i.e it could even be an older version of the widget and this was a bug that has been since fixed, etc. So I'll have a look at that, and then play with this a little more.

Thanks for pinning it down a reproducible pattern - i.e. if you haven't changed the active model since booting, it errors. But if you switch models, it's fine. That it also does it with the tx batt sensor is perfect also, easy to test. I'll let you know how I go. :)

DG9BFC commented 2 years ago

i fully agree with you that we need a better cell voltage table or say two different widgets to select mine is also not correct but at least works better for me as the original one ... still having a goooood safety margin left (i could only squeeze 6500 into the 10ah cells so say 35% rest margin to complete empty ... or 15 % if we say 20% rest charge) and its (cause easier for me) a linear table and also does not match the discharge curve very well it works .. so what :-)

""""Thanks for pinning it down a reproducible pattern - i.e. if you haven't changed the active model since booting, it errors. But if you switch models, it's fine""" if you switch models before see that error it does not help (see number3) so ... you have to see that fault then switch models and then its fine another thing .. if you see the fault at one sensor (say rx batt) and select other (tx batt) it works also (no model change needed)

hmm yes puzzling but easy to reproduce so now its your turn to find it hi hi

DG9BFC commented 2 years ago

its same behaviour on rx and tx battery on FIRST use of the fullscreen mode it errors ... before or after a model change (does not matter) if you have seen that error once ... change model ... error is gone if you see error and select other sensor .. THAT sensor works in fullscreen so i guess i nailed it down enough

DG9BFC commented 2 years ago

https://www.batteryspace.com/prod-specs/NCR18650B.pdf i guess discharge curve of most round cells are quite similar here data of the 21700 i use https://lygte-info.dk/review/batteries2012/Samsung%20INR21700-50E%205000mAh%20%28Cyan%29%20UK.html

gagarinlg commented 2 years ago

https://www.batteryspace.com/prod-specs/NCR18650B.pdf i guess discharge curve of most round cells are quite similar here data of the 21700 i use https://lygte-info.dk/review/batteries2012/Samsung%20INR21700-50E%205000mAh%20%28Cyan%29%20UK.html

You can get different cell chemistry types in the same form factor and not all radios are limited to "round cells". So a cell type option for the widget would be good.

DG9BFC commented 2 years ago

i guess two curves ... one for pouch packs (flight packs) and one for round cells (normal 3.7v 18650 cells) should be ok discharge limit of round cells can differ from 2.5 to 2.65 depending on manufacturer ... but if we stay above 2.8 or at best above 3v we should be quite safe ... i now have 0% at 3v and have some good rest charge left two curves should be ok (or two times the two widgets ... just drop the right ones on your sd card) i know that my "linear" curve is not right but better as the old one ... and 3v is a good safety margin ... not??

DG9BFC commented 2 years ago

GOOOOOD NEWS!!! flashed the 2.8.0 (nightly) and there the fullscreen does not show the error anymore problems solved :-) so ... seems as that is an issue that is solved and can be closed

offer-shmuely commented 2 years ago

Hello sorry, but I missed this thread. regarding the color, you are correct, there is a bug that miss that setting of the color when it top bar, the suggested fix does work, but I like to see if I can remove the use of CUSTOM_COLOR at all (this code came from openTx, that have less capable api)

regarding the voltage alignment, please do not call the battery rounded and pack, since different chemistry come in different forms. lets call them LiPo & Li-ion (lithium polymer, & lithium ion) indeed the current widget is showing LiPo levels, but I can add also Li-ion levels can you send me the level you suggest, and will also do a research to try and verify it)

DG9BFC commented 2 years ago

4.2 down to 3v is what i use here and that should be safe for all 18650 or 21700 cells (LI-ION) manufacturer squeeze some cells down to 2.5 ... some to 2.65 ... all are ABOVE 2.8v i took 3v to have some safety margin widgets like i use them now are above in the zip file my curve is linear between 4.2 and 3v ... sure needs some finetune between 4v and around 3.3 it is almost a linear drop in voltage and then drops faster down to 3v ... and then even faster down to 2.5 ... (see picture) ... i draw in "orange" what i guess does fit more or less exact discharge_curve_edit my "linear" curve does not fit exact ... but is a start point :-) 3V as 0% lets you enough safety margin

  1. for long cell live
  2. and for fly home safely and below 3v it drops a bit faster ... so i guess its ok like i have it now
offer-shmuely commented 2 years ago

I found this one https://electric-scooter.guide/guides/electric-scooter-battery-voltage-chart/

image

DG9BFC commented 2 years ago

that also does not match ... they just divided the 12v difference from 42v to 30v into equally 20 steps (0.6v per 5%) so that is as good (or bad) as what i already have ;-)

give me a bit of time and i draw a better matching curve

offer-shmuely commented 2 years ago

here is a fix https://github.com/EdgeTX/edgetx-sdcard/pull/72

it is a bad practice to use the same bug issue for more than one issue. this issue is about the colors

@DG9BFC I suggest to open a separate issue, as enhancement, with a title like "support Li-Ion values on BattAnalog & BattCheck widgets"

DG9BFC commented 2 years ago

... if that fix is merged /will be merged in the main package ... ok .. good sure you are right we (I) should make a new issue about the voltage curve ;-)

i am right now digging trough different cell data sheets to find a curve that matches (or almost) can you send me the two widgets by mail and i try them here?? callsign (at) freenet (dot) de thank you

ps right now i am working on a discharge curve / table ...

offer-shmuely commented 2 years ago

Hi you can take them from here: https://github.com/offer-shmuely/edgetx-x10-widgets/tree/master/BattAnalog https://github.com/offer-shmuely/edgetx-x10-widgets/tree/master/BattCheck

the BattCheck already include the suggested Li-Ion values

DG9BFC commented 2 years ago

i will maybe finetune your curve a bit (still working on the values) if you have an email address for me i can send you my modded file (just in case you want it) thank you dg9bfc sigi ps shall i open an issue about those values??

DG9BFC commented 2 years ago

discharge_curve_new

blue is your curve (more or less linear drop of voltage) orange is what i would use (your is 0% at 3v and mine still shows 9% at 3V) i guess my curve fits a bit better ... what do you think?

offer-shmuely commented 2 years ago

Please discuss the lithium-ion on different thread

DG9BFC commented 2 years ago

will do :-)

DG9BFC commented 2 years ago

had to reopen the issue cause batt check does NOT work :-( color can not be set (all white, no color circle to select) and does not show any voltage on the tx battery

DG9BFC commented 2 years ago

batt analog works (but has no option to switch to li-ion so shows wrong percentage) ... batt check does not work (no voltage and no color can be selected) so there must be still a bug hidden somewhere i guess ... hmm?? can we have the option to switch to LI-ION also on batt analog (tx battery sensor) and not only in batt check??

offer-shmuely commented 2 years ago

Please lets discuss on this thread only things that related to the title. i.e. color

I saw that on 2.8 RC2 the color picker is not working correctly Does the color picker work for you on other lua widgets?

DG9BFC commented 2 years ago

color picker works in batt analog ... and in other widgets ... only batt check seems to have a fault shall i open a new issue about the li ion add on in batt analog ??

offer-shmuely commented 2 years ago

Please open an enhancement issue,

"support Li-Ion values on BattAnalog & BattCheck widgets"

DG9BFC commented 2 years ago

i did open a new one (enhancement issue)

offer-shmuely commented 2 years ago

it looks like the new color picker does not handle itself correctly when starting from WHITE color, I changed it back to Yellow (most visible in direct sunlight)

from my testing on the simulator, you will need to:

you can find it both in the PR, or https://github.com/offer-shmuely/edgetx-x10-widgets/blob/master/BattCheck/main.lua

DG9BFC commented 2 years ago

just tested .. yes color picker now works fine in BATTCHECK batt check does not work with trx battery (even if i can select that sensor as input in the widget) so i have to remember to use batt analog instead so ... color issue can be closed .. can you copy the "switch to li-ion" inside the battanalog widget please (yes i have opened an issue about that)

pfeerick commented 2 years ago

I saw that on 2.8 RC2 the color picker is not working correctly Does the color picker work for you on other lua widgets?

it looks like the new color picker does not handle itself correctly when starting from WHITE color,

@offer-shmuely Do you mean in the current nighties? Or is this with the new (newer?) colour picker being worked on here? https://github.com/EdgeTX/edgetx/pull/2217