TransformationProfile and SystemOffsetProfile #5679

triller-telekom · 2018-06-04T12:00:09Z

This PR includes two new profiles.

The TransformationProfile is a new system profile which is registered
under the scope "transform". It offers the possibility to use all
TransformationServices on ChannelItemLinks. The Strings send from a device
will be transformed via the specified function in the "pattern" parameter.
The other direction, i.e. Item to device is left untouched.

The SystemOffsetProfile lives in the "system" scope of profiles and is able
to add or substract and offset from a value send from or to a device. This
profile works in both directions by applying the opposite offset for the
opposite direction. QuantityTypes are supported as well, i.e. one can put
an offset of "1°C" on a channel that sends Fahrenheit.

In addition there is a new "ChattyThing" fort he magic binding which sends
random values on a Number channel and random Strings on a StringType channel
based on a configurable interval. For profile testing this turned out to be
quite useful.

Signed-off-by: Stefan Triller [email protected]

triller-telekom · 2018-06-04T12:00:15Z

I have marked this PR as WIP because there is one thing left to discuss:

Currently I allow offsets without a unit and then implicitly assume its the same as the one send to the channel.
However, a user might not know this type or this type is changed in a future version of the binding and this will create confusing results. So we mighty only want to allow offsets without a unit if the binding only sends plain Number states without units.

sjsf · 2018-06-05T11:50:00Z

...ng/src/main/java/org/eclipse/smarthome/core/thing/internal/profiles/SystemOffsetProfile.java

+@NonNullByDefault
+public class SystemOffsetProfile implements StateProfile {
+
+    private static Logger logger = LoggerFactory.getLogger(SystemOffsetProfile.class);


final please, not static

sjsf · 2018-06-05T11:54:32Z

...ng/src/main/java/org/eclipse/smarthome/core/thing/internal/profiles/SystemOffsetProfile.java

+                        paramValue, OFFSET_PARAM);
+            }
+        } else {
+            logger.error("Parameter '{}' is not of type String", OFFSET_PARAM);


Isn't it surprising that an offset has to be a String? Especially in the context of plain numeric types this is counter-intuitive.

It is of type String because I allow Quantitytypes, what else shall I use for configuration parameters like "3.5 °C"?

Allowing strings in order to enable QuantityTypes is one thing and is great. Not allowing numeric values and logging them as an error is another. Isn't it?

sjsf · 2018-06-05T11:56:08Z

...ng/src/main/java/org/eclipse/smarthome/core/thing/internal/profiles/SystemOffsetProfile.java

+
+    @Override
+    public void onStateUpdateFromItem(State state) {
+        logger.debug("State update from item: '{}'", state);


Don't add yet more debug logging here. We have enough of it already on state updates and the like, I'd say. It doesn't add any value here.

sjsf · 2018-06-05T11:56:13Z

...ng/src/main/java/org/eclipse/smarthome/core/thing/internal/profiles/SystemOffsetProfile.java

+
+    @Override
+    public void onCommandFromItem(Command command) {
+        logger.debug("Command from item: '{}'", command);


sjsf · 2018-06-05T11:56:18Z

...ng/src/main/java/org/eclipse/smarthome/core/thing/internal/profiles/SystemOffsetProfile.java

+
+    @Override
+    public void onCommandFromHandler(Command command) {
+        logger.debug("Command from handler: '{}'", command);


sjsf · 2018-06-05T12:49:55Z

docs/documentation/development/bindings/profiles.md

+The `OffsetProfile` provides the possibility to adjust a value from a device before it arrives at the framework.
+An offset can be specified via the parameter `offset` which has to be a `QuantityType` or `DecimalType`.
+A positive offset is the amount of change from the device towards the framework and vice versa.


No, not "vice versa". You'll have to stick to explaining a single direction if this sentence should be of any help.

sjsf · 2018-06-05T12:50:54Z

docs/documentation/development/bindings/profiles.md

+The function and format of the value can be specified in the `pattern` parameter of the `TransformationProfile` as a string, separated by a colon.
+Note that these transformations are unidirectional, i.e. they are configured to transform the value send by a device towards the framework, only.
+Commands send by the framework towards the device are passed through the `Profile` without any adjustments.


send -> sent

sjsf · 2018-06-05T12:51:20Z

docs/documentation/development/bindings/profiles.md

+The function and format of the value can be specified in the `pattern` parameter of the `TransformationProfile` as a string, separated by a colon.
+Note that these transformations are unidirectional, i.e. they are configured to transform the value send by a device towards the framework, only.
+Commands send by the framework towards the device are passed through the `Profile` without any adjustments.


`Profile` -> profile

sjsf · 2018-06-05T12:54:03Z

docs/documentation/development/bindings/profiles.md

+
+```java
+Number MyItem { channel="<bindingID>:<thing-typeID>:MyThing:myChannel"[profile="offset", pattern="<transformFunction>:<valueFormat>"]}


You are mixing here BNF notation (e.g. <bindingID>) with concrete examples (e.g. MyThing). Both make sense, but I wouldn't mix them in the same example.

sjsf · 2018-06-05T12:54:13Z

docs/documentation/development/bindings/profiles.md

+
+```java
+String MyItem { channel="<bindingID>:<thing-typeID>:MyThing:myChannel"[profile="transform:<transformationName>", pattern="<transformFunction>:<valueFormat>"]}


triller-telekom · 2018-06-07T11:03:11Z

I have removed the WIP because for backward compatibility I now allow offsets of type DecimalType on states of type QuantityType, but log a warning that the user should check his setup.

triller-telekom · 2018-06-15T15:05:09Z

I have set this PR to WIP again because there is again one thing to discuss:

For transformations I started from the item definitions which have "Window [MAP(window_esp.map):%s]" in their state descriptions for example. The question is whether the formatting of the state before the transformation (the %s) is useful to the profiles or not. I have implemented it in the TransformationProfile but i am not sure whether we only should transform the raw state without any possibility to format the state before it will be transformed.

I have also added configdescriptions for the transformation profiles.

WDYT?

triller-telekom · 2018-06-19T14:11:01Z

Finally I implemented the special handling of temperatures so one can have "3°F" as an offset on a channel that sends values in "°C".

triller-telekom · 2018-06-19T14:11:41Z

This PR is now ready for review.

triller-telekom · 2018-06-21T09:05:37Z

Arrrg: Next time I'll leave a conceptual PR like this longer in the WIP state...

An update will follow: There should not be one single Transformationprofile implementation that picks up all available TransformationServices, but instead each TransformationService should provide its own factory and profile(s).

An update will follow.

This PR includes two new profiles. The TransformationProfile is a new system profile which is registered under the scope "transform". It offers the possibility to use all TransformationServices on ChannelItemLinks. The Strings send from a device will be transformed via the specified function in the "pattern" parameter. The other direction, i.e. Item to device is left untouched. The SystemOffsetProfile lives in the "system" scope of profiles and is able to add or substract and offset from a value send from or to a device. This profile works in both directions by applying the opposite offset for the opposite direction. QuantityTypes are supported as well, i.e. one can put an offset of "1°C" on a channel that sends Fahrenheit. In addition there is a new "ChattyThing" fort he magic binding which sends random values on a Number channel and random Strings on a StringType channel based on a configurable interval. For profile testing this turned out to be quite useful. Signed-off-by: Stefan Triller <[email protected]>

Signed-off-by: Stefan Triller <[email protected]>

…rofiles. Signed-off-by: Stefan Triller <[email protected]>

Signed-off-by: Stefan Triller <[email protected]>

htreu

Thanks @triller-telekom looks good overall. Since a lot of the transformation services look similar the comments about logging apply to all of them.
Also, please review your JavaDoc formatter settings since some changes are only from JavaDoc formatting.

htreu · 2018-06-26T08:44:59Z

...rc/test/java/org/eclipse/smarthome/core/thing/internal/profiles/SystemOffsetProfileTest.java

+import org.mockito.ArgumentCaptor;
+
+/**
+ *


Please add a simple description.

htreu · 2018-06-26T08:52:56Z

bundles/core/org.eclipse.smarthome.core.thing/ESH-INF/config/offsetProfile.xml

+	<config-description uri="profile:system:offset">
+		<parameter name="offset" type="text" required="true">
+			<label>Offset</label>
+			<description>Offset (plain number or number with unit) to be applied on the state towards the item. If positive the negative offset will be applied in the reverse direction.</description>


I would drop If positive and only go with The negative offset ..... Its not a constraint.

htreu · 2018-06-26T09:18:11Z

...ng/src/main/java/org/eclipse/smarthome/core/thing/internal/profiles/SystemOffsetProfile.java

+            BigDecimal bd = (BigDecimal) paramValue;
+            try {
+                offset = new QuantityType<>(bd.toString());
+            } catch (IllegalArgumentException e) {


This should always succeed, is it necessary to try/catch here?

htreu · 2018-06-26T09:21:07Z

...ng/src/main/java/org/eclipse/smarthome/core/thing/internal/profiles/SystemOffsetProfile.java

+        if (finalOffset == null) {
+            logger.error(
+                    "Offset not configured correctly, please make sure it is of type QuantityType, e.g. \"3\", \"-1.4\", \"3.2°C\". Using offset 0 now.");
+            return new QuantityType<Dimensionless>("0");


The error log says "using offset 0 now" but instead "0" is returned.

htreu · 2018-06-26T09:27:29Z

...ng/src/main/java/org/eclipse/smarthome/core/thing/internal/profiles/SystemOffsetProfile.java

+    private Type applyOffset(Type state, boolean towardsItem) {
+        QuantityType finalOffset = offset;
+        if (finalOffset == null) {
+            logger.error(


This should only happen when the constructor failed somehow. In this case an error is already logged. I suggest logging warn here.

htreu · 2018-06-26T11:22:03Z

...ipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/library/types/QuantityType.java

+        list.add(quantity);
+        list.add(offset.quantity);
+
+        final Quantity<T> sum = list.stream().reduce(QuantityFunctions.sum(unit)).get();


This might also be Arrays.asList(quantity, offset.quantity).stream().reduce(QuantityFunctions.sum(unit)).get();

htreu · 2018-06-26T13:43:26Z

...n/java/org/eclipse/smarthome/transform/exec/internal/profiles/ExecTransformationProfile.java

+            function = (String) paramFunction;
+            sourceFormat = (String) paramSource;
+        } else {
+            logger.warn("Parameter '{}' and '{}' have to be Strings. Profile will be inactive.", FUNCTION_PARAM,


should we log error here during construction time and warn later on in onCommandFromHandler and onStateUpdateFromHandler?

htreu · 2018-06-26T13:44:37Z

...n/java/org/eclipse/smarthome/transform/exec/internal/profiles/ExecTransformationProfile.java

+        try {
+            result = TransformationHelper.transform(service, function, sourceFormat, state.toFullString());
+        } catch (TransformationException e) {
+            logger.debug("Could not transform state '{}' with function '{}' and format '{}'", state, function,


better log this as a warning? is a transformation supposed to work every time? Or is it the default behaviour to not work for every incoming command/state?

htreu · 2018-06-26T14:08:51Z

extensions/transform/org.eclipse.smarthome.transform.xslt/ESH-INF/config/xsltProfile.xml

+    <config-description uri="profile:transform:XSLT">
+        <parameter name="function" type="text" required="true">
+            <label>XSL Filename</label>
+            <description>XSL Filename in transform folder, containing the transformation expression.</description>


filename or better file name.

htreu · 2018-06-26T14:10:39Z

extensions/transform/org.eclipse.smarthome.transform.xslt/README.md

+String <itemName> { channel="<channelUID>"[profile="transform:XSLT", function="<xsltExpression>", sourceFormat="<valueFormat>"]}
+```
+
+The XSLT expression to be executed has to be set in the `function` parameter.


in the config description the function parameter is supposed to hold a filename. Here it should be the XSLT expression...

edit: the XsltTransformationService expects it to be the file name.

Signed-off-by: Stefan Triller <[email protected]>

htreu

LGTM, thanks.

comments have been addressed

openhab-bot · 2018-07-02T20:39:32Z

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/transformationservice-of-type-scale-is-unavailable/47236/9

Because the TransformationHelper throws a TransformationException to the caller since PR eclipse-archived#5679, it is now possible to provide a meaningful warning where the transformation is started, i.e. provide the item name. Fixes eclipse-archived#5120 Signed-off-by: Stefan Triller <[email protected]>

…ls (#5900) Because the TransformationHelper throws a TransformationException to the caller since PR #5679, it is now possible to provide a meaningful warning where the transformation is started, i.e. provide the item name. Fixes #5120 Signed-off-by: Stefan Triller <[email protected]>

triller-telekom force-pushed the offsetProfile branch 4 times, most recently from d578c89 to dc2b836 Compare June 5, 2018 11:40

sjsf previously requested changes Jun 5, 2018

View reviewed changes

triller-telekom changed the title ~~WIP: TransformationProfile and SystemOffsetProfile~~ TransformationProfile and SystemOffsetProfile Jun 7, 2018

kaikreuzer mentioned this pull request Jun 7, 2018

RfC: How to treat addition/substraction for temperature values #5697

Closed

triller-telekom force-pushed the offsetProfile branch from d82a025 to 1afd3b6 Compare June 13, 2018 06:29

triller-telekom changed the title ~~TransformationProfile and SystemOffsetProfile~~ WIP: TransformationProfile and SystemOffsetProfile Jun 15, 2018

kaikreuzer mentioned this pull request Jun 18, 2018

[meteostick] Advanced channels for wind statistics openhab/openhab-addons#3675

Merged

triller-telekom changed the title ~~WIP: TransformationProfile and SystemOffsetProfile~~ TransformationProfile and SystemOffsetProfile Jun 19, 2018

triller-telekom changed the title ~~TransformationProfile and SystemOffsetProfile~~ WIP: TransformationProfile and SystemOffsetProfile Jun 21, 2018

htreu mentioned this pull request Jun 21, 2018

[OneWire] Initial contribution #4287

Merged

5 tasks

triller-telekom added 10 commits June 21, 2018 17:01

Incorporate review changes

dd0f4eb

Signed-off-by: Stefan Triller <[email protected]>

Fix failing test after changes

5f214e0

Signed-off-by: Stefan Triller <[email protected]>

Allow BigDecimal as offset parameter and improve warning message

d0ee464

Signed-off-by: Stefan Triller <[email protected]>

Allow decimal offsets on quantity states, but with warning

9f3246c

Signed-off-by: Stefan Triller <[email protected]>

ConfigDescription for OffsetProfile

246e265

Signed-off-by: Stefan Triller <[email protected]>

Split function and format into 2 parameters. ConfigDescriptions for p…

aff83fb

…rofiles. Signed-off-by: Stefan Triller <[email protected]>

Turn state formatter into an advanced parameter

e7b193b

Signed-off-by: Stefan Triller <[email protected]>

Advanced parameters should not be required

4ed2b9c

Signed-off-by: Stefan Triller <[email protected]>

Special handling of temperatures in offsetprofile

42d147e

Signed-off-by: Stefan Triller <[email protected]>

triller-telekom added 6 commits June 21, 2018 17:01

Do the math for temperature offsets in kelvin

782a368

Signed-off-by: Stefan Triller <[email protected]>

Extract variables into constants to speed up calculations

d8f40ad

Signed-off-by: Stefan Triller <[email protected]>

Create one profile and factory per transformation service bundle

9088d69

Signed-off-by: Stefan Triller <[email protected]>

Documented profile usage in transformation services

eaaa8e8

Signed-off-by: Stefan Triller <[email protected]>

USE DS-annotations for transformation services

cf186f5

Signed-off-by: Stefan Triller <[email protected]>

Fix parameter name and DS service of map transformation

f838438

Signed-off-by: Stefan Triller <[email protected]>

triller-telekom force-pushed the offsetProfile branch from 2bae966 to f838438 Compare June 21, 2018 15:02

triller-telekom added 2 commits June 22, 2018 10:11

Fix Javadoc

3135ad7

Signed-off-by: Stefan Triller <[email protected]>

Supported Items for OffsetProfile are only NumberItems

27f2411

Signed-off-by: Stefan Triller <[email protected]>

htreu suggested changes Jun 26, 2018

View reviewed changes

triller-telekom added 2 commits June 27, 2018 10:19

Address review comments (except javadoc format)

660f10d

Signed-off-by: Stefan Triller <[email protected]>

Fix formatting errors

4bd7ff0

Signed-off-by: Stefan Triller <[email protected]>

triller-telekom changed the title ~~WIP: TransformationProfile and SystemOffsetProfile~~ TransformationProfile and SystemOffsetProfile Jun 27, 2018

htreu approved these changes Jun 27, 2018

View reviewed changes

kaikreuzer merged commit cf01b86 into eclipse-archived:master Jun 27, 2018

triller-telekom deleted the offsetProfile branch June 27, 2018 10:35

triller-telekom mentioned this pull request Jul 12, 2018

Provide a more meaningful warning if state transformation on item state fails #5900

Merged

htreu added this to the 0.10.0 milestone Oct 30, 2018

wborn mentioned this pull request Jun 5, 2019

[UoM] Absolute vs. Relative Temperature openhab/openhab-core#855

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TransformationProfile and SystemOffsetProfile #5679

TransformationProfile and SystemOffsetProfile #5679

triller-telekom commented Jun 4, 2018

triller-telekom commented Jun 4, 2018

sjsf Jun 5, 2018

sjsf Jun 5, 2018

triller-telekom Jun 5, 2018

sjsf Jun 6, 2018

sjsf Jun 5, 2018

sjsf Jun 5, 2018

sjsf Jun 5, 2018

sjsf Jun 5, 2018

sjsf Jun 5, 2018

sjsf Jun 5, 2018

sjsf Jun 5, 2018

sjsf Jun 5, 2018

triller-telekom commented Jun 7, 2018

triller-telekom commented Jun 15, 2018

triller-telekom commented Jun 19, 2018

triller-telekom commented Jun 19, 2018

triller-telekom commented Jun 21, 2018

htreu left a comment

htreu Jun 26, 2018

htreu Jun 26, 2018

htreu Jun 26, 2018

htreu Jun 26, 2018

htreu Jun 26, 2018

htreu Jun 26, 2018

htreu Jun 26, 2018

htreu Jun 26, 2018

htreu Jun 26, 2018

htreu Jun 26, 2018

htreu left a comment

openhab-bot commented Jul 2, 2018


		```java
		Number MyItem { channel="<bindingID>:<thing-typeID>:MyThing:myChannel"[profile="offset", pattern="<transformFunction>:<valueFormat>"]}


		```java
		String MyItem { channel="<bindingID>:<thing-typeID>:MyThing:myChannel"[profile="transform:<transformationName>", pattern="<transformFunction>:<valueFormat>"]}

TransformationProfile and SystemOffsetProfile #5679

TransformationProfile and SystemOffsetProfile #5679

Conversation

triller-telekom commented Jun 4, 2018

triller-telekom commented Jun 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

triller-telekom commented Jun 7, 2018

triller-telekom commented Jun 15, 2018

triller-telekom commented Jun 19, 2018

triller-telekom commented Jun 19, 2018

triller-telekom commented Jun 21, 2018

htreu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htreu left a comment

Choose a reason for hiding this comment

openhab-bot commented Jul 2, 2018