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

Support for Fahrenheit temperatures #1326

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

Conversation

ardichoke
Copy link

This was taken from #1103, it seems the author never got around to fixing the temperature const.

Adds support for Fahrenheit units, using the proper UnitOfTemperature const this time.

Closes #1042

@jhstatewide
Copy link

This is the 3rd PR I have seen for Fahrenheit support not get merged. I am patching SmartIR manually each time now. Is there some reason why this shouldn't be merged? I haven't seen any maintainers weigh in and this is now the 3rd PR that is unaddressed. As someone cursed with Fahrenheit units.... please merge! (or tell us what is wrong so we can supply a new PR)

@vassilis-panos
Copy link
Member

Hi and thank you for your contribution. What happens if a device file is already in Fahrenheit?

@ardichoke
Copy link
Author

Hi and thank you for your contribution. What happens if a device file is already in Fahrenheit?

Is that even possible? Everything in the smartir codebase that I have seen assumes Celsius. This just silently converts between the units if HA is configured to use Fahrenheit.

@vassilis-panos
Copy link
Member

Yes it is. There are already some code files in Fahrenheit

@ardichoke
Copy link
Author

Yes it is. There are already some code files in Fahrenheit

Can you point me at an example so that I can refactor this to account for it?

@vassilis-panos
Copy link
Member

3200.json

@jhstatewide
Copy link

I hope this isn't too controversial to mention here, but after the trouble I had using this integration with Fahrenheit, I switched to this fork: https://github.com/litinoveweedle/SmartIR/ It works perfectly with my units and I had no trouble with unit mismatch.

On the latest release of this integration, my setpoints were reported as "22F". I am unlucky enough to live in a country with Imperial units while using heat pumps that internally operate in Celsius while the remotes display in Fahrenheit!

Regarding the other fork, it allows one to configure Fahrenheit or Celsius and works perfectly for me. I hope some of the ideas from the fork can be brought to this project in hopes to unify them both and avoid wasted effort maintaining two repos that do the same thing.

@vassilis-panos
Copy link
Member

At a glance, the fork you are referring merged your pr.

@ardichoke
Copy link
Author

That wasn't me that posted the fork.... however, having looked at it, I think the above commenter is right. That fork has much better handling of Imperial units. Wish I had found that fork sooner. I'm going to switch to using it right now honestly and hope that you would consider merging their changes into mainline.

@jhstatewide
Copy link

At a glance, the fork you are referring merged your pr.

Here's what happened: I had been using THIS repo for maybe about a year. I had manually patched in the Fahrenheit fix found in one of the issues here and I was using two code files I authored myself to get support for the two heat pumps I have. The other day I saw an update for this repo and forgot all about my patch and pressed 'Update', which promptly busted my integrations (the reported temperatures were clearly in Celsius but reported in Fahrenheit).

I had recalled about a year ago having the same problem and observing that the Fahrenheit fixes/patches were just sitting around. I was going to fork the repo myself and implement the fixes first hand, but then I ran into https://github.com/litinoveweedle/SmartIR/ which worked perfectly after installation. He was also kind enough to merge the PR which had my two units, so I don't need to maintain them out of tree anymore.

I'd prefer to use this official repo instead of a fork, but it doesn't seem to work properly if you are unfortunate enough to need Fahrenheit support. So, I'll be using the fork until there is some movement here re: Fahrenheit, or if the repo's owners comment that Fahrenheit is out of scope.

I understand that open source maintenance is very demanding and people ask a lot for nothing, but it really frustrates me to see open issues with PRs that fix them sitting and turning stale! Apparently at least one other person was frustrated as well, thus the fork.

Anyway, I'm hoping there can be some reconciliation between this repo and the fork so the development efforts can all go in one place. Despite my small troubles with temperature units, the functionality this repo provides is quite important and I can say that it's a great integration.

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.

Adding support for imperial units... again
4 participants