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

Deal with ppValue == ppThresh scenario #150

Merged
merged 1 commit into from
Jan 28, 2025

Conversation

chrskly
Copy link
Contributor

@chrskly chrskly commented Jan 26, 2025

There's a possible bug here. The code deals with ppValue < ppThresh and ppValue > ppThresh but not ppValue == ppThresh. It's probably not likely that this would occur often, but it could lead weird behavior when ppValue is near ppThresh. Noise could possibly produce irratic results. E.g., plug in first time, charge fails, plug in second time, charge succeeds.

I've set the == condition on the positive side because I think that is what the user expectation would be.

Another option would be to set the == on the negative side.

Yet another option would be to remove the if (ppValue > ppThresh) condition from the else block.

Copy link
Owner

@damienmaguire damienmaguire left a comment

Choose a reason for hiding this comment

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

Has this been tested? Thanks

@chrskly
Copy link
Contributor Author

chrskly commented Jan 27, 2025

No, I don't have the means to test it right now. I'm just reading through the code to get an understanding of how it works and noticed that there's a gap here.

The change just substitutes a < for a <= to catch every possible value of ppValue. Should be a relatively safe change.

Copy link
Collaborator

@Tom-evnut Tom-evnut left a comment

Choose a reason for hiding this comment

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

Agree, this is the "correct" way of defining.

@Tom-evnut Tom-evnut merged commit 9533afb into damienmaguire:master Jan 28, 2025
1 check passed
@chrskly
Copy link
Contributor Author

chrskly commented Jan 28, 2025

Cheers!

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