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.

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

[tellstick] Add support for Tellstick local API #10020

Merged
merged 3 commits into from
Feb 18, 2021

Conversation

jannegpriv
Copy link
Contributor

@jannegpriv jannegpriv commented Feb 1, 2021

Fixes #9841

Support for local API according to:
https://tellstick-server.readthedocs.io/en/v1.0.12/api.html

Signed-off-by: Jan Gustafsson [email protected]

Link to community thread where I posted a jar-file for test:
https://community.openhab.org/t/tellstick-local-api/90204/4

This binding was not updated to use Null anntotations, I have therfor not added that in my PR.

The local API only returns JSON format and the live API is implemented to handle XML format, in the future the live API should probably be updated to use JSON as well (since live API seems to support both XML and JSON).

Signed-off-by: Jan Gustafsson <[email protected]>
@jannegpriv jannegpriv requested a review from a team as a code owner February 1, 2021 16:04
@cpmeister cpmeister added the enhancement An enhancement or new feature for an existing add-on label Feb 7, 2021
@fwolter fwolter self-assigned this Feb 10, 2021
Comment on lines 107 to 110
public static final Set<ThingTypeUID> SUPPORTED_THING_TYPES_UIDS = Collections
.unmodifiableSet(Stream.of(DIMMER_THING_TYPE, SWITCH_THING_TYPE, SENSOR_THING_TYPE, RAINSENSOR_THING_TYPE,
WINDSENSOR_THING_TYPE, POWERSENSOR_THING_TYPE, TELLDUSCOREBRIDGE_THING_TYPE,
TELLDUSLIVEBRIDGE_THING_TYPE, TELLDUSLOCALBRIDGE_THING_TYPE).collect(Collectors.toSet()));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public static final Set<ThingTypeUID> SUPPORTED_THING_TYPES_UIDS = Collections
.unmodifiableSet(Stream.of(DIMMER_THING_TYPE, SWITCH_THING_TYPE, SENSOR_THING_TYPE, RAINSENSOR_THING_TYPE,
WINDSENSOR_THING_TYPE, POWERSENSOR_THING_TYPE, TELLDUSCOREBRIDGE_THING_TYPE,
TELLDUSLIVEBRIDGE_THING_TYPE, TELLDUSLOCALBRIDGE_THING_TYPE).collect(Collectors.toSet()));
public static final Set<ThingTypeUID> SUPPORTED_THING_TYPES_UIDS = Set.of(DIMMER_THING_TYPE, SWITCH_THING_TYPE, SENSOR_THING_TYPE, RAINSENSOR_THING_TYPE,
WINDSENSOR_THING_TYPE, POWERSENSOR_THING_TYPE, TELLDUSCOREBRIDGE_THING_TYPE,
TELLDUSLIVEBRIDGE_THING_TYPE, TELLDUSLOCALBRIDGE_THING_TYPE);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed for all Sets!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed for all sets!

private List<DeviceStatusListener> deviceStatusListeners = new Vector<>();
private final HttpClient httpClient;

private static final int REFRESH_DELAY = 10;
Copy link
Member

Choose a reason for hiding this comment

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

It's good practice to append the unit to the field name e.g. TIMEOUT_SEC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

Comment on lines 70 to 71
private ScheduledFuture<?> pollingJob;
private ScheduledFuture<?> immediateRefreshJob;
Copy link
Member

Choose a reason for hiding this comment

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

Please put all fields to the top.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!


@Override
public void dispose() {
logger.debug("Local Handler disposed.");
Copy link
Member

Choose a reason for hiding this comment

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

Can this message be replaced by using the debugger or by increasing the framework's log level? See point 4 https://www.openhab.org/docs/developer/guidelines.html#f-logging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it!


@Override
public void initialize() {
logger.debug("Initializing Telldus Local bridge handler.");
Copy link
Member

Choose a reason for hiding this comment

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

See above. Remove logging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it!

*
* @author Jan Gustafsson - Initial contribution
*/
public class TelldusLocalException extends TellstickException {
Copy link
Member

Choose a reason for hiding this comment

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

Please add the NonNullByDefault annotation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

*
* @author Jan Gustafsson - Initial contribution
*/
public class LocalDataTypeValue {
Copy link
Member

Choose a reason for hiding this comment

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

You could move this into a package called dto or append DTO to the class name to get rid of the checkstyle warning about missing NonNullByDefault annotation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

<config-description>
<parameter name="ipAddress" type="text" required="true">
<label>Local IP Address</label>
<description>The local IP address of the Tellstick.</description>
Copy link
Member

Choose a reason for hiding this comment

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

You could specify <context>network-address</context> to get a free validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

<description>The local IP address of the Tellstick.</description>
</parameter>
<parameter name="accessToken" type="text" required="true">
<context>credentials</context>
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this conext exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I have removed it!

<label>Access Token</label>
<description>The access token.</description>
</parameter>
<parameter name="refreshInterval" type="integer" required="false">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<parameter name="refreshInterval" type="integer" required="false">
<parameter name="refreshInterval" type="integer" required="false" min="0" unit="ms">

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

Signed-off-by: Jan Gustafsson <[email protected]>
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.

The titel of this PR will show up in the OH changelog. Can you make it a bit more self-explanatory?

for (TellstickLocalSensorDTO sensor : previouslist.getSensors()) {
sensor.setUpdated(false);
}
logger.trace("Update sensors, reset updated flag1");
Copy link
Member

Choose a reason for hiding this comment

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

Can this message be replaced by using the debugger or by increasing the framework's log level? See point 4 https://www.openhab.org/docs/developer/guidelines.html#f-logging

Please check all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

}
this.deviceList = newList;
} else {
logger.trace("updateDevices, Updating devices.");
Copy link
Member

Choose a reason for hiding this comment

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

Can this message be replaced by using the debugger or by increasing the framework's log level? See point 4 https://www.openhab.org/docs/developer/guidelines.html#f-logging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

private static final long serialVersionUID = 3067179547449454711L;

@Override
public @NonNull String getMessage() {
Copy link
Member

Choose a reason for hiding this comment

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

This is added automatically.

Suggested change
public @NonNull String getMessage() {
public String getMessage() {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got a compilation error if I remove it:

ERROR] /Users/jannegpriv/git/openhab-master3/git/openhab-addons/bundles/org.openhab.binding.tellstick/src/main/java/org/openhab/binding/tellstick/internal/local/TelldusLocalException.java:[35,12] The default '@org.eclipse.jdt.annotation.NonNull' conflicts with the inherited '@org.eclipse.jdt.annotation.Nullable' annotation in the overridden method from org.tellstick.device.TellstickException 

@jannegpriv jannegpriv changed the title [tellstick] Fix for #9841. [tellstick] Fix for #9841, adding support for Tellstick local API. Feb 17, 2021
Signed-off-by: Jan Gustafsson <[email protected]>
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

@fwolter fwolter merged commit 0853aa7 into openhab:main Feb 18, 2021
@fwolter fwolter added this to the 3.1 milestone Feb 18, 2021
@fwolter fwolter removed their assignment Feb 18, 2021
@jannegpriv jannegpriv deleted the 9841-tellstick branch March 31, 2021 18:01
themillhousegroup pushed a commit to themillhousegroup/openhab2-addons that referenced this pull request May 10, 2021
…API. (openhab#10020)

* Fix for openhab#9841.

Signed-off-by: Jan Gustafsson <[email protected]>
Signed-off-by: John Marshall <[email protected]>
@wborn wborn changed the title [tellstick] Fix for #9841, adding support for Tellstick local API. [tellstick] Add support for Tellstick local API Jun 22, 2021
thinkingstone pushed a commit to thinkingstone/openhab-addons that referenced this pull request Nov 7, 2021
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[tellstick] - Add support for local API
3 participants