[shelly] BLU Motion, optimize ShellyManager for BLU devices #15401
Conversation
markus7017
added
bug
bundles/org.openhab.binding.shelly/src/main/resources/OH-INF/i18n/shelly.properties
Outdated
Show resolved
Hide resolved
|
@lsiepel changes applied |
|
@lsiepel anything missing? |
I don't have shelly devices and this binding is not known area to me, so i guess it's better for others to review. |
|
@openhab/add-ons-maintainers Could somebody please pick-up this PR |
|
@markus7017 : why you don't create different small PRs for each fixed issue or new enhancement ? I am sure you will get quicker feedback. Recommendation for new PRs. |
|
@lolodomo 90% (about 270 lines) are dependent on BLU motion, required changes to general initialization and decided to do Creating a separate PR for every logic change or bug fix creates a lot of overhead and makes it hard to manage PRs in parallel. Due to longer response times I have already a bunch of new changes I kept out of this PR to keep it small. Allterco is delivering a lot of nee devices, which change a) in direct changes b) require changes to the generic logic and c) maybe bugs need to be fixed. So it's unrealistic to serialize those changes, complete one by the other and the create a dozen 50 line PRs. Do I need to remove the 30 lines bug fixes and crate another PR for those? |
Let's keep them. |
|
Note that there is one file in conflict. |
...inding.shelly/src/main/java/org/openhab/binding/shelly/internal/api/ShellyDeviceProfile.java
Outdated
Show resolved
Hide resolved
....binding.shelly/src/main/java/org/openhab/binding/shelly/internal/api2/Shelly2ApiClient.java
Outdated
Show resolved
Hide resolved
...ding.shelly/src/main/java/org/openhab/binding/shelly/internal/handler/ShellyBaseHandler.java
Outdated
Show resolved
Hide resolved
...ding.shelly/src/main/java/org/openhab/binding/shelly/internal/handler/ShellyBaseHandler.java
Outdated
Show resolved
Hide resolved
|
@markus7017 : I reviewed fully your PR. There is one important comment to consider. Please also note that a file conflict has to be resolved. |
|
@markus7017 : I do not know if you are available now. There is a new milestone build that will start in few hours. If you can consider my comments and resolve the conflict, I will try to merge your PR just in time for the new milestone. |
|
@lolodomo I was busy the last weeks, just working on this PR and 2 important fixes |
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/oh4-runs-out-of-memory/148699/93 |
|
@lolodomo one question: pom.xml still includes a dependency on org.eclipse.jetty.websocket. Is this still required? Should I keep it for backward compatibility? |
[...]
I will not interfere with the agreement between you and @lolodomo, but just add that if those fixes are important/critical for 4.0 as well, I would advise you to create a separate PR with them so we can cherry-pick them into Also, I noticed many issues mentioned in the PR description. @markus7017, can you link those issues that will be closed by this PR? When they are linked, they will be automatically closed when the PR is merged, so it would make it easier to manage. |
...inding.shelly/src/main/java/org/openhab/binding/shelly/internal/api/ShellyDeviceProfile.java
Outdated
Show resolved
Hide resolved
...ding.shelly/src/main/java/org/openhab/binding/shelly/internal/handler/ShellyBaseHandler.java
Outdated
Show resolved
Hide resolved
That's strange, why did you do that first ?? You should rely on current version of Jetty "packaged" with OH server. We are currently at 9.4.52.v20230823 after upgrading to Karaf 4.4.4. |
|
@markus7017 : you marked all my comments as resolved without pushing any changes ! |
Hmm, I reviewed the above list, which is correct. Most of them were tiny fixes, one line or so. and this one includes 2 fixes for resource leaks when discovery handler fails (NOE+IPv6) All of them would be useful to fixes (serious) issues or little improvements. So would make sense also to get into 4.0. |
|
I know, I need to do a rebase and to double check that all resource leak fixes are included |
markus7017
changed the title
markus7017
changed the title
markus7017
changed the title
Signed-off-by: Markus Michels <[email protected]>
Signed-off-by: Markus Michels <[email protected]>
Signed-off-by: Markus Michels <[email protected]>
Signed-off-by: Markus Michels <[email protected]>
markus7017
force-pushed
the
shelly_init_discovery_motion
branch
from
ccf5d37
to
c51cdf2
Compare
markus7017
removed
awaiting other PR
|
@lolodomo This is the left-over based one the 2 PRs before :-) |
bundles/org.openhab.binding.shelly/src/main/resources/OH-INF/i18n/shelly.properties
Outdated
Show resolved
Hide resolved
...helly/src/main/java/org/openhab/binding/shelly/internal/manager/ShellyManagerActionPage.java
Outdated
Show resolved
Hide resolved
...ding.shelly/src/main/java/org/openhab/binding/shelly/internal/handler/ShellyBaseHandler.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Markus Michels <[email protected]>
|
@lolodomo requested changes applied |
Signed-off-by: Markus Michels <[email protected]>
@lolodomo changes applied |
|
@lolodomo thank you!! |
As you can notice, small PR = faster review |
…15401) Signed-off-by: Markus Michels <[email protected]>
…15401) Signed-off-by: Markus Michels <[email protected]> Signed-off-by: Jørgen Austvik <[email protected]>
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
The PR includes the following enhancements and fixes. Most changes are related to device discovery and initialization as well as BLU motion support. The other topics are small fixes, we were able to catch a bunch of them.
Signed-off-by: Markus Michels [email protected]