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

[miele] Improve multicast implementation #14199

Merged
merged 1 commit into from
Jan 14, 2023
Merged

[miele] Improve multicast implementation #14199

merged 1 commit into from
Jan 14, 2023

Conversation

jlaur
Copy link
Contributor

@jlaur jlaur commented Jan 9, 2023 •
edited
Loading

The main intention here was to fix some MulticastSocket deprecation warnings after upgrading to Java 17 - see description in #14198. However, while testing the refactoring needed for this, I came across some other issues that I have fixed as well:

  • The multicast listener thread did not allow to be properly interrupted and thus would keep running after stopping or uninstalling the binding. I noticed this when replacing the JAR a few times and ended up with multiple threads in different versions running simultaneously.
  • The conversion from byte buffer to String was not correct and the resulting string was filled up with trailing null-bytes. This could be seen in logs with log level trace, but there was a work-around in place for correctly handling the received data.
  • Minor additional refactoring.

Fixes #14198

JARs for testing:

@jlaur jlaur added the enhancement An enhancement or new feature for an existing add-on label Jan 9, 2023
@jlaur jlaur force-pushed the 14198-miele-multicast branch 4 times, most recently from fc4ddb1 to 2f8a471 Compare January 10, 2023 20:38
@jlaur
Improve multicast implementation
06b3b6f 
Fixes openhab#14198

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
@jlaur jlaur force-pushed the 14198-miele-multicast branch from 2f8a471 to 06b3b6f Compare January 10, 2023 21:48
@jlaur jlaur marked this pull request as ready for review January 10, 2023 22:01
@jlaur jlaur requested a review from kgoderis as a code owner January 10, 2023 22:01
@jlaur jlaur requested a review from a team January 10, 2023 22:01
lolodomo
lolodomo reviewed Jan 14, 2023
View reviewed changes
Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

Just one question ?

...nding.miele/src/main/java/org/openhab/binding/miele/internal/handler/MieleBridgeHandler.java Show resolved Hide resolved
lolodomo
lolodomo approved these changes Jan 14, 2023
View reviewed changes
Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@lolodomo lolodomo merged commit f4268c4 into openhab:main Jan 14, 2023
@lolodomo lolodomo added this to the 4.0 milestone Jan 14, 2023
@lolodomo
Copy link
Contributor

Is the search in Github no more working ? If I search with MulticastSocket, I get nothing found !

@jlaur
Copy link
Contributor Author

jlaur commented Jan 14, 2023

@lolodomo - thanks for the review. Regarding GitHub search, I'm using it less and less as I'm also having problems getting some trustworthy results.

@jlaur jlaur deleted the 14198-miele-multicast branch January 14, 2023 11:33
@lolodomo
Copy link
Contributor

No search is working, whatever the filled value !

@lolodomo
Copy link
Contributor

lolodomo commented Jan 14, 2023 •
edited
Loading

I run a grep locally. We have 16 other bindings using MulticastSocket. Should they be all checked/updated ?

@jlaur
Copy link
Contributor Author

jlaur commented Jan 14, 2023

@lolodomo - only if they use any of the deprecated methods, see https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/net/MulticastSocket.html

@mhilbush
Copy link
Contributor

Is the search in Github no more working ? If I search with MulticastSocket, I get nothing found !

Same for me.

https://community.openhab.org/t/searching-openhab-addons-repo-not-giving-any-code-results/143136

@mhilbush
Copy link
Contributor

No response in 6 days... I was beginning to think it was just me lol

@mhilbush mhilbush mentioned this pull request Jan 14, 2023
Closed
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Feb 28, 2023
@jlaur @nemerdaud
Improve multicast implementation (openhab#14199)
b53f792 
Fixes openhab#14198

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
This was referenced Mar 14, 2023
Merged
Merged
renescherer pushed a commit to renescherer/openhab-addons that referenced this pull request Mar 23, 2023
@jlaur @renescherer
Improve multicast implementation (openhab#14199)
046afa2 
Fixes openhab#14198

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
@jlaur
Copy link
Contributor Author

jlaur commented May 6, 2023

@openhab/add-ons-maintainers - I have been running this version since creating the PR without any issues. It stabilizes the multicast implementation and I wouldn't want to go back to the current 3.4 version in my production system. I would therefore ask you to consider this as a candidate for being cherry-picked into the 3.4.x branch.

kaikreuzer pushed a commit that referenced this pull request May 6, 2023
@jlaur @kaikreuzer
Improve multicast implementation (#14199)
4d1337f 
Fixes #14198

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
@kaikreuzer
Copy link
Member

@jlaur I cherry-picked it to 3.4.x.

@kaikreuzer kaikreuzer added the patch A PR that has been cherry-picked to a patch release branch label May 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lolodomo lolodomo lolodomo approved these changes

@kgoderis kgoderis Awaiting requested review from kgoderis kgoderis is a code owner

Assignees
No one assigned
Labels
enhancement An enhancement or new feature for an existing add-on patch A PR that has been cherry-picked to a patch release branch
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.

[miele] MulticastSocket deprecation warnings
4 participants
@jlaur @lolodomo @mhilbush @kaikreuzer