[radiothermostat] Added Absolute Setpoint Mode #8393

silverfunk · 2020-09-04T04:20:31Z

When controlled remotely Radio Thermostat's can function in 2 modes. In temporary mode setpoint changes will be temporary; with the thermostat returning to run its program after a period of time. In absolute mode setpoint changes are permanent. The thermostat will maintain the given setpoint temperature until it is changed, completely ignoring the program. With OpenHab controlling the thermostat it is often not desirable to have the thermostat run its own program. Adding the absolute setpoint mode allows users to control the thermostat setpoint entirely through OpenHab.

TravisBuddy · 2020-09-04T04:24:22Z

Travis tests were successful

Hey @silverfunk,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Signed-off-by: Ted Jordan <[email protected]>

TravisBuddy · 2020-09-04T04:38:35Z

Travis tests were successful

Hey @silverfunk,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Hilbrand · 2020-09-04T06:41:26Z

I'm not familiar with this device. But Is this something that is set once and than not changed? Or does it make sense for the user to sometimes change the setpoint temporary and other times permanent? In that case it might make more sense to have this as a channel parameter of a separate temporarySetPoint channel?

marcelrv · 2020-09-04T09:00:11Z

Indeed afree with @Hilbrand would suggest the same.
Make some sort of 'mode' channel instead, which than would be like program mode or permanent instead of doing this via config option.

I would also recommend not to use 'absolute' as channel name, I don't think it well describes the intent of the functionality

Hilbrand · 2020-09-04T09:05:09Z

I would opt for a specific channel for temporarySetPoint as it would be easier. The user can simply pick a channel. Otherwise the user must first make sure the correct mode is set and then set the setpoint.

bundles/org.openhab.binding.radiothermostat/src/main/resources/ESH-INF/thing/thing-types.xml

TravisBuddy · 2020-09-05T12:40:43Z

Travis tests were successful

Hey @silverfunk,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

mlobstein · 2020-09-08T01:53:56Z

I would opt for a specific channel for temporarySetPoint as it would be easier. The user can simply pick a channel. Otherwise the user must first make sure the correct mode is set and then set the setpoint.

I am not entirely sure this is needed. With the current implementation, the user can set the 'hold' switch on which will cause the temperature to maintain its current set point indefinitely. The thermostat will ignore its internal program schedule while in hold mode.

mlobstein · 2020-09-08T01:56:34Z

bundles/org.openhab.binding.radiothermostat/README.md

+| logRefresh      | Overrides the refresh interval of the run-time logs & humidity data. Optional, the default is 10 minutes.                                                                                                                                                                                |
+| isCT80          | Flag to enable additional features only available on the CT80 thermostat. Optional, the default is false.                                                                                                                                                                                |
+| disableLogs     | Disable retrieval of run-time logs from the thermostat. Optional, the default is false.                                                                                                                                                                                                  |
+| absolute        | Controls temporary or absolute mode. When false setpoint changes are temporary; the thermostat will return to its program after a time. When true setpoint changes are perminant; the thermostat will ignore its program maintaining the given setpoint. Optional, the default is false. |


spelling: 'permanent'

silverfunk · 2020-09-08T02:29:48Z

The main idea behind creating an absolute mode is to allow the thermostat to be operated primarily via a program in the thermostat (thermostat driven) vs. operated via a smart home controller (openHAB). Without an absolute mode the thermostat only temporarily changes to the given setpoint, reverting later back to its program. Yes, hold could be used to force this same behavior but then hold cannot be used for smart home controller operations. For example I use hold on my thermostats to inform openHAB that it should maintain the setpoint regardless of whether or not people are home, e.g. it is hold for the thermostat and the smart home controller.

The real point of adding an absolute mode is to essentially make the thermostat a dumb device. In this mode it controls the temperature based solely on what openHAB tells it to do. I do not want it to have other ideas of how and when the temperature should be changed. In my house openHAB is able to tell the thermostat to control the temperature based on when people are home, whether or not we are in bed for the night, and the price I am paying for electricity. In an absolute mode the thermostat's only function is to monitor the temperature and turn on the furnace or AC when it gets beyond the setpoint ranges. I do not want the thermostat to be able to make decisions beyond what openHAB tells it to do.

When I originally contemplated this functionality I dithered between making it a parameter on the thing or a separate item channel. Ultimately I decided to go with a parameter on the thing because I could not fathom a reason why anyone would want to switch between the two. But that being said I am open to allowing for flexibility. If there is a desire to make the absolute mode into an item channel I can do that. I do think this would have to be an on/off switch though. Creating separate temporary and absolute setpoint items, while possible, would be complicated for a user interface. The thermostat is only able to maintain one active setpoint, either temporary or absolute. If both setpoints were items you could have a sitemap where both are visible but there is no indication which one is active. Changing one would inherently change the mode of the thermostat with no indication the thermostat was functioning differently.

TravisBuddy · 2020-09-08T02:33:41Z

Travis tests were successful

Hey @silverfunk,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

…/ESH-INF/thing/thing-types.xml Co-authored-by: cpmeister <[email protected]> Signed-off-by: Ted Jordan <[email protected]>

TravisBuddy · 2020-09-08T02:45:23Z

Travis tests were successful

Hey @silverfunk,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Co-authored-by: mlobstein Signed-off-by: Ted Jordan <[email protected]>

TravisBuddy · 2020-09-08T02:53:09Z

Travis tests were successful

Hey @silverfunk,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Hilbrand

Thank you for your extensive explanation. I now better understand it's usage. This is more about how to one wants to use the thermostat. I can image that if you want to run in absolute mode you never should send a temporary command as this will trigger the thermostat to start following it's program again. The naming of this option can be a discussion, but I guess it would best make sense to follow naming closely to what the thermostat uses and thus follow the terminology used as used in the pr.

I've one review request. Instead of using the a boolean to make it a options list. You can actually test this value in the initialize method and set the specific command, instead of testing it every time, since doesn't change anymore.

Hilbrand · 2020-09-08T13:12:15Z

bundles/org.openhab.binding.radiothermostat/src/main/resources/ESH-INF/thing/thing-types.xml

+			<parameter name="absolute" type="boolean">
+				<label>Run in Absolute Setpoint Mode</label>
+				<description>Optional Flag to run in absolute or temporary setpoint mode</description>
+				<default>false</default>
+			</parameter>


Suggested change

<parameter name="absolute" type="boolean">

<label>Run in Absolute Setpoint Mode</label>

<description>Optional Flag to run in absolute or temporary setpoint mode</description>

<default>false</default>

</parameter>

<parameter name="setpointMode" type="text">

<label>Setpoint Mode</label>

<description>Run in absolute or temporary setpoint mode</description>

<default>temporary</default>

<options>

<option value="absolute">Absolute</option>

<option value="temporary">Temporary</option>

</options>

</parameter>

mlobstein · 2020-09-08T16:07:44Z

The main idea behind creating an absolute mode is to allow the thermostat to be operated primarily via a program in the thermostat (thermostat driven) vs. operated via a smart home controller (openHAB). Without an absolute mode the thermostat only temporarily changes to the given setpoint, reverting later back to its program.

That makes sense. I had never explored absolute mode myself. It is rather strange that there is no way to tell if the thermostat is using an absolute setpoint or not. So from the binding perspective it makes sense to chose one mode or the other and stick with it instead of providing a way to dynamically switch. I have attached some refactoring changes to the code to remove the SuppressWarnings mainly and improve a few little things. Please add them to your PR (note .java files renamed to .txt)

RadioThermostatConfiguration.java.txt

RadioThermostatStateDescriptionProvider.java.txt

RadioThermostatHandler.java.txt

fwolter · 2020-09-08T19:48:26Z

@mlobstein You could also file a PR for the forked repo of @silverfunk

Changed the absolute parameter to setpointMode. Signed-off-by: silverfunk <[email protected]>

TravisBuddy · 2020-09-09T02:02:00Z

Travis tests have failed

Hey @silverfunk,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

Signed-off-by: silverfunk <[email protected]>

mlobstein · 2020-09-09T02:13:53Z

...c/main/java/org/openhab/binding/radiothermostat/internal/handler/RadioThermostatHandler.java

+        this.isCT80 = config.isCT80;
+        this.disableLogs = config.disableLogs;
+
+        if (hostName == null || hostName == "") {


Should probably be hostName.equals(""), sorry about that.

...c/main/java/org/openhab/binding/radiothermostat/internal/handler/RadioThermostatHandler.java

TravisBuddy · 2020-09-09T02:20:44Z

Travis tests were successful

Hey @silverfunk,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

silverfunk · 2020-09-09T02:21:52Z

@mlobstein, I incorporated your refactoring changes. Thank you for the updates.

In temporary mode the thermostat displays "Temporary" on the thermostat's screen while it is adjusting to the given temporary setpoint. In absolute mode it does not display "Temporary". That's the only indication I've seen that differentiates between the two modes. I have found nothing in the API to designate a difference between the modes.

Signed-off-by: silverfunk <[email protected]>

TravisBuddy · 2020-09-09T03:24:42Z

Travis tests were successful

Hey @silverfunk,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Signed-off-by: silverfunk <[email protected]>

TravisBuddy · 2020-09-09T03:36:42Z

Travis tests were successful

Hey @silverfunk,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Hilbrand

LGTM

marcelrv

LGTM

Hilbrand · 2020-09-09T10:26:40Z

@mlobstein any last words before I merge 😉

mlobstein · 2020-09-09T13:38:56Z

@mlobstein any last words before I merge wink

LGTM

Signed-off-by: Ted Jordan <[email protected]>

silverfunk added 2 commits September 3, 2020 23:34

Added absolute control mode to the Radio Thermostat binding.

edd222f

Signed-off-by: Ted Jordan <[email protected]>

Explained the absolute parameter is optional.

13475ea

Signed-off-by: Ted Jordan <[email protected]>

silverfunk changed the title ~~Added Absolute Setpoint Mode~~ [radiothermostat] Added Absolute Setpoint Mode Sep 4, 2020

Hilbrand added the enhancement An enhancement or new feature for an existing add-on label Sep 4, 2020

cpmeister requested changes Sep 4, 2020

View reviewed changes

bundles/org.openhab.binding.radiothermostat/src/main/resources/ESH-INF/thing/thing-types.xml Outdated Show resolved Hide resolved

mlobstein reviewed Sep 8, 2020

View reviewed changes

Update bundles/org.openhab.binding.radiothermostat/src/main/resources…

ed64593

…/ESH-INF/thing/thing-types.xml Co-authored-by: cpmeister <[email protected]> Signed-off-by: Ted Jordan <[email protected]>

Fixed the spelling of permanent.

a6c91b2

Co-authored-by: mlobstein Signed-off-by: Ted Jordan <[email protected]>

Hilbrand reviewed Sep 8, 2020

View reviewed changes

Refactored code to removed suppressed warnings.

507140f

Changed the absolute parameter to setpointMode. Signed-off-by: silverfunk <[email protected]>

Fixed a few code style errors.

d15ea1b

Signed-off-by: silverfunk <[email protected]>

mlobstein requested changes Sep 9, 2020

View reviewed changes

mlobstein reviewed Sep 9, 2020

View reviewed changes

...c/main/java/org/openhab/binding/radiothermostat/internal/handler/RadioThermostatHandler.java Show resolved Hide resolved

Changed a string comparison to use the equals() method.

802d7fb

Signed-off-by: silverfunk <[email protected]>

silverfunk requested a review from cpmeister September 9, 2020 03:28

Forgot to leave the check for null in place.

9f91747

Signed-off-by: silverfunk <[email protected]>

Hilbrand approved these changes Sep 9, 2020

View reviewed changes

marcelrv approved these changes Sep 9, 2020

View reviewed changes

Hilbrand merged commit 811d329 into openhab:2.5.x Sep 9, 2020

CSchlipp pushed a commit to CSchlipp/openhab-addons that referenced this pull request Sep 12, 2020

[radiothermostat] Added Absolute Setpoint Mode (openhab#8393)

0a41185

Signed-off-by: Ted Jordan <[email protected]>

wborn added this to the 2.5.9 milestone Sep 18, 2020

markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Sep 19, 2020

[radiothermostat] Added Absolute Setpoint Mode (openhab#8393)

0ada427

Signed-off-by: Ted Jordan <[email protected]>

andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Oct 8, 2020

[radiothermostat] Added Absolute Setpoint Mode (openhab#8393)

6fb369b

Signed-off-by: Ted Jordan <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[radiothermostat] Added Absolute Setpoint Mode #8393

[radiothermostat] Added Absolute Setpoint Mode #8393

silverfunk commented Sep 4, 2020 •

edited

Loading

TravisBuddy commented Sep 4, 2020

TravisBuddy commented Sep 4, 2020

Hilbrand commented Sep 4, 2020

marcelrv commented Sep 4, 2020

Hilbrand commented Sep 4, 2020

TravisBuddy commented Sep 5, 2020

mlobstein commented Sep 8, 2020

mlobstein Sep 8, 2020

silverfunk commented Sep 8, 2020 •

edited

Loading

TravisBuddy commented Sep 8, 2020

TravisBuddy commented Sep 8, 2020

TravisBuddy commented Sep 8, 2020

Hilbrand left a comment

Hilbrand Sep 8, 2020

mlobstein commented Sep 8, 2020 •

edited

Loading

fwolter commented Sep 8, 2020

TravisBuddy commented Sep 9, 2020

mlobstein Sep 9, 2020

TravisBuddy commented Sep 9, 2020

silverfunk commented Sep 9, 2020

TravisBuddy commented Sep 9, 2020

TravisBuddy commented Sep 9, 2020

Hilbrand left a comment

marcelrv left a comment

Hilbrand commented Sep 9, 2020

mlobstein commented Sep 9, 2020

-			<parameter name="absolute" type="boolean">
-				<label>Run in Absolute Setpoint Mode</label>
-				<description>Optional Flag to run in absolute or temporary setpoint mode</description>
-				<default>false</default>
-			</parameter>
+			<parameter name="setpointMode" type="text">
+				<label>Setpoint Mode</label>
+				<description>Run in absolute or temporary setpoint mode</description>
+				<default>temporary</default>
+				<options>
+				    <option value="absolute">Absolute</option>
+				    <option value="temporary">Temporary</option>
+				</options>
+			</parameter>

[radiothermostat] Added Absolute Setpoint Mode #8393

[radiothermostat] Added Absolute Setpoint Mode #8393

Conversation

silverfunk commented Sep 4, 2020 • edited Loading

TravisBuddy commented Sep 4, 2020

Travis tests were successful

TravisBuddy commented Sep 4, 2020

Travis tests were successful

Hilbrand commented Sep 4, 2020

marcelrv commented Sep 4, 2020

Hilbrand commented Sep 4, 2020

TravisBuddy commented Sep 5, 2020

Travis tests were successful

mlobstein commented Sep 8, 2020

mlobstein Sep 8, 2020

Choose a reason for hiding this comment

silverfunk commented Sep 8, 2020 • edited Loading

TravisBuddy commented Sep 8, 2020

Travis tests were successful

TravisBuddy commented Sep 8, 2020

Travis tests were successful

TravisBuddy commented Sep 8, 2020

Travis tests were successful

Hilbrand left a comment

Choose a reason for hiding this comment

Hilbrand Sep 8, 2020

Choose a reason for hiding this comment

mlobstein commented Sep 8, 2020 • edited Loading

fwolter commented Sep 8, 2020

TravisBuddy commented Sep 9, 2020

Travis tests have failed

mlobstein Sep 9, 2020

Choose a reason for hiding this comment

TravisBuddy commented Sep 9, 2020

Travis tests were successful

silverfunk commented Sep 9, 2020

TravisBuddy commented Sep 9, 2020

Travis tests were successful

TravisBuddy commented Sep 9, 2020

Travis tests were successful

Hilbrand left a comment

Choose a reason for hiding this comment

marcelrv left a comment

Choose a reason for hiding this comment

Hilbrand commented Sep 9, 2020

mlobstein commented Sep 9, 2020

silverfunk commented Sep 4, 2020 •

edited

Loading

silverfunk commented Sep 8, 2020 •

edited

Loading

mlobstein commented Sep 8, 2020 •

edited

Loading