add board battery percentage#413
Conversation
446564
left a comment
There was a problem hiding this comment.
can't approve my own review after changes...
| uint16_t batt_percent(uint8_t batt, uint16_t mv) { | ||
| uint16_t last = mv; // set last to mv so it can never start greater than itself | ||
| uint16_t p = 0; | ||
| for (int i = 0; i < 102; i++) { // always 101 elements, 100 percent and 0 |
There was a problem hiding this comment.
pretty sure I have a small bug here, 0-99 == 100 so should be i < 101. will fix shortly
|
Any reason we aren't moving forward with some version of this? Example: my companion is currently reporting 53% in app @3.65V but that's about 10% maybe. We can of course offload this to the mobile app but it's not future proof and doesn't allow custom power setups. As well the UI will show incorrect battery status. cc @ripplebiz |
|
I would like to see this move forward as well, reporting (accurate and realistic) battery percentage is a very valuable feature to have. |
And should be done at the lowest level possible so different clients not only do not have to implement their own but the implementations do not differ. Only the hardware knows what SoC a given voltage is equal to. |
|
This feature would be very helpful! |
defaults to lipo with no changes to existing variants - adds two chemistry types to start lipo and lifepo4 - adds getBattPercent to MainBoard - adds getBattType to MainBboard - companion UI now uses getBattPercent in renderBatteryIndicator
|
i was just about to submit a bug for this so i'm glad there's a PR to fix it already |
|
needs rebasing and moving to new UI |
|
Needs rebasing.. and much more:
#403 (comment) :-) |
How do you propose a lookup table for a variable input value? Also this is a lookup table, it's a concept not a specific structure. I thought map but we dont know the value being looked up, which is further complicated by number 3 below. As well, I designed the data structure to be easy to maintain and expand. It's easier to read a table like this. And I doubt the small storage savings is worth the extra complexity.
Absolutely but not in this PR. Smaller changes are easier to manage and right now everything is assumed to be lipo anyway so we can just leave that as default. Anyone getting battery voltage from an alternative chemistry is already building custom firmware.
I disagree. |
#403 (comment)
No.. you added bloat and complexity.
Then start there.
Wrong again.. the current model assumes a theoretical battery with a linear relationship between voltage and capacity.
If you need more than 5% accuracy, look at the volts. |
|
Do it yourself then |
|
@jbrazio no offense, but unless I misread it, the tone in your reply is not very collaborative. Please tell me I read it wrong. |
|
No offence taken. I would not call it "not very collaborative" but it's indeed blunt. This thread has a long history, actually it's the second PR about the same topic with exactly the same fundamental issues. |
|
blunt with constructive feedback is good. blunt and blunt alone is bad. It is easy for anyone to criticize. Constructive and collaborative means pointing out where and what can be improved and also provide support and recommendation and even code. not simply replying with "bloat and complexity" and "wrong again". Scott does this very well, if you read his feedback on PRs. If he has something to say, he tells you why he likes it or not, and he tells what the submitter can do to improve and sometimes provide a line or two of code to demonstrate. This is a feature that we sorely need. If it is not good enough, you can be the change that you wish to see, like @446564. Please don't reply bluntly for the sake of being blunt. We don't want to be like the other mesh project. |
|
I always suggested what would need to be done in order to have a better approach to the problem. |
defaults to lipo with no changes to existing variants