Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix LTO discharge curve #3385

Merged
merged 2 commits into from
Mar 12, 2024
Merged

Fix LTO discharge curve #3385

merged 2 commits into from
Mar 12, 2024

Conversation

wnagele
Copy link
Contributor

@wnagele wnagele commented Mar 12, 2024

Been testing LTO cells recently and noticed that the SoC values didn't make much sense.

Upon a closer look the values for the discharge curve are far off from what a typical LTO discharge curve looks like. Here I put in the current curve (red dots) to visualise this for myself:
lto_dis_curve
This PR changes the curve to the values from this graph (1C) which in turn result in reasonable SoC values.

I did a cursory check on the other chemistries and they don't seem to suffer the same issue. So I guess some wrong discharge curve was used to deduct those values from maybe?

@wnagele
Copy link
Contributor Author

wnagele commented Mar 12, 2024

Maybe @Gabrielerusso can also give some feedback. As the original implementation is from him in #3216. I think the LTO curve was from @code8buster.

@wnagele
Copy link
Contributor Author

wnagele commented Mar 12, 2024

I've also removed that comment block as all that information is in the define section below and no value is added by those comments from my pov.

Copy link
Contributor

@Gabrielerusso Gabrielerusso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are not code of line and aren't put there randomly, for example there are three different curve for LiIon to chose from, and are there for reference for who wants to edit them to their custom usage.

@Gabrielerusso
Copy link
Contributor

Maybe @Gabrielerusso can also give some feedback. As the original implementation is from him in #3216. I think the LTO curve was from @code8buster.

Never took a look on what the LTO curve was, the other one I took them from BMS projects I was working so should be correct.

@wnagele
Copy link
Contributor Author

wnagele commented Mar 12, 2024

Those are not code of line and aren't put there randomly, for example there are three different curve for LiIon to chose from, and are there for reference for who wants to edit them to their custom usage.

How about I add those as individual configurations instead so that someone who wants to use them can directly configure for them?

@wnagele
Copy link
Contributor Author

wnagele commented Mar 12, 2024

On second thought - this is probably too many options.
If people already have to go re-configure, they can of course also adjust those curves whichever way they see fit.
So I suggest we keep this to the already long list of chemistries that are supported now.

@thebentern this should be fine to merge.

@thebentern thebentern merged commit 38ea681 into meshtastic:master Mar 12, 2024
69 checks passed
@wnagele wnagele deleted the fix_lto_curve branch March 13, 2024 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants