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

[hue] Fix potential NPE when new battery devices are added to bridge #16619

Merged
merged 1 commit into from
Apr 6, 2024
Merged

[hue] Fix potential NPE when new battery devices are added to bridge #16619

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 9 additions & 5 deletions ...enhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/api/dto/clip2/Power.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
package org.openhab.binding.hue.internal.api.dto.clip2;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
import org.openhab.binding.hue.internal.api.dto.clip2.enums.BatteryStateType;
import org.openhab.core.library.types.DecimalType;
import org.openhab.core.library.types.OnOffType;
Expand All @@ -27,15 +28,18 @@
*/
@NonNullByDefault
public class Power {
private @NonNullByDefault({}) @SerializedName("battery_state") String batteryState;
private @Nullable @SerializedName("battery_state") String batteryState;
private @SerializedName("battery_level") int batteryLevel;

public BatteryStateType getBatteryState() {
try {
return BatteryStateType.valueOf(batteryState.toUpperCase());
} catch (IllegalArgumentException e) {
return BatteryStateType.CRITICAL;
String batteryState = this.batteryState;
if (batteryState != null) {
try {
return BatteryStateType.valueOf(batteryState.toUpperCase());
} catch (IllegalArgumentException e) {
}
}
return BatteryStateType.CRITICAL;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if reporting a critical level is the right thing for an unknown state. This might cause some negative sideeffects. Would it be possible to extend the enum with an Unknown state?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oups, sorry @lsiepel , you submitted you comment at the same time I approved.
But your question is good, I asked me the same.
As I understood the description, it should happen only during a short time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Waiting for @andrewfg answer ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I understood the description, it should happen only during a short time.

This is correct. Hue battery devices communicate their status to the Hue bridge by means of two processes..

  1. event driven: if someone presses a button on such a device, it immediately sends its new status to the bridge.
  2. polling: in case nobody presses a button, the bridge also does a slow polling of the battery state; the polling interval is not too fast in order to prevent waking the device too often

This bug would only occur if a new device is added to the Hue bridge and then immediately also added to OH before either case 1. or 2. will have occurred. Prior to this PR that case would cause an NPE, whereas after this PR the battery state is reported as critical until either case 1. or 2. actually occurs, at which time it is updated to the new actual value.

This is quite a seldom case that rights itself quite quickly, so IMHO temporarily reporting critical is actually perfectly Ok.

}

public int getBatteryLevel() {
Expand Down