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

[bigbasec]

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

[wborn]

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

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

[bigbasec]

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

[MikeTheTux]

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

[openhab-bot]

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]

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

[Skinah]

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

[lsiepel]

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.

[jlaur]

Thank you for this new binding. I have provided a few comments mostly towards the documentation.

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.

@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.

[jlaur]

This seems like a stateless channel? In this case you could apply this policy:

Suggested change

	<description>Trigger in order to run Speedtest manually</description>
	<description>Trigger in order to run Speedtest manually</description>
	<autoUpdatePolicy>veto</autoUpdatePolicy>

See https://www.openhab.org/docs/developer/bindings/thing-xml.html#auto-update-policies

[MikeTheTux]

veto would be one possiblity to keep the state to OFF even when receiving the ON command.

Right now it is implemented in a way, that the framework sets the state to ON and the binding sets the sate back to OFF after the test was executed:

        if (ch.equals(SpeedtestBindingConstants.TRIGGER_TEST)) {
            if (command instanceof OnOffType) {
                if (command == OnOffType.ON) {
                    getSpeed();
                    updateState(channelUID, OnOffType.OFF);
                }
            }
        }

@jlaur, is there a preferred solution?

[jlaur]

Not that I'm aware of, I think both are valid solutions, and in both cases I believe you need to handle concurrent state changes (i.e. receiving ON while a speed test is already in progress).

[MikeTheTux]

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.

[wborn]

It looks already quite nice! 👍
I also added a few minor comments.

[jlaur]

@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?

[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:

• Pico TTS (PicoTTSAudioStream)
• Network Binding (NetworkUtils)

[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:

• Pico TTS (PicoTTSAudioStream)
• Network Binding (NetworkUtils)

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

[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]

Thanks again for your contribution, @bigbasec and @MikeTheTux. LGTM!

Now, you could add your binding's logo to the openHAB website. See https://www.openhab.org/docs/developer/bindings/#add-your-binding-s-logo-to-the-openhab-website