[irobot] Fix password discovery and command sending for Roomba I-Models. (using gson) #10860

rimago · 2021-06-13T11:16:18Z

Hello everybody,

this PR is the successor of #10592 and brings the fix of @falkena. I have added a commit to use gson instead of json-path to make it compatible to the openhab requirements.

The original description of the old PR:

this PR fixes password discovery for iRobot Roomba I-Series. Additionally, the regression introduced in
openhab/openhab-core#2208 is fixed. User has reported, that password
fetching works for iRobot Braava m-series mopping robots (https://community.openhab.org/t/irobot-9xx-on-openhab/17774/245). Sadly there is breaking change in robot configuration. I need it, since different robots report different features. I plan to raise more refactoring and feature extending PR's . Additionally this PR fixes #10116. Credit for the fix goes to @J-N-K (@J-N-K I hope, you are fine with. If not, please, ping me i'll revert the change)

Signed-off-by: Florian Binder fb@java4.info

Signed-off-by: Alexander Falkenstern <alexander.falkenstern@gmail.com>

Signed-off-by: Florian Binder <fb@java4.info>

fwolter

Great that you did a follow up! Doesn't look too bad with GSON...

There are some checkstyle warnings left. You could take a look at target/code-analysis/report.html.

fwolter · 2021-06-15T22:14:11Z

....binding.irobot/src/main/java/org/openhab/binding/irobot/internal/handler/RoombaHandler.java

+        IRobotConfiguration config = getConfigAs(IRobotConfiguration.class);
+
+        try {
+            InetAddress.getByName(config.getAddress());


initialize() must return fast. This can block if the network fails. You could use scheduler to execute the lookup in a task.

Or can this be removed at all?

fwolter · 2021-06-15T22:19:11Z

....binding.irobot/src/main/java/org/openhab/binding/irobot/internal/handler/RoombaHandler.java

-            logger.warn("Failed to parse JSON message from {}: {}", config.ipaddress, e.toString());
-            logger.warn("Raw contents: {}", payload);
+        } catch (JsonParseException exception) {
+            IRobotConfiguration config = getConfigAs(IRobotConfiguration.class);


Why is the configuration read again here? Actually it should be sufficient to read it once in initialize().

removed usage of config. Using Thing UID instead

fwolter · 2021-06-15T22:20:02Z

...b.binding.irobot/src/main/java/org/openhab/binding/irobot/internal/utils/LoginRequester.java

+        } catch (IOException exception) {
+        } finally {


You could add a comment, why this exception is ignored.

removed catch-block, as the exception is handled outside the function anyway

fwolter · 2021-06-15T22:21:05Z

...b.binding.irobot/src/main/java/org/openhab/binding/irobot/internal/utils/LoginRequester.java

+            socket.close();
+        }
+
+        Gson gson = new Gson();


You could make this a static final field to save resources.

fwolter · 2021-06-15T22:26:11Z

bundles/org.openhab.binding.irobot/src/main/resources/OH-INF/config/thing.xml

+		<parameter name="family" type="text">
+			<label>IRobot Family</label>
+			<description>IRobot device family</description>


If I see correctly this is newly introduced. If you need to differentiate between models, you could define a thing type for each model.

removed the parameter family, as it is not yet used. We can add it again as soon as it is needed.

fwolter · 2021-06-15T22:27:42Z

bundles/org.openhab.binding.irobot/src/main/resources/OH-INF/config/thing.xml

+	xsi:schemaLocation="https://openhab.org/schemas/config-description/v1.0.0 https://openhab.org/schemas/config-description-1.0.0.xsd">
+
+	<config-description uri="thing-type:irobot:thing">
+		<parameter name="address" type="text">


Is this renamed from ipaddress to address intentionally? Can this be kept as ipaddress to maintain backward compatibility? Or is there more in this PR which is not backward compatible?

As far as I know, it was part of a bigger refactoring. For now I changed it back to ipaddress to preserve backward compatibility.
(We can rename it to "address" together with the other refactoring and break the compatibility only once.)

That's a good approach!

rimago · 2021-06-16T07:59:30Z

Hi @fwolter ,
thank you very much for your feedback. I have removed all codestyle warnings and addressed your comments above.

remove unused parameters Signed-off-by: Florian Binder <fb@java4.info>

fwolter

LGTM

Thanks for taking this over!

…ls. (using gson) (openhab#10860) * Fix password discovery for Roomba I-Models. Signed-off-by: Alexander Falkenstern <alexander.falkenstern@gmail.com> * [irobot] remove json-path dependency (use gson instead) Signed-off-by: Florian Binder <fb@java4.info> * [irobot] fix checkstyle warnings, preserve backward compatibility, and remove unused parameters Signed-off-by: Florian Binder <fb@java4.info> Co-authored-by: Alexander Falkenstern <alexander.falkenstern@gmail.com>

…ls. (using gson) (openhab#10860) * Fix password discovery for Roomba I-Models. Signed-off-by: Alexander Falkenstern <alexander.falkenstern@gmail.com> * [irobot] remove json-path dependency (use gson instead) Signed-off-by: Florian Binder <fb@java4.info> * [irobot] fix checkstyle warnings, preserve backward compatibility, and remove unused parameters Signed-off-by: Florian Binder <fb@java4.info> Co-authored-by: Alexander Falkenstern <alexander.falkenstern@gmail.com> Signed-off-by: Luca Calcaterra <calcaterra.luca@gmail.com>

…ls. (using gson) (openhab#10860) * Fix password discovery for Roomba I-Models. Signed-off-by: Alexander Falkenstern <alexander.falkenstern@gmail.com> * [irobot] remove json-path dependency (use gson instead) Signed-off-by: Florian Binder <fb@java4.info> * [irobot] fix checkstyle warnings, preserve backward compatibility, and remove unused parameters Signed-off-by: Florian Binder <fb@java4.info> Co-authored-by: Alexander Falkenstern <alexander.falkenstern@gmail.com>

falkena and others added 2 commits June 13, 2021 12:52

Fix password discovery for Roomba I-Models.

1d30620

Signed-off-by: Alexander Falkenstern <alexander.falkenstern@gmail.com>

[irobot] remove json-path dependency (use gson instead)

a99904a

Signed-off-by: Florian Binder <fb@java4.info>

fwolter requested changes Jun 15, 2021

View reviewed changes

[irobot] fix checkstyle warnings, preserve backward compatibility, and

dcd37de

remove unused parameters Signed-off-by: Florian Binder <fb@java4.info>

rimago requested a review from fwolter June 17, 2021 09:52

fwolter approved these changes Jun 18, 2021

View reviewed changes

fwolter merged commit 1630430 into openhab:main Jun 18, 2021

fwolter added the bug An unexpected problem or unintended behavior of an add-on label Jun 18, 2021

fwolter added this to the 3.1 milestone Jun 18, 2021

rimago deleted the falkenas_fix branch June 18, 2021 09:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[irobot] Fix password discovery and command sending for Roomba I-Models. (using gson) #10860

[irobot] Fix password discovery and command sending for Roomba I-Models. (using gson) #10860

rimago commented Jun 13, 2021 •

edited

fwolter left a comment

fwolter Jun 15, 2021

rimago Jun 16, 2021

fwolter Jun 15, 2021

rimago Jun 16, 2021

fwolter Jun 15, 2021

rimago Jun 16, 2021

fwolter Jun 15, 2021

rimago Jun 16, 2021

fwolter Jun 15, 2021

rimago Jun 16, 2021

fwolter Jun 15, 2021

rimago Jun 16, 2021

fwolter Jun 16, 2021

rimago commented Jun 16, 2021

fwolter left a comment

[irobot] Fix password discovery and command sending for Roomba I-Models. (using gson) #10860

[irobot] Fix password discovery and command sending for Roomba I-Models. (using gson) #10860

Conversation

rimago commented Jun 13, 2021 • edited

fwolter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rimago commented Jun 16, 2021

fwolter left a comment

Choose a reason for hiding this comment

rimago commented Jun 13, 2021 •

edited