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.

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

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

Merged
merged 1 commit into from Apr 6, 2024

Conversation

andrewfg
Copy link
Contributor

@andrewfg andrewfg commented Apr 6, 2024

Resolves #16617

Sometimes when new battery devices are added to the bridge, the device will not have had enough time on the network for its battery state to have been queried by the bridge. In such cases the bridge returns null in the Power.batteryState DTO field, and this causes an NPE.

This PR resolves that issue.

Signed-off-by: Andrew Fiddian-Green software@whitebear.ch

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@andrewfg andrewfg added the bug An unexpected problem or unintended behavior of an add-on label Apr 6, 2024
@andrewfg andrewfg requested a review from a team April 6, 2024 10:52
@andrewfg andrewfg self-assigned this Apr 6, 2024
@andrewfg andrewfg requested a review from cweitkamp as a code owner April 6, 2024 10:52
}
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.

Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@lolodomo lolodomo merged commit 203be93 into openhab:main Apr 6, 2024
3 checks passed
@lolodomo lolodomo added this to the 4.2 milestone Apr 6, 2024
lo92fr pushed a commit to lo92fr/openhab-addons that referenced this pull request Apr 30, 2024
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[hue] New dimmer switch model RWL022 causes NPE
3 participants