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.

Sign up for GitHub

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

Jump to bottom

[intesis] - added IntesisBox support #8694

Merged
merged 8 commits into from
Oct 24, 2020
Merged

[intesis] - added IntesisBox support #8694

merged 8 commits into from
Oct 24, 2020

Conversation

hmerk
Copy link
Contributor

@hmerk hmerk commented Oct 8, 2020

Signed-off-by: Hans-Jörg Merk [email protected]
Also Signed-off-by: Rocky Amatulli [email protected]
Also Signed-off-by: Cody Cutrer [email protected]

@TravisBuddy
Copy link

TravisBuddy commented Oct 8, 2020 •
edited
Loading

Travis tests were successful

Hey @hmerk,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@hmerk
applied spotless
8ea1d26 
Signed-off-by: Hans-Jörg Merk <[email protected]>
@hmerk hmerk requested review from cpmeister and Hilbrand October 8, 2020 11:23
@hmerk hmerk added the enhancement An enhancement or new feature for an existing add-on label Oct 12, 2020
fwolter
fwolter requested changes Oct 13, 2020
View reviewed changes
...binding.intesis/src/main/java/org/openhab/binding/intesis/internal/IntesisConfiguration.java Outdated Show resolved Hide resolved
...nding.intesis/src/main/java/org/openhab/binding/intesis/internal/api/IntesisBoxIdentity.java Outdated Show resolved Hide resolved
...ding.intesis/src/main/java/org/openhab/binding/intesis/internal/api/IntesisBoxSocketApi.java Outdated Show resolved Hide resolved
...ding.intesis/src/main/java/org/openhab/binding/intesis/internal/api/IntesisBoxSocketApi.java Outdated Show resolved Hide resolved
...ding.intesis/src/main/java/org/openhab/binding/intesis/internal/api/IntesisBoxSocketApi.java Outdated Show resolved Hide resolved
...ng.intesis/src/main/java/org/openhab/binding/intesis/internal/handler/IntesisBoxHandler.java Outdated Show resolved Hide resolved
...ng.intesis/src/main/java/org/openhab/binding/intesis/internal/handler/IntesisBoxHandler.java Show resolved Hide resolved
...ng.intesis/src/main/java/org/openhab/binding/intesis/internal/handler/IntesisBoxHandler.java Outdated Show resolved Hide resolved
...ng.intesis/src/main/java/org/openhab/binding/intesis/internal/handler/IntesisBoxHandler.java Outdated Show resolved Hide resolved
bundles/org.openhab.binding.intesis/README.md Outdated Show resolved Hide resolved
@cpmeister cpmeister changed the title [Intesis Binding] - added IntesisBox support [intesis] - added IntesisBox support Oct 15, 2020
@hmerk
Changes after first review
b255eec 
Signed-off-by: Hans-Jörg Merk <[email protected]>
@hmerk hmerk requested a review from fwolter October 19, 2020 14:50
@hmerk
Copy link
Contributor Author

hmerk commented Oct 19, 2020

Thanks @fwolter
I have addressed all your comments so far.

cpmeister
cpmeister requested changes Oct 19, 2020
View reviewed changes
...nding.intesis/src/main/java/org/openhab/binding/intesis/internal/api/IntesisBoxIdentity.java Outdated Show resolved Hide resolved
...inding.intesis/src/main/java/org/openhab/binding/intesis/internal/api/IntesisBoxMessage.java Outdated Show resolved Hide resolved
...ding.intesis/src/main/java/org/openhab/binding/intesis/internal/api/IntesisBoxSocketApi.java Outdated Show resolved Hide resolved
...ding.intesis/src/main/java/org/openhab/binding/intesis/internal/api/IntesisBoxSocketApi.java Outdated Show resolved Hide resolved
...ding.intesis/src/main/java/org/openhab/binding/intesis/internal/api/IntesisBoxSocketApi.java Outdated Show resolved Hide resolved
...ding.intesis/src/main/java/org/openhab/binding/intesis/internal/api/IntesisBoxSocketApi.java
this.port = port;
}

private class IntesisSocket {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this class needed? It doesn't add any functionality so just keep track of the Socket instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's how I saw it in the energenie Binding, which socket class was written by @Hilbrand . I have no clue of socket handling, so it was simply copy and paste.

...ng.intesis/src/main/java/org/openhab/binding/intesis/internal/handler/IntesisBoxHandler.java Outdated Show resolved Hide resolved
...ng.intesis/src/main/java/org/openhab/binding/intesis/internal/handler/IntesisBoxHandler.java Show resolved Hide resolved
...ng.intesis/src/main/java/org/openhab/binding/intesis/internal/handler/IntesisBoxHandler.java Outdated Show resolved Hide resolved
@cpmeister
Copy link
Contributor

Also please address the build warnings:

[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.intesis/src/main/java/org/openhab/binding/intesis/internal/api/IntesisBoxSocketApi.java:[91,17] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.intesis/src/main/java/org/openhab/binding/intesis/internal/api/IntesisBoxSocketApi.java:[95,17] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.intesis/src/main/java/org/openhab/binding/intesis/internal/api/IntesisBoxSocketApi.java:[99,17] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.intesis/src/main/java/org/openhab/binding/intesis/internal/api/IntesisBoxSocketApi.java:[134,17] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.intesis/src/main/java/org/openhab/binding/intesis/internal/api/IntesisBoxSocketApi.java:[137,17] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.intesis/src/main/java/org/openhab/binding/intesis/internal/api/IntesisBoxSocketApi.java:[152,27] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.intesis/src/main/java/org/openhab/binding/intesis/internal/handler/IntesisBoxHandler.java:[96,13] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.intesis/src/main/java/org/openhab/binding/intesis/internal/handler/IntesisBoxHandler.java:[102,25] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.intesis/src/main/java/org/openhab/binding/intesis/internal/handler/IntesisBoxHandler.java:[105,25] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.intesis/src/main/java/org/openhab/binding/intesis/internal/handler/IntesisBoxHandler.java:[108,25] Potential null pointer access: this expression has a '@Nullable' type
[WARNING] /home/travis/build/openhab/openhab-addons/bundles/org.openhab.binding.intesis/src/main/java/org/openhab/binding/intesis/internal/handler/IntesisBoxHandler.java:[111,25] Potential null pointer access: this expression has a '@Nullable' type

@hmerk
Changes after second review
bf96a6f 
Signed-off-by: Hans-Jörg Merk <[email protected]>
@hmerk
Copy link
Contributor Author

hmerk commented Oct 21, 2020

Thanks @cpmeister for your review, I have addressed most of it, some comments left.
According to the compiler warnings, i have already added null checks, but the warnings still exist. Don't have a clue how to fix this.

@hmerk hmerk requested a review from cpmeister October 21, 2020 07:53
@hmerk
Copy link
Contributor Author

hmerk commented Oct 21, 2020

Failing build is not related to intesis binding but Feed tests...

@hmerk hmerk added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Oct 21, 2020
@cpmeister
Copy link
Contributor

According to the compiler warnings, i have already added null checks, but the warnings still exist. Don't have a clue how to fix this.

The way to fix them is to assign the @Nullable fields to a local variable and then do all your processing on that local variable instead. The null checker is operating under the assumption that a @Nullable field could be changed to null at any time by another thread.

@hmerk
Fix compiler warnings
5ede918 
Signed-off-by: Hans-Jörg Merk <[email protected]>
@hmerk
Copy link
Contributor Author

hmerk commented Oct 22, 2020

Thanks @cpmeister I have fixed the compiler warnings and the double null check.

@hmerk
Changes after review
04cac76 
Signed-off-by: Hans-Jörg Merk <[email protected]>
@hmerk
Copy link
Contributor Author

hmerk commented Oct 23, 2020

Thanks @cpmeister I have addressed your comments, the rssi channel will be added later today.

@hmerk hmerk requested a review from cpmeister October 23, 2020 07:09
@hmerk
Copy link
Contributor Author

hmerk commented Oct 23, 2020

@cpmeister RSSI is now changed from property to be a system-channel.

@hmerk
Changes after next review
bab56ce 
Signed-off-by: Hans-Jörg Merk <[email protected]>
@hmerk
Copy link
Contributor Author

hmerk commented Oct 24, 2020

Thanks @fwolter all comments addressed.

@hmerk hmerk requested a review from fwolter October 24, 2020 07:57
fwolter
fwolter approved these changes Oct 24, 2020
View reviewed changes
Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

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

LGTM

cpmeister
cpmeister approved these changes Oct 24, 2020
View reviewed changes
Copy link
Contributor

@cpmeister cpmeister left a comment

Choose a reason for hiding this comment

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

LGTM

@cpmeister cpmeister merged commit 91fbe74 into openhab:main Oct 24, 2020
@cpmeister cpmeister added this to the 3.0.0.M2 milestone Oct 24, 2020
@hmerk
Copy link
Contributor Author

hmerk commented Oct 24, 2020

Thanks @fwolter and @cpmeister for your great support !!!

@hmerk hmerk deleted the intesisBox branch October 25, 2020 11:45
boehan pushed a commit to boehan/openhab-addons that referenced this pull request Apr 12, 2021
@hmerk @boehan
[intesis] - added IntesisBox support (openhab#8694)
d05cf72 
* Intesis Binding - added IntesisBox support

Signed-off-by: Hans-Jörg Merk <[email protected]>
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
@hmerk
[intesis] - added IntesisBox support (openhab#8694)
050f7b3 
* Intesis Binding - added IntesisBox support

Signed-off-by: Hans-Jörg Merk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fwolter fwolter fwolter approved these changes

@cpmeister cpmeister cpmeister approved these changes

@Hilbrand Hilbrand Awaiting requested review from Hilbrand

Assignees
No one assigned
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Milestone
3.0.0.M2
Development

Successfully merging this pull request may close these issues.
4 participants
@hmerk @TravisBuddy @cpmeister @fwolter