search

EdgeTX / edgetx-sdcard

SD Card contents and images for EdgeTX
125 stars 41 forks source link

BattAnalog: Allow users to define the cell count manually #144

Closed dkorkmazturk closed 7 months ago

dkorkmazturk commented 7 months ago

Even though the current automatic cell count detection logic works fine for the batteries with lower cell counts, it does not work as expected for the batteries with higher cell counts under some circumstances. For example, 45.6V total battery voltage could either mean 4.14V per cell for an 11S battery or 3.8V per cell for a 12S battery. In this case, the logic in use decides that the battery is 11S and gives wrong per cell voltage if the battery in use is 12S.

This change allows users to optionally define the cell count manually in the settings of this widget to prevent the type of problems described above.

offer-shmuely commented 7 months ago

we can not do that, we are limited to 5 option only your change is throwing the "Lithium_HV."

In addition to the future, if you add an option in the middle, you should take care of backward compatibility to existing instances, since the edgeTx is keeping the user selection by order, and not by key-value pairs ;-( e.g. if the user so if selected [4] Lithium_Ion=off [5] Lithium_HV=on

after your changes, since the Lithium_Ion is now in position [5] you will get Lithium_Ion=on

dkorkmazturk commented 7 months ago

Thanks a lot for your reply. Now that you pointed out, I realized that "Lithium_HV" is missing in the options menu after these changes. I guess I focused too much on the functionality I was adding while testing and missed that detail.

If it is not possible to combine the existing battery type options in a single option, would it be feasible to use global variables to set the cell count? Similar to what the "Flights" widget does with flight counts.

offer-shmuely commented 7 months ago

Easy to do, hard to explain to users. And there is pitfalls in this solution, since you can have more than one widget each monitoring different battery.

If we could get a simple storage space in the model (like the jeti api have) , i could do mach more

offer-shmuely commented 7 months ago

Please close this PR so it will not be merged by mistake

pfeerick commented 7 months ago

Little chance of that... I was going to ping you to review it since it is your baby anyway, plus I was thinking we were still limited to five options unless I missed a memo or PR... :scream: Back to the drawing board for this I'm afraid.

If we could get a simple storage space in the model (like the jeti api have) , i could do mach more

Hm... do I hear an enhancement issue that hasn't been raised? :zany_face: Sounds interesting (and useful)...

offer-shmuely commented 7 months ago

It was raised, during the dev session, But i was told I am asking too many things 😎 And jeti (i gave a sample of there api) take money for their features 🤔 So I will seat back and wait… 🤷‍♂️

dkorkmazturk commented 1 week ago

At the EdgeTX fest, I was told that the the number of the options we are allowed to have in a widget is increased (to 10 I believe?). I haven't verified it myself, but if that is the case, do you mind if I revive this MR?

offer-shmuely commented 1 week ago

There is no need to revive this issue It was already implemented 😀

dkorkmazturk commented 1 week ago

Cool. Thank you.

offer-shmuely commented 1 week ago

We now just need to wait to v2.11