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.

Sign up for GitHub

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

[homematic] Validate datapoint values before writing to config #12557

Merged
merged 2 commits into from
Apr 3, 2022
Merged

[homematic] Validate datapoint values before writing to config #12557

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fa4d32b
[homematic] Validate datapoint values before writing to config
maniac103 Apr 1, 2022
4fac861
Address review comments
maniac103 Apr 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 34 additions & 3 deletions ...c/src/main/java/org/openhab/binding/homematic/internal/handler/HomematicThingHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import java.util.Objects;
import java.util.concurrent.Future;

import org.eclipse.jdt.annotation.Nullable;
import org.openhab.binding.homematic.internal.HomematicBindingConstants;
import org.openhab.binding.homematic.internal.common.HomematicConfig;
import org.openhab.binding.homematic.internal.communicator.HomematicGateway;
Expand Down Expand Up @@ -140,8 +141,7 @@ private void doInitializeInBackground() throws GatewayNotAvailableException, Hom
loadHomematicChannelValues(channel);
for (HmDatapoint dp : channel.getDatapoints()) {
if (dp.getParamsetType() == HmParamsetType.MASTER) {
config.put(MetadataUtils.getParameterName(dp),
dp.isEnumType() ? dp.getOptionValue() : dp.getValue());
config.put(MetadataUtils.getParameterName(dp), getValueForConfiguration(dp));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As getValueForConfiguration can retrun null, I am not sure whether this config entry should be set to null, or not set at all ?
I am even not sure if this is valid to set a null value for a config parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only case under which getValueForConfiguration() returns null is if the datapoint holds a null value. In that case I'd say that writing null into the config is correct.
The places I checked that care about the keys present in the map (those that call containsKey() and keySet() seem to be able to deal with null values being present. Not sure about e.g. the UI, but given the previous code put null values in this case (value being null) as well, I guess it should be fine.

}
}
}
Expand Down Expand Up @@ -401,7 +401,7 @@ protected void updateDatapointState(HmDatapoint dp) {
if (dp.getParamsetType() == HmParamsetType.MASTER) {
// update configuration
Configuration config = editConfiguration();
config.put(MetadataUtils.getParameterName(dp), dp.isEnumType() ? dp.getOptionValue() : dp.getValue());
config.put(MetadataUtils.getParameterName(dp), getValueForConfiguration(dp));
updateConfiguration(config);
} else if (!HomematicTypeGeneratorImpl.isIgnoredDatapoint(dp)) {
// update channel
Expand All @@ -420,6 +420,37 @@ protected void updateDatapointState(HmDatapoint dp) {
}
}

private @Nullable Object getValueForConfiguration(HmDatapoint dp) {
if (dp.getValue() == null) {
return null;
}
if (dp.isEnumType()) {
return dp.getOptionValue();
}
if (dp.isNumberType()) {
// For number datapoints that are only used depending on the value of other datapoints,
// the CCU may return invalid (out of range) values if the datapoint currently is not in use.
// Make sure to not invalidate the whole configuration by returning the datapoint's default
// value in that case.
final boolean minValid, maxValid;
if (dp.isFloatType()) {
Double numValue = dp.getDoubleValue();
minValid = dp.getMinValue() == null || numValue >= dp.getMinValue().doubleValue();
maxValid = dp.getMaxValue() == null || numValue <= dp.getMaxValue().doubleValue();
} else {
Integer numValue = dp.getIntegerValue();
minValid = dp.getMinValue() == null || numValue >= dp.getMinValue().intValue();
maxValid = dp.getMaxValue() == null || numValue <= dp.getMaxValue().intValue();
}
if (minValid && maxValid) {
return dp.getValue();
}
logger.warn("Value for datapoint {} is outside of valid range, using default value for config.", dp);
return dp.getDefaultValue();
}
return dp.getValue();
}

/**
* Converts the value of the datapoint to a State, updates the channel and also sets the thing status if necessary.
*/
Expand Down
36 changes: 25 additions & 11 deletions ...ing.homematic/src/main/java/org/openhab/binding/homematic/internal/model/HmDatapoint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
*/
package org.openhab.binding.homematic.internal.model;

import org.eclipse.jdt.annotation.Nullable;
import org.openhab.binding.homematic.internal.misc.MiscUtils;

/**
Expand Down Expand Up @@ -148,21 +149,34 @@ public int getOptionIndex(String option) {
/**
* Returns the value of a option list.
*/
public String getOptionValue() {
if (options != null && value != null) {
int idx = 0;
if (value instanceof Integer) {
idx = (int) value;
} else {
idx = Integer.parseInt(value.toString());
}
if (idx < options.length) {
return options[idx];
}
public @Nullable String getOptionValue() {
Integer idx = getIntegerValue();
if (options != null && idx != null && idx < options.length) {
return options[idx];
}
return null;
}

public @Nullable Integer getIntegerValue() {
if (value instanceof Integer) {
return (int) value;
} else if (value != null) {
return Integer.parseInt(value.toString());
} else {
return null;
}
}

public @Nullable Double getDoubleValue() {
if (value instanceof Double) {
return (double) value;
} else if (value != null) {
return Double.parseDouble(value.toString());
} else {
return null;
}
}

/**
* Returns the max value.
*/
Expand Down