[ecovacs] Catch more exceptions when parsing vacuum data by maniac103 · Pull Request #20016 · openhab/openhab-addons

maniac103 · 2026-01-09T12:38:55Z

Missing fields in the responses so far went uncaught:

2026-01-08 14:19:31.852 [WARN ] [mmon.WrappedScheduledExecutorService] - Scheduled runnable ended with an exception: java.lang.NullPointerException: Cannot invoke "java.lang.Integer.intValue()" because "resp.waterAmount" is null
	at org.openhab.binding.ecovacs.internal.api.commands.GetMoppingWaterAmountCommand.convertResponse(GetMoppingWaterAmountCommand.java:45) ~[?:?]
	at org.openhab.binding.ecovacs.internal.api.commands.GetMoppingWaterAmountCommand.convertResponse(GetMoppingWaterAmountCommand.java:1) ~[?:?]
	at org.openhab.binding.ecovacs.internal.api.impl.EcovacsApiImpl.sendIotCommand(EcovacsApiImpl.java:308) ~[?:?]
	at org.openhab.binding.ecovacs.internal.api.impl.EcovacsIotMqDevice.sendCommand(EcovacsIotMqDevice.java:98) ~[?:?]
	at org.openhab.binding.ecovacs.internal.handler.EcovacsVacuumHandler.lambda$15(EcovacsVacuumHandler.java:632) ~[?:?]
	at org.openhab.binding.ecovacs.internal.handler.EcovacsVacuumHandler.doWithDevice(EcovacsVacuumHandler.java:822) ~[?:?]
	at org.openhab.binding.ecovacs.internal.handler.EcovacsVacuumHandler.pollData(EcovacsVacuumHandler.java:591) ~[?:?]
	at org.openhab.binding.ecovacs.internal.api.util.SchedulerTask.run(SchedulerTask.java:95) ~[?:?]
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:572) ~[?:?]
	at java.util.concurrent.FutureTask.run(FutureTask.java:317) ~[?:?]
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:304) ~[?:?]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144) ~[?:?]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642) ~[?:?]
	at java.lang.Thread.run(Thread.java:1583) [?:?]

Make sure to catch those and make the user aware of it by setting the thing to offline / communication error instead of just emitting a warning in the log, so they get a chance to report that problem. With the previous behavior everything apparently was OK, but channels were not populated correctly.

(The root cause for the particular stack trace above was a wrong feature flag, which will be fixed in a separate MR)

Missing fields in the responses so far went uncaught: 2026-01-08 14:19:31.852 [WARN ] [mmon.WrappedScheduledExecutorService] - Scheduled runnable ended with an exception: java.lang.NullPointerException: Cannot invoke "java.lang.Integer.intValue()" because "resp.waterAmount" is null at org.openhab.binding.ecovacs.internal.api.commands.GetMoppingWaterAmountCommand.convertResponse(GetMoppingWaterAmountCommand.java:45) ~[?:?] at org.openhab.binding.ecovacs.internal.api.commands.GetMoppingWaterAmountCommand.convertResponse(GetMoppingWaterAmountCommand.java:1) ~[?:?] at org.openhab.binding.ecovacs.internal.api.impl.EcovacsApiImpl.sendIotCommand(EcovacsApiImpl.java:308) ~[?:?] at org.openhab.binding.ecovacs.internal.api.impl.EcovacsIotMqDevice.sendCommand(EcovacsIotMqDevice.java:98) ~[?:?] at org.openhab.binding.ecovacs.internal.handler.EcovacsVacuumHandler.lambda$15(EcovacsVacuumHandler.java:632) ~[?:?] at org.openhab.binding.ecovacs.internal.handler.EcovacsVacuumHandler.doWithDevice(EcovacsVacuumHandler.java:822) ~[?:?] at org.openhab.binding.ecovacs.internal.handler.EcovacsVacuumHandler.pollData(EcovacsVacuumHandler.java:591) ~[?:?] at org.openhab.binding.ecovacs.internal.api.util.SchedulerTask.run(SchedulerTask.java:95) ~[?:?] at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:572) ~[?:?] at java.util.concurrent.FutureTask.run(FutureTask.java:317) ~[?:?] at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:304) ~[?:?] at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144) ~[?:?] at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642) ~[?:?] at java.lang.Thread.run(Thread.java:1583) [?:?] Make sure to catch those and make the user aware of it by setting the thing to offline / communication error instead of just emitting a warning in the log, so they get a chance to report that problem. With the previous behavior everything apparently was OK, but channels were not populated correctly. Signed-off-by: Danny Baumann <dannybaumann@web.de>

lsiepel · 2026-01-15T08:33:47Z

This looks like it can be fixed by a null check on resp.waterAmount rather than catching a generic Exception. We generally avoid catching Exception; an NPE should be prevented, not caught, since it indicates a bug and exception handling is expensive.

Edit: I would also not try to catch the RuntimeException as suggested by copilot, but rather catch the known exceptions and prevent NPE or IOB exceptions that can be prevented easily

Copilot

Pull request overview

This PR improves error handling in the Ecovacs vacuum binding by catching more exception types when parsing vacuum data. The change ensures that unexpected runtime exceptions (like NullPointerExceptions from missing fields in API responses) are properly caught and reported to users by setting the thing offline, rather than just logging warnings while leaving the thing in an apparently OK state with unpopulated channels.

Changes:

Broadened exception catching from specific exceptions to general Exception in three device communication classes
Removed now-unused DataParsingException imports from two files

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
EcovacsXmppDevice.java	Changed catch clause to catch all exceptions instead of just specific parsing exceptions
EcovacsIotMqDevice.java	Changed catch clause to catch all exceptions and removed unused DataParsingException import
EcovacsApiImpl.java	Changed catch clause to catch all exceptions and removed unused DataParsingException import

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-15T08:41:27Z

                }
            }
-        } catch (DataParsingException | ParserConfigurationException | TransformerException e) {
+        } catch (Exception e) {


Catching generic Exception is overly broad and may mask unexpected errors that shouldn't be recoverable (like OutOfMemoryError or other serious JVM errors). Consider catching RuntimeException instead, or at minimum catching specific exceptions like NullPointerException, IllegalStateException, and keeping the existing specific exceptions. This allows truly exceptional errors to propagate properly while still catching the parsing and data-related runtime exceptions mentioned in the PR description.

Suggested change

} catch (Exception e) {

} catch (RuntimeException e) {

Copilot · 2026-01-15T08:41:27Z

                    logger.trace("{}: Got MQTT message on topic {}: {}", getSerialNumber(), receivedTopic, payload);
                    parser.handleMessage(eventName, payload);
-                } catch (DataParsingException e) {
+                } catch (Exception e) {


Catching generic Exception is overly broad and may mask unexpected errors that shouldn't be recoverable (like OutOfMemoryError or other serious JVM errors). Consider catching RuntimeException instead, or at minimum catching specific exceptions like NullPointerException, IllegalStateException, and keeping the existing specific exceptions. This allows truly exceptional errors to propagate properly while still catching the parsing and data-related runtime exceptions mentioned in the PR description.

Suggested change

} catch (Exception e) {

} catch (RuntimeException e) {

Copilot · 2026-01-15T08:41:27Z

        try {
            return command.convertResponse(commandResponse, desc.protoVersion, gson);
-        } catch (DataParsingException e) {
+        } catch (Exception e) {


Catching generic Exception is overly broad and may mask unexpected errors that shouldn't be recoverable (like OutOfMemoryError or other serious JVM errors). Consider catching RuntimeException instead, or at minimum catching specific exceptions like NullPointerException, IllegalStateException, and keeping the existing specific exceptions. This allows truly exceptional errors to propagate properly while still catching the parsing and data-related runtime exceptions mentioned in the PR description.

Suggested change

} catch (Exception e) {

} catch (RuntimeException e) {

lsiepel · 2026-04-28T08:32:15Z

Gentle ping @maniac103, you seem busy at the moment with android changes, maybe you can have a look at the comments afterwards.

maniac103 · 2026-04-28T09:13:07Z

Gentle ping @maniac103, you seem busy at the moment with android changes, maybe you can have a look at the comments afterwards.

Thanks for the ping, I completely forgot about this.
I'm not sure I agree about the Copilot comments. I wrote the code on the following premises:

An exception being thrown in a scheduled runnable ends the execution of said runnable
There will be a log entry for that exception being caught, but nothing more
The polling runnable we're dealing with here needs dynamic scheduling, so it reschedules itself at the end of its run
If it's ended earlier it won't reschedule itself

Those points together mean that an uncaught exception in that runnable is 'catastrophic' in that device data update will break silently without recovering. It's the intention of this PR to change this, and to do that, I need to avoid uncaught exceptions as much as possible - why is why I'm catching all of them, including unexpected ones. They'll still be logged (via EcovacsVacuumHandler::doWithDevice() and cause the thing to transition to OFFLINE temporarily to increase awareness.

Unlike stated in the Copilot comments, OutOfMemoryError or other JVM errors do not derive from Exception, but from Error (Exception and Error just share the common base class Throwable).

prevent NPE or IOB exceptions that can be prevented easily

As mentioned, the root cause for this was already fixed separately (#20017).

lsiepel

Thanks for the quick reply. LGTM

lsiepel requested a review from Copilot January 15, 2026 08:31

Copilot started reviewing on behalf of lsiepel January 15, 2026 08:31 View session

Copilot AI reviewed Jan 15, 2026

View reviewed changes

lsiepel approved these changes Apr 28, 2026

View reviewed changes

lsiepel merged commit 7d1fed4 into openhab:main Apr 28, 2026
7 of 8 checks passed

lsiepel added the enhancement An enhancement or new feature for an existing add-on label Apr 28, 2026

lsiepel added this to the 5.2 milestone Apr 28, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ecovacs] Catch more exceptions when parsing vacuum data#20016

[ecovacs] Catch more exceptions when parsing vacuum data#20016
lsiepel merged 1 commit intoopenhab:mainfrom
maniac103:ecovacs-parsing-error

maniac103 commented Jan 9, 2026 •

edited

Loading

Uh oh!

lsiepel commented Jan 15, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 15, 2026

Uh oh!

Copilot AI Jan 15, 2026

Uh oh!

Copilot AI Jan 15, 2026

Uh oh!

lsiepel commented Apr 28, 2026

Uh oh!

maniac103 commented Apr 28, 2026

Uh oh!

lsiepel left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

maniac103 commented Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lsiepel commented Jan 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

lsiepel commented Apr 28, 2026

Uh oh!

maniac103 commented Apr 28, 2026

Uh oh!

lsiepel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

maniac103 commented Jan 9, 2026 •

edited

Loading

lsiepel commented Jan 15, 2026 •

edited

Loading