[senechome] code cleanup and rename of battery state to system state #9535
Conversation
| @@ -118,6 +118,13 @@ private void refresh() { | |||
| refreshCache.getValue(); | |||
| } | |||
|
|
|||
| private void updateChannelState(String channelConstant, org.openhab.core.types.State channelState) { | |||
There was a problem hiding this comment.
State should be added as import:
| private void updateChannelState(String channelConstant, org.openhab.core.types.State channelState) { | |
| private void updateChannelState(String channelConstant, State channelState) { |
| if (channelConsumption != null) { | ||
| updateState(channelConsumption.getUID(), | ||
| new QuantityType<Power>( | ||
| getSenecValue(response.energy.homePowerConsumption).setScale(2, RoundingMode.HALF_UP), | ||
| Units.WATT)); |
There was a problem hiding this comment.
I don't know if it's relevant. But what changed here is that the creation of the QuantityType instance is always done even if channelConsumption is null. I don't know if that could result in errors, which could happen if the value cannot be evaluated if channelConsumption is null and in case that dependency does exist.
If this is an actual problem you could pass the QuantityType as Consumer. Like: () -> new QuantityType(... and then in the method it can be called when the parameter is not null.
There was a problem hiding this comment.
Hey,
I don't see the problem that you mention in the code. The Data is passed by getSenecValue() from the Senec Device. So if the channel is null or not has no influence on the creation of the QuantityType, only the .getUID() will not work on a null Object.
I actually don't understand the check for null, how can a channel be null?
Cheers
There was a problem hiding this comment.
getThing().getChannel(
The above can return NULL if the channel does not exist. However you have removed the use of this and instead used a constant instead which I don't see the issue in doing it this way. The only thing to comment on is you can use an import and change it to...
updateChannelState(CHANNEL_SENEC_POWER_CONSUMPTION, new QuantityType<Power>(.............
If you did a find replace on SenecHomeBindingConstants. it would be a quick change to make.
Signed-off-by: Korbinian Probst <[email protected]>
Signed-off-by: Korbinian Probst <[email protected]>
Signed-off-by: Korbinian Probst <[email protected]>
Skinah
added
rebuild
|
@KorbinianP thank you for both this PR and for helping with the other PR here #10687 |
Hey,
While testing with OH3 I realized that the name "Battery State" is misleading. The State is of the whole PV system (Battery, Inverter, ...), so I renamed it to System State. This will lead to incompatibilities with older configs.
While reviewing the code I also found it quite ugly to have inside SenecHomeHandler:refreshState() function so much repeated code. I extracted it to a separate function and cleaned the refreshState() function. Saved about 40 lines of code for the same functionality and improved readability.
As usual please find here my self compiled jar for testing:
org.openhab.binding.senechome-3.1.0-SNAPSHOT.jar.zip
Cheers,
Korbinian