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

Improve unit compatibility and enable statistics #4

Merged
merged 7 commits into from
Oct 22, 2024

Conversation

phidauex
Copy link
Contributor

Hi, nice work for a first integration! This PR corrects a few things to allow easier use in different unit systems, by switching many of the variables to the native version, meaning it specifies that the value is in the native unit given in the config, which fixes unit conversion for Fahrenheit, inHg, etc. I also fixed the native wind speed unit (m/s, rather than kph).

I also added StateClass and DeviceClass to the battery and rewards sensors, this allows for proper statistics usage, and particularly for the total rewards sensors, would allow for nicer graphs showing change over time.

I'm playing with the automatic update thing, but I can't figure out why it isn't working either - everything seems to be there from other working examples I've seen... If I figure it out I'll submit another PR.

@elboletaire
Copy link
Owner

Hi, nice work for a first integration!

Thanks! I clearly still have a lot to learn after reviewing your PR —I didn’t know anything about the native_ units!

I'm playing with the automatic update thing, but I can't figure out why it isn't working either - everything seems to be there from other working examples I've seen... If I figure it out I'll submit another PR.

I felt the same, but since I’m a newbie, I can’t add much insight tbh. However, there’s clearly something wrong with the fetched/cached data, as the forecasts aren’t updating properly either.

I’ll try to review and accept your PR during the day. Everything looks good, but I’d like to test it first. I’m also wondering what will happen with the old data, since the units are being changed (I guess it’s just lost?).

Copy link
Owner

@elboletaire elboletaire left a comment

Choose a reason for hiding this comment

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

LGTM

@elboletaire elboletaire merged commit 48944a0 into elboletaire:main Oct 22, 2024
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.

2 participants