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

Repair SolarEdge_local inverter fahrenheit temperature #27096

Merged
merged 13 commits into from Oct 5, 2019

Conversation

scheric
Copy link
Contributor

@scheric scheric commented Oct 1, 2019

Description:

The SolarEdge inverters report the inverter temperature in Fahrenheit if country is set to US. The unit_system will then convert the received value. That will result in the temperature reading beeing way off.
Rounding values that could sometimes be extremely unnecessary large #27065 (comment)

This is a follow-up for #26708

Related issue: fixes #27065

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@scheric
Copy link
Contributor Author

scheric commented Oct 1, 2019

@adamj12b
Could you test this version?
You do not need to change the units to Fahrenheit. That should be automatically done byunit_system

@adamj12b
Copy link

adamj12b commented Oct 1, 2019

@scheric I have updated sensor.py. Here is what the system is currently reporting.

Inverter temperature is reported as 93F.

temperature {
  value: 93
  units {
    farenheit: true
  }

image

@scheric
Copy link
Contributor Author

scheric commented Oct 1, 2019

@adamj12b Sorry for my syntax error that did not come up as syntax error. Could you try it again.

@scheric scheric changed the title Repair SolarEdge_local inverter temperature Repair SolarEdge_local inverter fahrenheit temperature Oct 1, 2019
@adamj12b
Copy link

adamj12b commented Oct 1, 2019

@scheric Yes. Formatting appears to be correct now.

IMO, the temperatures in Fahrenheit should display rounded to 1 decimal place. Additionally, Avrage should be corrected to Average on the 4 optimizer sensors.

Everything else looks good.

image

@scheric scheric marked this pull request as ready for review October 1, 2019 19:26
@scheric
Copy link
Contributor Author

scheric commented Oct 1, 2019

@adamj12b
Thanks for your feedback.

I think that this PR can be merged now.

Copy link
Contributor

@awarecan awarecan left a comment

Choose a reason for hiding this comment

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

Why you do not change the unit of inverter_temperature sensor?

homeassistant/components/solaredge_local/sensor.py Outdated Show resolved Hide resolved
@scheric
Copy link
Contributor Author

scheric commented Oct 2, 2019

from @awarecan
Why you do not change the unit of inverter_temperature sensor?

You are totally right that is indeed the best solution.

@adamj12b
Would you like to test this version.

@adamj12b
Copy link

adamj12b commented Oct 2, 2019

Looks good on my end.

image

@scheric scheric requested a review from awarecan October 2, 2019 12:05
@scheric
Copy link
Contributor Author

scheric commented Oct 3, 2019

I got my Home Assistant back.
I can confirm this works in Celsius.
Could @MartinHjelmare review this because you reviewed my last SolarEdge_local PR.

homeassistant/components/solaredge_local/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/solaredge_local/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/solaredge_local/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/solaredge_local/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/solaredge_local/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/solaredge_local/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/solaredge_local/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/solaredge_local/sensor.py Outdated Show resolved Hide resolved
@MartinHjelmare MartinHjelmare added this to Review in progress in Dev Oct 4, 2019
Dev automation moved this from Review in progress to Reviewer approved Oct 4, 2019
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Good!

@MartinHjelmare
Copy link
Member

Can be merged when build passes.

@awarecan awarecan merged commit 9985948 into home-assistant:dev Oct 5, 2019
Dev automation moved this from Reviewer approved to Done Oct 5, 2019
@lock lock bot locked and limited conversation to collaborators Oct 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
Development

Successfully merging this pull request may close these issues.

Solaredge Local incorrect temperature conversion in dev branch
6 participants