openhab-addons icon indicating copy to clipboard operation
openhab-addons copied to clipboard

Published 20 hours ago • verified publisher icon openhab

[speedtest] Binding for Ookla's Speedtest - Initial contribution

Open bigbasec opened this issue 4 years ago • 18 comments

The goal of this binding was to create an easy to setup speedtest option. It's fairly low frills, though should work with very limited interaction outside of the main ui.

Does require the installation of Ookla's speedtest though has been tested on Linux, Windows and openHabian without issue.

Jar file : https://github.com/bigbasec/openhab-addons/releases/download/v0.4/org.openhab.binding.speedtest-3.1.0-SNAPSHOT.jar

bigbasec avatar Jan 22 '21 15:01 bigbasec

It might be an idea to merge any missing functionality into the speedtest thing that is already part of the Network Binding. :thinking:

See: https://www.openhab.org/addons/bindings/network/#thing-configuration

wborn avatar Jan 22 '21 16:01 wborn

That is all covered in the post on the forum. I didn't do that for a reason.

bigbasec avatar Jan 22 '21 16:01 bigbasec

Successfully tested under Linux running OH3.0.1 in a Docker using v0.5

MikeTheTux avatar Mar 09 '21 18:03 MikeTheTux

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

https://community.openhab.org/t/oh3-and-speedtest-ookla/124721/25

openhab-bot avatar Jul 22 '21 12:07 openhab-bot

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

https://community.openhab.org/t/speedtest-cli-by-ookla-internet-up-downlink-measurement-integration/94447/106

openhab-bot avatar Jul 22 '21 12:07 openhab-bot

@bigbasec When you have completed going over the suggestions please post that you have finished and want someone to take a look.

Skinah avatar Oct 03 '21 10:10 Skinah

Last activity form the author was 17 months ago. Unfortunately, it might be time to close this PR ?

lolodomo avatar Jun 11 '22 07:06 lolodomo

@bigbasec : if you want to resume the work on this binding, you are welcome to reopen this PR.

lolodomo avatar Jun 11 '22 12:06 lolodomo

In https://github.com/MikeTheTux/openhab-addons/tree/speedtest I fixed most of the review findings from above. My TODOs for the next days:

  • Introduction of UOM
  • Improvement of Docu

@bigbasec, I would like to ask for your consent in order to contribute this great binding into openHAB.

MikeTheTux avatar Oct 10 '22 18:10 MikeTheTux

100%. I tried that when I first wrote it and I was declined because it was something someone could do other ways. Was told that it shouldn't need to be a binding. Sort of why I stopped doing any development on the platform. But hey, if you have better luck by all means go for it.

bigbasec avatar Oct 11 '22 01:10 bigbasec

Feel free to reopen the Pull request as it was already offered just above. Don't let people in a forum discourage you, we value everyone that wants to contribute and do not wish someone that does contribute to move away from our project or lose interest.

If someone will code and maintain a binding PLUS someone will review it, then it should get merged IMHO. Otherwise we loose people that are willing to contribute and we always need more of them. Multiple people have contributed to reviewing as well as we now have/had two willing to maintain the code, so let's move this forward.

The person who wants to move it forward, just reopen this as bigbasec has given the OK and signed off the contribution to opensource. Thanks for all contributions made.

Skinah avatar Oct 11 '22 04:10 Skinah

Great to see that this binding is able to continue and therefore the efforts of @bigbasec are not lost.

MikeTheTux avatar Oct 11 '22 06:10 MikeTheTux

Re-based the changes to main (OH3.4.0) and resolved the existing conflict. Now we have a green build and I'm ready to continue developing on the remaining review findings.

MikeTheTux avatar Oct 11 '22 06:10 MikeTheTux

@bigbasec When you have completed going over the suggestions please post that you have finished and want someone to take a look.

Finished - please have a look.

Remaining open point:

  • Improvement of Docu

MikeTheTux avatar Oct 11 '22 19:10 MikeTheTux

updated docu.

I guess now it's time to remove the stale and await feedback flag ;-)

MikeTheTux avatar Oct 12 '22 17:10 MikeTheTux

Can you fix the DCO signoff issues?

After this link works, can you then create a post on the forum so this goes into the Marketplace? Having multiple users reporting that the binding works and is useful will help convince any nay sayers that this should get added.

https://openhab.jfrog.io/artifactory/libs-pullrequest-local/org/openhab/addons/bundles/org.openhab.binding.speedtest/3.4.0-SNAPSHOT/org.openhab.binding.speedtest-3.4.0-SNAPSHOT.jar

Skinah avatar Oct 12 '22 23:10 Skinah

Can you fix the DCO signoff issues?

done

After this link works, can you then create a post on the forum so this goes into the Marketplace?

There is already a Forum topic. I will check if this one can be re-used for this purpose

MikeTheTux avatar Oct 13 '22 05:10 MikeTheTux

After this link works, can you then create a post on the forum so this goes into the Marketplace?

Now also on Marketplace: https://community.openhab.org/t/ooklas-speedtest-binding/139924

MikeTheTux avatar Oct 13 '22 18:10 MikeTheTux

Can you add yourself to the codeowners list/file? When someone raises an issue or PR in the future you then get notified.

Skinah avatar Oct 16 '22 01:10 Skinah

Can you add yourself to the codeowners list/file? When someone raises an issue or PR in the future you then get notified.

done

MikeTheTux avatar Oct 16 '22 06:10 MikeTheTux

You might also want to look into the last SAT warning: NonThreadSafeSingleton

done

MikeTheTux avatar Dec 03 '22 09:12 MikeTheTux

As I can see there are some comments to be addressed and a conflict to be resolved I'm updating review status.

Think everything is addressed now.

lsiepel avatar Jan 24 '23 22:01 lsiepel

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

@jlaur, compiled it in an OH4.0.0 environment and report.html contains zero findings.

MikeTheTux avatar Mar 05 '23 09:03 MikeTheTux

@openhab/add-ons-maintainers - I could use a second pair of eyes on the approach of requiring software installation on the host OS and calling an executable.

@wborn - thanks for joining the review, can you help with input here?

jlaur avatar Mar 12 '23 19:03 jlaur

can you help with input here?

What kind of input?

We already have some other add-ons that require software installation and that create processes to use this software:

wborn avatar Mar 12 '23 20:03 wborn

can you help with input here?

What kind of input?

We already have some other add-ons that require software installation and that create processes to use this software:

That kind of input. 🙂 Thanks! I just needed confirmation that having such requirements is fine/acceptable.

jlaur avatar Mar 12 '23 20:03 jlaur

@bigbasec - there are three compiler warnings left to fix:

Warning:  /home/runner/work/openhab-addons/openhab-addons/bundles/org.openhab.binding.speedtest/src/main/java/org/openhab/binding/speedtest/internal/SpeedtestHandler.java:[164,39] Potential null pointer access: this expression has a '@Nullable' type
Warning:  /home/runner/work/openhab-addons/openhab-addons/bundles/org.openhab.binding.speedtest/src/main/java/org/openhab/binding/speedtest/internal/SpeedtestHandler.java:[174,36] Potential null pointer access: this expression has a '@Nullable' type
Warning:  /home/runner/work/openhab-addons/openhab-addons/bundles/org.openhab.binding.speedtest/src/main/java/org/openhab/binding/speedtest/internal/SpeedtestHandler.java:[175,13] Potential null pointer access: this expression has a '@Nullable' type

I'll comment one of them with a proposed solution.

jlaur avatar Apr 02 '23 19:04 jlaur