[shelly] Improve error handling for Gen1 (CoAP/CoIoT) #19229

jlaur · 2025-08-23T23:17:35Z

The goal of this PR is to prevent NullPointerException rather than catching it. A few IllegalArgumentException were prevented as well.

As principle for this PR, I have not changed behavior.

markus7017 · 2025-08-24T07:36:42Z

...ding.shelly/src/main/java/org/openhab/binding/shelly/internal/api1/Shelly1CoIoTProtocol.java

            double power = -1.0;
            for (CoIotSensor update : allUpdates) {
-                CoIotDescrSen d = fixDescription(sensorMap.get(update.id), blkMap);
+                CoIotDescrSen d = sensorMap.getOrDefault(update.id, new CoIotDescrSen());


why did you removed the call to fixDescription(), this is important
I'm not fine with allocation of an empty CoIotDescrSen() - if the process fails, this is an error

This is the method previously called:

openhab-addons/bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/api1/Shelly1CoIoTProtocol.java

Lines 373 to 375 in c417fe3

public CoIotDescrSen fixDescription(@Nullable CoIotDescrSen sen, Map<String, CoIotDescrBlk> blkMap) {

return sen != null ? sen : new CoIotDescrSen();

}

So what you are suggesting is to change that behavior. As principle for this PR I did not want to change behavior, because I don't know the logic well enough and I don't have Gen 1 devices.

...inding.shelly/src/main/java/org/openhab/binding/shelly/internal/api1/Shelly1CoapHandler.java

markus7017 · 2025-08-24T07:39:40Z

...inding.shelly/src/main/java/org/openhab/binding/shelly/internal/api1/Shelly1CoapHandler.java

                updatesRequested = true;
            }
-        } catch (JsonSyntaxException | IllegalArgumentException | NullPointerException e) {
+        } catch (Exception e) {


We are discussing to use IllegalArgumentException or RuntimeException all the time and now you catch all Exception generically. Also I thing explicitly point our that it could result in a JsonSyntaxException has a better quality. I would prefer JsonSyntaxException | RuntimeException

I noticed several calls to org.eclipse.californium.core.coap, i.e. a third party library outside of our control. If you are sure none of those exceptions are thrown by that library, we can remove the catch entirely. I checked and ensured that the binding doesn't throw any of those anymore.

If you parsed a malformed JSON to gson.fromJson() this will cause JsonSyntaxException.
What do you think about JsonSyntaxException | RuntimeException?

Where do you see gson.fromJson() within this try/catch block?

markus7017 · 2025-08-24T07:41:27Z

...inding.shelly/src/main/java/org/openhab/binding/shelly/internal/api1/Shelly1CoapHandler.java

+        if (!valid) {
+            logger.debug("{}: WARNING: Incompatible device description detected for CoIoT version {}!", thingName,
+                    coiot.getVersion());
+            return;


hmm, that does not provide the call stack, which makes it easy to fix things

Same as before. I'm not sure what you are suggesting? This message is only logged at this specific place, so at least you know where it's coming from.

addSensor logs before returning false:

openhab-addons/bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/api1/Shelly1CoapHandler.java

Lines 391 to 392 in c417fe3

logger.debug("{}: Invalid format for sensor definition detected, id={}", thingName, sen.id);

return false;

markus7017 · 2025-08-24T07:41:46Z

...inding.shelly/src/main/java/org/openhab/binding/shelly/internal/api1/Shelly1CoapHandler.java

-                                                                      // description is buggy
-            logger.debug("{}: Unable to decode sensor definition -> skip", thingName, e);
-        }
+        CoIotDescrSen fixed = coiot.fixDescription(sen, blkMap);


Same as what?

markus7017 · 2025-08-24T07:43:02Z

...inding.shelly/src/main/java/org/openhab/binding/shelly/internal/api1/Shelly1CoapHandler.java

-                logger.trace("{}:  Sensor value[{}]: id={}, Value={} ({}, Type={}, Range={}, Link={}: {})", thingName,
-                        i, s.id, getString(s.valueStr).isEmpty() ? s.value : s.valueStr, sen.desc, sen.type, sen.range,
-                        sen.links, element.desc);
+        for (CoIotSensor s : sensorUpdates) {


Did you tested the logic changes with multiple Gen1 devices?

Unfortunately I don't have any Gen 1 devices, so I have to rely on testing when integrated in snapshot builds and the DEV build, is that okay? There should be no logic changes here though, I did this refactoring very carefully step by step. If you see any, please let me know.

markus7017 · 2025-08-26T05:03:51Z

@jlaur I'll create a version with your changes and do some testing with different Gen1 devices

jlaur · 2025-08-26T15:46:22Z

I'll create a version with your changes and do some testing with different Gen1 devices

Thank you,. 👍 In the meantime, can we discuss the open comments?

jlaur · 2025-09-07T20:11:10Z

@markus7017 - did you see my comments?

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>

jlaur · 2025-09-18T17:49:22Z

@markus7017?

jlaur requested a review from markus7017 as a code owner August 23, 2025 23:17

markus7017 requested changes Aug 24, 2025

View reviewed changes

markus7017 changed the title ~~[shelly] Improve error handling~~ [shelly] Improve error handling for Gen1 (COAP/CoIOT) Aug 24, 2025

jlaur changed the title ~~[shelly] Improve error handling for Gen1 (COAP/CoIOT)~~ [shelly] Improve error handling for Gen1 (CoAP/CoIoT) Aug 24, 2025

jlaur force-pushed the shelly-error-handling branch from 386515c to 9da2c4d Compare August 26, 2025 19:25

jlaur added 3 commits September 9, 2025 22:24

Improve error handling

d2350a4

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>

Remove useless method

5db97c2

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>

Do not reset serial/payload on bad version

ea163e0

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>

jlaur force-pushed the shelly-error-handling branch from 9da2c4d to ea163e0 Compare September 9, 2025 20:25

lsiepel requested a review from markus7017 September 16, 2025 19:19

jlaur mentioned this pull request Sep 21, 2025

Top 10 of bindings having the most of open issues #13706

Open

	public CoIotDescrSen fixDescription(@Nullable CoIotDescrSen sen, Map<String, CoIotDescrBlk> blkMap) {
	return sen != null ? sen : new CoIotDescrSen();
	}

	logger.debug("{}: Invalid format for sensor definition detected, id={}", thingName, sen.id);
	return false;

Uh oh!

[shelly] Improve error handling for Gen1 (CoAP/CoIoT) #19229

Are you sure you want to change the base?

[shelly] Improve error handling for Gen1 (CoAP/CoIoT) #19229

Uh oh!

Conversation

jlaur commented Aug 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlaur Aug 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markus7017 commented Aug 26, 2025

Uh oh!

jlaur commented Aug 26, 2025

Uh oh!

jlaur commented Sep 7, 2025

Uh oh!

jlaur commented Sep 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jlaur commented Aug 23, 2025 •

edited

Loading

jlaur Aug 24, 2025 •

edited

Loading