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

Jump to bottom

[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 1 commit
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
36 changes: 33 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 @@ -140,8 +140,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 +400,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 +419,37 @@ protected void updateDatapointState(HmDatapoint dp) {
}
}

private Object getValueForConfiguration(HmDatapoint dp) {
maniac103 marked this conversation as resolved.
Show resolved Hide resolved
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.getMaxValue().doubleValue();
maxValid = dp.getMinValue() == null || numValue <= dp.getMinValue().doubleValue();
maniac103 marked this conversation as resolved.
Show resolved Hide resolved
} else {
Integer numValue = dp.getIntegerValue();
minValid = dp.getMinValue() == null || numValue >= dp.getMaxValue().intValue();
maxValid = dp.getMinValue() == null || numValue <= dp.getMinValue().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
27 changes: 21 additions & 6 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 @@ -150,19 +150,34 @@ public int getOptionIndex(String option) {
*/
public String getOptionValue() {
if (options != null && value != null) {
int idx = 0;
if (value instanceof Integer) {
idx = (int) value;
} else {
idx = Integer.parseInt(value.toString());
}
int idx = getIntegerValue();
maniac103 marked this conversation as resolved.
Show resolved Hide resolved
if (idx < options.length) {
return options[idx];
}
}
return null;
}

public Integer getIntegerValue() {
maniac103 marked this conversation as resolved.
Show resolved Hide resolved
if (value instanceof Integer) {
return (int) value;
} else if (value != null) {
return Integer.parseInt(value.toString());
} else {
return null;
}
}

public Double getDoubleValue() {
maniac103 marked this conversation as resolved.
Show resolved Hide resolved
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