Closed kocherjj closed 2 years ago
Is it the same as #106? Which was "fixed" but I think the developer forgot to undo the last merge that messed it up. Still waiting on that
@melyux - Same issue. Good eyes. 👁️👃👁️
@gysmo38 - Looks like you closed #109 which reverted #108 ("Already done in PR #105") but since #108 (Sep 29, 2020 9:16 AM EDT) was approved after #105 (Sep 29, 2020, 8:43 AM EDT), #108 replaced code that #105 added (mitsubishi2mqtt.ino, L1415) in the master branch. You (or someone) may need to do another new PR to fix this, as the error persists at time of writing.
🐰🕳️ Side note: If anyone wants to see the rabbit hole I went down before I saw those PRs, grab some popcorn.
It appears that when you set the temperature unit to Fahrenheit (Setup - Unit - Temperature unit), publishing a value in Fahrenheit to
<topic>/<Friendly name>/temp/set
results in the value you provided being treated as Celsius, and needlessly converted to Fahrenheit. For example: Publishing70
to<topic>/<Friendly name>/temp/set
results in<topic>/<Friendly name>/state
reportingtemperature
as158
. (Note: the formula for C to F is(F * 1.8) + 32
, or(70 * 1.8) + 32
, which is where the158
came from)What we've got is:
temperature
- The temperature, provided in the MQTT message. Not sure what unit at the time of declaration.temperature_c
- The temperature, converted from Fahrenheit to Celsuis if temperature mode is Fahrenheit.- PR #108 incorrectly calls the
getTemperature
function, which expects the input temperature to be Celsius, and outputs a temperature based on the user-specified temperature mode (useFahrenheit
). The issue here is that the call should have been passed the Celsius version of the temperature (temperature_c
), as passing it a Fahrenheit temperature (e.g. the MQTT-specifiedtemperature
) results in a double-conversion to Fahrenheit whenuseFahrenheit
is True (when the user has set the temperature mode to Fahrenheit).Relevant code:
1408| else if (strcmp(topic, ha_temp_set_topic.c_str()) == 0) { 1409| float temperature = strtof(message, NULL); 1410| float temperature_c = setTemperature(temperature, useFahrenheit); 1411| if (temperature_c < min_temp || temperature_c > max_temp) { 1412| temperature_c = 23; 1413| rootInfo["temperature"] = getTemperature(temperature_c, useFahrenheit); 1414| } else { 1415| rootInfo["temperature"] = getTemperature(temperature, useFahrenheit); 1416| } 1417| hpSendLocalState(); 1418| hp.setTemperature(temperature_c); 1419| hp.update(); ... 1652|// temperature helper functions 1653|float toFahrenheit(float fromCelcius) { 1654| return round(1.8 * fromCelcius + 32.0); 1655|} 1656| 1657|float toCelsius(float fromFahrenheit) { 1658| return (fromFahrenheit - 32.0) / 1.8; 1659|} 1660| 1661|float getTemperature(float temperature, bool isFahrenheit) { 1662| if (isFahrenheit) { 1663| return toFahrenheit(temperature); 1664| } else { 1665| return temperature; 1666| } 1667|} 1668| 1669|float setTemperature(float temperature, bool isFahrenheit) { 1670| if (isFahrenheit) { 1671| return toCelsius(temperature); 1672| } else { 1673| return temperature; 1674| } 1675|}
Simplifying:
getTemperature(temperature, isFahrenheit)
: returnstoFahrenheit(temperature)
ifisFahrenheit
is True; elsetemperature
setTemperature(temperature, isFahrenheit)
: returnstoCelsius(temperature)
ifisFahrenheit
is True; elsetemperature
Changes required to fix the issue we're experiencing:
1408| else if (strcmp(topic, ha_temp_set_topic.c_str()) == 0) { 1409| float temperature = strtof(message, NULL); 1410| float temperature_c = setTemperature(temperature, useFahrenheit); 1411| if (temperature_c < min_temp || temperature_c > max_temp) { 1412| temperature_c = 23; 1413| rootInfo["temperature"] = getTemperature(temperature_c, useFahrenheit); 1414| } else { // This is the line that results in converting the temperature (from the MQTT publish) from Celsius to // Fahrenheit if the temperature mode is set to Fahrenheit. This is incorrect, as the provided `temperature` is the // MQTT-provided Fahrenheit temperature; what happens in the `getTemperature()` call is the temperature is // treated as though it's a Celsius temperature, and erroneously converted to Fahrenheit 1415- rootInfo["temperature"] = getTemperature(temperature, useFahrenheit); // One option is to fix the conversion, using the previously calculated Celsius representation of the `temperature`, // when the temperature mode is Fahrenheit; that's quite unnecessary, however, and would just look like this: // - Celsius mode: (Celsius) `temperature` to (Celsius) `temperature_c` to (Celsius) `rootInfo["temperature"]` // - Fahrenheit mode: (Fahrenheit) `temperature` to (Celsius) `temperature_c` back to (Fahrenheit) `rootInfo["temperature"]` 1415+ rootInfo["temperature"] = getTemperature(temperature_c, useFahrenheit); // Another option is instead of doing extra work, skip the double-conversion and use the provided `temperature` 1415+ rootInfo["temperature"] = temperature; 1416| } 1417| hpSendLocalState(); 1418| hp.setTemperature(temperature_c); 1419| hp.update();
However in the end, we don't need to duplicate lines 1413 and 1415 - just move them both under the
if
block that fixes the temperature if it's out of range. We could really just do this, as it's a bit more straightforward and easier to read:1408| else if (strcmp(topic, ha_temp_set_topic.c_str()) == 0) { 1409| float temperature = strtof(message, NULL); 1410| float temperature_c = setTemperature(temperature, useFahrenheit); 1411| if (temperature_c < min_temp || temperature_c > max_temp) { 1412| temperature_c = 23; 1413| temperature = getTemperature(temperature_c, useFahrenheit); 1414| } 1415| rootInfo["temperature"] = temperature;
I just
git clone
the newest code (including #108), fixed this in my own copy of the code, compiled a bin, flashed it OTA, and my publishes to<topic>/<Friendly name>/temp/set
result in the state reporting the correct temperature. I tested both Celsius and Fahrenheit mode, providing temperatures in their respective units, to ensure that there isn't any issue with either one. Both MQTT publishes result in the correct state being returned, as well as the correct temperature actually being sent to the unit.In Celsius temperature mode:
temperature
is a temperature provided in Celsius (e.g.23
)temperature_c
is (Celsius)temperature
left as Celsius (from thesetTemperature
call), asuseFahrenheit
is FalserootInfo["temperature"]
is (Celsius)temperature_c
left as Celsius (from thegetTemperature
call), asuseFahrenheit
is FalseIn Fahrenheit temperature mode:
temperature
is a temperature provided in Fahrenheit (e.g.70
)temperature_c
is (Fahrenheit)temperature
converted to Celsius (from thesetTemperature
call), asuseFahrenheit
is TruerootInfo["temperature"]
is (Celsius)temperature_c
converted to Fahrenheit (from thegetTemperature
call), asuseFahrenheit
is TrueIn BOTH cases:
rootInfo["temperature"]
is a value in the user-specified temperature unit, updated in the MQTT status topic (<topic>/<Friendly name>/status
)temperature_c
is a value in Celsius, that is passed tohp.setTemperature()
, which always expects the value to be in Celsius.
Wow. I feel like these variables could use some clarification - instead of having generic temperature
, min_temp
, minTemp
, max_temp
, maxTemp
, etc - all of these should clarify which units they are in. In the case where temperature is read from MQTT where it could be either Celcius or Fahrenheit, then leave off the units - but in all other cases, I think it should be clarified so this code is easier to understand.
I would make a PR myself, but I'm just encountering this project now and have not finished the hardware bits yet to connect my heat pumps and get the code running. If this still remains an issue when I get to that, I'll do my own PR for that (if I remember to do it!).
Wow. I feel like these variables could use some clarification - instead of having generic
temperature
,min_temp
,minTemp
,max_temp
,maxTemp
, etc - all of these should clarify which units they are in. In the case where temperature is read from MQTT where it could be either Celcius or Fahrenheit, then leave off the units - but in all other cases, I think it should be clarified so this code is easier to understand.I would make a PR myself, but I'm just encountering this project now and have not finished the hardware bits yet to connect my heat pumps and get the code running. If this still remains an issue when I get to that, I'll do my own PR for that (if I remember to do it!).
Yeah honestly that's some of the issue here - you have to dig into the code to figure out what the intent of all of the functions and variables are; who would've ever guessed getTemperature
converts a given temperature in Celsius to Fahrenheit if the user sets the temperature mode to Fahrenheit, and setTemperature
converts a given temperature in Fahrenheit to Celsius if the user sets the temperature mode to Fahrenheit? Personally I would've assumed getTemperature
would be, you know, getting the temperature from something... and setTemperature
would be used to set it somewhere. That's aside from the unit, as you mentioned, that it expects.
The thing I'm having trouble wrapping my brain around is getTemperature
takes a temperature in Celsius and turns it into Fahrenheit if the user wants to use Fahrenheit... Okay, that makes sense! Make it user friendly! But setTemperature
turns a temperature in Fahrenheit into Celsius if the user wants to use Fahrenheit? I'm really trying to dig in and understand the logic on this one. It's just counter-intuitive - it's like "I'll give you Fahrenheit and you give me Celsius even though I asked for Fahrenheit".
I think a lot of this can be cleared up by re-labeling variables and functions, as you mentioned.
Edit: So after looking more into setTemperature
, it appears to be used to convert the user-specified temperature to Celsius, for all of the things that expect to only ever receive Celsius.
Temperature mode Celsius:
getTemperature
accepts a temperature in Celsius and returns it, unconverted, in the user-defined unit (Celsius), as the user expects to see temperatures in Celsius.setTemperature
accepts a temperature in the user-defined unit (Celsius) and returns it, unconverted, in Celsius, as HeatPump::setTemperature
expects Celsius.Temperature mode Fahrenheit:
getTemperature
accepts a temperature in Celsius and converts it to the user-defined unit (Fahrenheit), as the user expects to see temperatures in Fahrenheit.setTemperature
accepts a temperature in the user-defined unit (Fahrenheit) and converts it to Celsius, as HeatPump::setTemperature
expects Celsius.So in summary:
getTemperature
could be renamed to tempCelsiusToUserDefinedUnit
as it always converts a temperature from Celsius to the user-defined temperature unit.setTemperature
could be renamed to tempUserDefinedUnitToCelsius
as it always converts a temperature from the user-defined unit to Celsius.Down the rabbit hole we go! 🐇🕳️
Well, I wrote the original code to enable Fahrenheit values since I live in the US. I called it getTemperature
because it was intended, as you intuited, to display the temperature in the unit you want. setTemperature
was necessary because the SwiCago HeatPump library only handles Celsius values, and I needed an equivalent method to only set the temp for the unit.
Since this is turning out to be confusing for people, I agree those methods could be renamed. convertCelsiusToLocalUnit
and convertLocalUnitToCelsius
are also possibilities.
A huge problem is that for most people in the world, they can just use the unlocalized Celsius temperature variable and it works, because they don't use Fahrenheit. (This is SwiCago's attitude, fwiw: he's basically said 'I'm not going to refactor my code for Fahrenheit users.') But if you need Fahrenheit values, and someone doesn't bother using the conversion methods everything breaks spectacularly.
Awesome. Yeah personally I'm in the US as well, and Fahrenheit is pretty much baked in my brain. 😄 I do remember that 20°C (68°F) and 25°C (77°F) are the bounds for what's generally considered "comfortable" but a quick glance at a temperature in Fahrenheit is always easier than trying to remember the mappings of C to F.
I really appreciate the work you've done, it makes the MQTT interface with the Mitsubishi units a million times more useful. Your suggestions of convertCelsiusToLocalUnit
and convertLocalUnitToCelsius
sound good! I could open a PR for you if you like, give me a shout and I'll get one going.
Yeah, that'd be great. Please open a PR. Maybe we need inline comments on the methods, or a wiki page or README entry on how the code handles C/F.
PR Merged
First of all, thanks for the work that's gone into this program! I was stumbling through my own micropython implementation when I found this and it saved me a lot of time! I have experienced the same Fahrenheit bug as others so I have been using Celsius and converting in Node Red, which adds an undesirable MQTT layer. I saw that the latest merge addressed the among other things so I pulled the files and tried it out and there is still an issue. When I set it to Celsius and publish 20 or 30, those are the temps that come back and the units appear to respond correctly. If I set it to Fahrenheit and publish 67 the unit reports back that it is set to 153. If I publish 85 it comes back with 185.
Also, if anyone is keeping a list of units that this code works with, these three can be added:
SLZ-KF15NA MLZ-KP09NA MSZ-GL06NA-U1