[DLinkSmartHome] add DSP-W215 Smart Plug #8648

pasbi · 2020-10-03T19:13:31Z

Note: the original PR (2.5.x) is #8361.

DESCRIPTION

What is the classification of the PR, e.g. Bugfix, Improvement, Novel Addition, ... ?
Add support for a thing.
Did you describe the PRs motivation and goal?
Read temperature, state and total/current power consumption of the thing, set the state of the thing.
Did you provide a link to any prior discussion, e.g. an issue or community forum thread?
No.
Did you describe new features for the end user?
Yes, in the README.md
Did you describe any noteworthy changes in usage for the end user?
Not necessary.
Was the documentation updated accordingly, e.g. the add-on README?
Yes.
Does your contribution follow the coding guidelines:
https://www.openhab.org/docs/developer/development/guidelines.html
It passed all the checks, and I'm not aware of any violation. Yet, I'm not sure. BTW, the link is broken.
Did you check for any (relevant) issues from the static code analysis:
https://www.openhab.org/docs/developer/development/bindings.html#static-code-analysis
Yes.
Did you sign-off your work:
Yes.

TESTING

add DSP-W215 Smart Plug (2.5.x PR: openhab#8361) Signed-off-by: Pascal Bies <pascal.bies@k-lens.de>

TravisBuddy · 2020-10-03T19:15:55Z

Travis tests have failed

Hey @pasbi,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

fwolter · 2020-10-04T15:25:23Z

bundles/org.openhab.binding.dlinksmarthome/pom.xml

-    <version>3.0.0-SNAPSHOT</version>
+    <version>2.5.9-SNAPSHOT</version>


Is this change intentional?

fwolter · 2020-10-04T15:25:59Z

...ding.dlinksmarthome/src/main/java/org/openhab/binding/dlinksmarthome/internal/DLinkHNAP.java

+ *
+ * @author Pascal Bies - Initial contribution
+ */
+public abstract class DLinkHNAP {


Please add the NonNullByDefault annotation.

fwolter · 2020-10-04T15:26:46Z

...ding.dlinksmarthome/src/main/java/org/openhab/binding/dlinksmarthome/internal/DLinkHNAP.java

+    public void start(ScheduledExecutorService scheduler) {
+        try {
+            pollFuture = scheduler.scheduleWithFixedDelay(this::poller, 0, DETECT_POLL_S, TimeUnit.SECONDS);
+        } catch (Exception e) {


What type of exception do you expect here? Can you specify the concrete type?

fwolter · 2020-10-04T15:27:09Z

...ding.dlinksmarthome/src/main/java/org/openhab/binding/dlinksmarthome/internal/DLinkHNAP.java

+        } catch (Exception e) {
+            updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.HANDLER_INITIALIZING_ERROR,
+                    "Unexpected internal error.");
+            logger.error("Failed to start http client or scheduler.");


Bindings should only log to error if something severe happened, like the detection of a bug in your code. This could be warn.

Additionally, you could remove the logging statement at all, as updateStatus() already does the logging.

fwolter · 2020-10-04T15:29:39Z

...ding.dlinksmarthome/src/main/java/org/openhab/binding/dlinksmarthome/internal/DLinkHNAP.java

+        } catch (CommunicationException e) {
+            logger.error("Failed to get quantity (communication error).");
+            updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR);
+        } catch (SOAPException e) {
+            logger.error("Unexpected internal error.");
+            updateStatus(ThingStatus.OFFLINE);
+        }


You could include the exceptions message as third parameter to updateStatus() to display the error in the UI.
You could remove the logging statement, as updateStatus() already does the logging.

fwolter · 2020-10-04T15:45:59Z

...src/main/java/org/openhab/binding/dlinksmarthome/internal/handler/DLinkSmartPlugHandler.java

+                    try {
+                        DLinkSmartPlugHandler.this.setState((OnOffType) command);
+                    } catch (ClassCastException e) {
+                        logger.error("Unexpected command type for channel '{}'.", DLinkSmartHomeBindingConstants.STATE);
+                    }


Unchecked exceptions should be cought only if necessary. Please check the type beforehand.

fwolter · 2020-10-04T15:46:28Z

...src/main/java/org/openhab/binding/dlinksmarthome/internal/handler/DLinkSmartPlugHandler.java

+    @Override
+    public void dispose() {
+        super.dispose();
+        hnap.stop();


The super destructor should be called at the end of the implementing method.

fwolter · 2020-10-04T15:47:27Z

...src/main/java/org/openhab/binding/dlinksmarthome/internal/handler/DLinkSmartPlugHandler.java

+    private ChannelUID getChannelUID(final String name) {
+        Channel channel = thing.getChannel(name);
+        if (channel == null) {
+            logger.error("Did not find channel '{}'.", name);


See above. this could be warn.

fwolter · 2020-10-04T15:48:11Z

...src/main/java/org/openhab/binding/dlinksmarthome/internal/handler/DLinkSmartPlugHandler.java

+                    logger.error("Unexpected channel: '{}'.", channelID);
+                    throw new IllegalArgumentException("Unexpected channel.");


Uncought unchecked exceptions are logged by the framework. You could remove the logging message.

fwolter · 2020-10-04T15:49:30Z

bundles/org.openhab.binding.dlinksmarthome/src/main/resources/OH-INF/thing/channels.xml

+	xmlns:thing="https://openhab.org/schemas/thing-description/v1.0.0"
+	xsi:schemaLocation="https://openhab.org/schemas/thing-description/v1.0.0 https://openhab.org/schemas/thing-description-1.0.0.xsd">
+
+	<channel-type id="current_consumption" advanced="true">


Is there are reason why you made all Channels advanced?

Hilbrand · 2020-10-06T06:52:22Z

Can you also address the comments in #8361 that are shown under 8 hidden conversations. Also note that in this pr are hidden conversations.

fwolter · 2021-01-13T22:33:58Z

@pasbi What is the status of this PR?

pasbi · 2021-01-16T09:19:44Z

Sorry -- I'm totally confused right now. Please be patient with me.

That confusion comes from two points:

The automatic conversion from 2.5.x to 3.x (maybe this is easy, but for me it's intransparent)
The fact that the original PR [DLinkSmartHome] add DSP-W215 Smart Plug #8361 was kind of rejected (reason: new code does not integrate well into the old one).

So my proposal was to close this PR and start over again with the approach proposed in #8361 :

refactor existing code
add the new device properly

I'm willing to do that, but I need some advice from the code owner @MikeJMajor to refactor.

please also see #8361 (comment)

Edit: I've now updated my IDE and looked through the existing code, Everything is ready to start.
Before I start to refactor, I'd like to chat with @MikeJMajor to agree on the changes.

fwolter · 2021-01-16T18:06:58Z

I'm not quite sure what you are confused about. You already managed to migrate the code from 2.5 to 3.x. Next step would be to incorporate the review feedback, I gave in this PR.

Maybe you could ask a concrete question, that @MikeJMajor or anybody else can answer?

openhab-bot · 2021-02-03T08:55:19Z

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/developing-a-binding-for-dlink-plugs/24122/69

pasbi · 2021-02-03T12:03:00Z

I'm planning to continue working on this PR, but I'm quite busy atm, unfortunately. If others want to contribute, go ahead 😄

wborn · 2022-03-07T20:06:45Z

Feel free to reopen this PR when you continue working on it.

[DLinkSmartHome] migrate code from 2.5.x

a03b035

add DSP-W215 Smart Plug (2.5.x PR: openhab#8361) Signed-off-by: Pascal Bies <pascal.bies@k-lens.de>

pasbi requested a review from MikeJMajor as a code owner October 3, 2020 19:13

pasbi changed the title ~~[DLinkSmartHome] migrate code from 2.5.x~~ [DLinkSmartHome] add DSP-W215 Smart Plug Oct 3, 2020

pasbi mentioned this pull request Oct 3, 2020

[DLinkSmartHome] add DSP-W215 Smart Plug #8361

Closed

fwolter requested changes Oct 4, 2020

View reviewed changes

cpmeister added the enhancement An enhancement or new feature for an existing add-on label Oct 4, 2020

fwolter added the awaiting feedback label Nov 1, 2020

Skinah added the work in progress A PR that is not yet ready to be merged label Jun 15, 2021

fwolter added the stale As soon as a PR is marked stale, it can be removed 6 months later. label Dec 11, 2021

wborn closed this Mar 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DLinkSmartHome] add DSP-W215 Smart Plug #8648

[DLinkSmartHome] add DSP-W215 Smart Plug #8648

pasbi commented Oct 3, 2020

TravisBuddy commented Oct 3, 2020

fwolter Oct 4, 2020

fwolter Oct 4, 2020

fwolter Oct 4, 2020

fwolter Oct 4, 2020

fwolter Oct 4, 2020

fwolter Oct 4, 2020

fwolter Oct 4, 2020

fwolter Oct 4, 2020

fwolter Oct 4, 2020

fwolter Oct 4, 2020

Hilbrand commented Oct 6, 2020

fwolter commented Jan 13, 2021

pasbi commented Jan 16, 2021 •

edited

fwolter commented Jan 16, 2021

openhab-bot commented Feb 3, 2021

pasbi commented Feb 3, 2021

wborn commented Mar 7, 2022

		<version>3.0.0-SNAPSHOT</version>
		<version>2.5.9-SNAPSHOT</version>

		logger.error("Unexpected channel: '{}'.", channelID);
		throw new IllegalArgumentException("Unexpected channel.");

[DLinkSmartHome] add DSP-W215 Smart Plug #8648

[DLinkSmartHome] add DSP-W215 Smart Plug #8648

Conversation

pasbi commented Oct 3, 2020

DESCRIPTION

TESTING

TravisBuddy commented Oct 3, 2020

Travis tests have failed

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

Hilbrand commented Oct 6, 2020

fwolter commented Jan 13, 2021

pasbi commented Jan 16, 2021 • edited

fwolter commented Jan 16, 2021

openhab-bot commented Feb 3, 2021

pasbi commented Feb 3, 2021

wborn commented Mar 7, 2022

pasbi commented Jan 16, 2021 •

edited