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

LCD Display: Display Energy formatted to significant figures instead of decimal digits. #341

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rbsexton
Copy link

@rbsexton rbsexton commented Apr 4, 2022

The existing FW scales energy to SI notation and then places one digit after the decimal point.

This doesn't work so well for low values in kWh, ie 1.3kWh.

The ADCs in the system deliver about 3 significant figures, so display this number to a total of four - ie:

1.305kWh
13.05kWh
130.5kWh

@rbsexton rbsexton changed the title Rbsexton/issue340 LCD Display: Display Energy formatted to significant figures instead of decimal digits. Apr 4, 2022
@jeremypoulter
Copy link
Collaborator

@rbsexton thanks for the contribution, been something I have been meaning to do this for a while. Will check it over.

Comment on lines +629 to +632
"m",
"g",
"t",
"p"
Copy link
Contributor

Choose a reason for hiding this comment

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

I opened another PR before seeing this one to correct the capitalization of these units - might be worth updating it here since you're moving these around? #344

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a fan of multiple changes per PR but #344 will conflict with this PR and visa-versa so ok if @rbsexton wants to add the unit update

Copy link
Author

Choose a reason for hiding this comment

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

This one has been laying around! I'd forgotten all about it. I'll update this so that it can get pulled.

Copy link
Author

Choose a reason for hiding this comment

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

Ok. I'm pretty sure that the merge of PR#344 made mine unmergable. I'm going to start over and I'll fix the rounding error while I'm at it.

Copy link
Collaborator

@jeremypoulter jeremypoulter left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, this is something I have been meaning to do for ages. I have added a few comments

Comment on lines +629 to +632
"m",
"g",
"t",
"p"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a fan of multiple changes per PR but #344 will conflict with this PR and visa-versa so ok if @rbsexton wants to add the unit update

Comment on lines +680 to +681
// of significant figures eg, for 4: 1.00598kWh appears as 1.005 rather than 1.0
// Overloaded on displayScaledNumberValue
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this example highlights a flaw with the implementation. 1.00598kWh should be rounded up to 1.006kWh

Copy link
Author

Choose a reason for hiding this comment

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

Good catch. printf() doesn't round. I'll add that.

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