-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[radiothermostat] Add Remote Temperature channel #10194
Conversation
Signed-off-by: Michael Lobstein <[email protected]>
Signed-off-by: Michael Lobstein <[email protected]>
Signed-off-by: Michael Lobstein <[email protected]>
bundles/org.openhab.binding.radiothermostat/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
Signed-off-by: Michael Lobstein <[email protected]>
connector.sendCommand("rem_temp", cmdInt.toString(), REMOTE_TEMP_RESOURCE); | ||
} else { | ||
connector.sendCommand("rem_mode", "0", REMOTE_TEMP_RESOURCE); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should make sure that the units are what you expect before you send the command to the device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am already parsing out the value of the command string into a whole number on line 232. This device is US centric and only supports Fahrenheit so it should already be understood by the user that they are sending a Fahrenheit temperature value to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That may be the case but the user might be using Celsius as their system default so the UI would end up displaying and sending values with that unit instead. Certain countries have easy access to products that use Fahrenheit even though they actually use celsius. E.g. Canada.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to recap this functionality a bit... This new channel is a write-only channel, it does not display anything back, It allows a user to override the internal temperature reading of this thermostat with a temperature value from an external sensor. Presumably a rule would be used to monitor an item containing a temperature sensor's value and when it changes, it would update an item linked to this channel. What ever number is received by the thermostat would then display on its screen as the current temperature reading (a whole number Fahrenheit value) and then be sent back to the main temperature channel (and displayed per whatever unit is specified by their system default settings) when the binding does its next polling update.
I am sure that there are a few of these in Canada even though it was not sold there.....
If someone in a Celsius country is using this, they will simply have to do a conversion of the Celsius temperature value they want to use in the rule that will send the value to this new channel. This will hopefully force them to ensure that a sane value is sent instead of relying on an automatic conversion that might have unpredictable results.
bundles/org.openhab.binding.radiothermostat/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
Signed-off-by: Michael Lobstein <[email protected]>
break; | ||
case REMOTE_TEMP: | ||
if (cmdInt != -1) { | ||
connector.sendCommand("rem_temp", cmdInt.toString(), REMOTE_TEMP_RESOURCE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
connector.sendCommand("rem_temp", cmdInt.toString(), REMOTE_TEMP_RESOURCE); | |
QuantityType<?> quantity = ((QuantityType<Temperature>) command).toUnit(SIUnits.Celsius); | |
connector.sendCommand("rem_temp", String.valueOf(quantity.intValue()), REMOTE_TEMP_RESOURCE); |
Please just make the recommended change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, done.
Signed-off-by: Michael Lobstein <[email protected]>
Signed-off-by: Michael Lobstein <[email protected]>
@fwolter can this be merged in time for 3.1.0.M3? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Add Remote Temperature channel Signed-off-by: Michael Lobstein <[email protected]> * Fix spelling error Signed-off-by: Michael Lobstein <[email protected]> * Fix spelling error2 Signed-off-by: Michael Lobstein <[email protected]> * review changes Signed-off-by: Michael Lobstein <[email protected]> * review changes Signed-off-by: Michael Lobstein <[email protected]> * review changes Signed-off-by: Michael Lobstein <[email protected]> * minor README update Signed-off-by: Michael Lobstein <[email protected]> Signed-off-by: John Marshall <[email protected]>
* Add Remote Temperature channel Signed-off-by: Michael Lobstein <[email protected]> * Fix spelling error Signed-off-by: Michael Lobstein <[email protected]> * Fix spelling error2 Signed-off-by: Michael Lobstein <[email protected]> * review changes Signed-off-by: Michael Lobstein <[email protected]> * review changes Signed-off-by: Michael Lobstein <[email protected]> * review changes Signed-off-by: Michael Lobstein <[email protected]> * minor README update Signed-off-by: Michael Lobstein <[email protected]>
* Add Remote Temperature channel Signed-off-by: Michael Lobstein <[email protected]> * Fix spelling error Signed-off-by: Michael Lobstein <[email protected]> * Fix spelling error2 Signed-off-by: Michael Lobstein <[email protected]> * review changes Signed-off-by: Michael Lobstein <[email protected]> * review changes Signed-off-by: Michael Lobstein <[email protected]> * review changes Signed-off-by: Michael Lobstein <[email protected]> * minor README update Signed-off-by: Michael Lobstein <[email protected]>
* Add Remote Temperature channel Signed-off-by: Michael Lobstein <[email protected]> * Fix spelling error Signed-off-by: Michael Lobstein <[email protected]> * Fix spelling error2 Signed-off-by: Michael Lobstein <[email protected]> * review changes Signed-off-by: Michael Lobstein <[email protected]> * review changes Signed-off-by: Michael Lobstein <[email protected]> * review changes Signed-off-by: Michael Lobstein <[email protected]> * minor README update Signed-off-by: Michael Lobstein <[email protected]>
The goal of this PR is to add a channel that will send values to the Remote Temperature endpoint of the thermostat's API. This functionality was requested by issue #9807. Some minor code cleanup was done also.