Skip to content

Commit

Permalink
Second fwolter code review
Browse files Browse the repository at this point in the history 
Signed-off-by: clinique <[email protected]>
  • Loading branch information
@clinique
clinique committed Jun 22, 2023
1 parent 001a6cf commit f57f7d3
Show file tree
Hide file tree
Showing 27 changed files with 106 additions and 109 deletions.
2 changes: 1 addition & 1 deletion bundles/org.openhab.binding.freeboxos/NOTICE
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ https://github.com/openhab/openhab-addons

== Third-party Content

IPAddress : Java library for handling IP addresses and subnets, both IPv4 and IPv6
IPAddress: Java library for handling IP addresses and subnets, both IPv4 and IPv6
* License: Apache License 2.0
* Project: https://github.com/seancfoley/IPAddress
* Source: https://github.com/seancfoley/IPAddress
24 changes: 12 additions & 12 deletions bundles/org.openhab.binding.freeboxos/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -196,17 +196,17 @@ The following channels are supported:

The following actions are available in rules/scripting:

| Thing Type | Action Name | Parameters | Description |
|-------------|------------------|---------------------------|------------------------------------------------------|
| host | wol | None | Sends a wake on lan packet to the lan connected host |
| player | reboot | None | Reboots the player device |
| player | sendKey | key : String | Send a key (remote emulation) to the player |
| player | sendLongKey | key : String | Sends the key emulating a longpress on the button |
| player | sendMultipleKeys | keys : String | Sends multiple keys to the player, comma separated |
| player | sendKeyRepeat | key : String, count : int | Sends the key multiple times |
| server | reboot | None | Reboots the Freebox Server |
| freeplug | reset | None | Resets the Freeplug |
| call | reset | None | Clears the call log queue |
| repeater | reboot | None | Reboots the Repeater |
| Thing Type | Action Name | Parameters | Description |
|-------------|------------------|-------------------------|------------------------------------------------------|
| host | wol | None | Sends a wake on lan packet to the lan connected host |
| player | reboot | None | Reboots the player device |
| player | sendKey | key: String | Send a key (remote emulation) to the player |
| player | sendLongKey | key: String | Sends the key emulating a longpress on the button |
| player | sendMultipleKeys | keys: String | Sends multiple keys to the player, comma separated |
| player | sendKeyRepeat | key: String, count: int | Sends the key multiple times |
| server | reboot | None | Reboots the Freebox Server |
| freeplug | reset | None | Resets the Freeplug |
| call | reset | None | Clears the call log queue |
| repeater | reboot | None | Reboots the Repeater |


2 changes: 1 addition & 1 deletion ...inding.freeboxos/src/main/java/org/openhab/binding/freeboxos/internal/api/ApiHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ public ApiHandler(HttpClient httpClient, TimeZoneProvider timeZoneProvider) {

public synchronized <T> T executeUri(URI uri, HttpMethod method, Class<T> clazz, @Nullable String sessionToken,
@Nullable Object payload) throws FreeboxException, InterruptedException {
logger.debug("executeUrl {} : {} ", method, uri);
logger.debug("executeUrl {}: {} ", method, uri);

Request request = httpClient.newRequest(uri).method(method).timeout(timeoutInMs, TimeUnit.MILLISECONDS)
.header(HttpHeader.CONTENT_TYPE, CONTENT_TYPE);
Expand Down
2 changes: 1 addition & 1 deletion ...boxos/src/main/java/org/openhab/binding/freeboxos/internal/api/FreeboxOsIconProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ protected Integer getPriority() {
return new ByteArrayInputStream(response.getContent());
}
} catch (InterruptedException | TimeoutException | ExecutionException e) {
logger.warn("Error getting icon {} : {}", resourceName, e.getMessage());
logger.warn("Error getting icon {}: {}", resourceName, e.getMessage());
}
return null;
}
Expand Down
4 changes: 2 additions & 2 deletions ...boxos/src/main/java/org/openhab/binding/freeboxos/internal/api/rest/FreeboxOsSession.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public static record ApiVersion(String apiBaseUrl, @Nullable String apiDomain, S
String uid) {

/**
* @return a string like eg : '/api/v8'
* @return a string like eg: '/api/v8'
*/
private String baseUrl() {
return "%sv%s".formatted(apiBaseUrl, apiVersion.split("\\.")[0]);
Expand Down Expand Up @@ -127,7 +127,7 @@ public void closeSession() {
getManager(LoginManager.class).closeSession();
session = null;
} catch (FreeboxException e) {
logger.info("Error closing session: {}", e.getMessage());
logger.warn("Error closing session: {}", e.getMessage());
}
}
appToken = "";
Expand Down
2 changes: 1 addition & 1 deletion ....freeboxos/src/main/java/org/openhab/binding/freeboxos/internal/api/rest/HomeManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ private static enum DisplayType {
UNKNOWN;
}

private static record EndpointValue<T>(T value) {
private static record EndpointValue<T> (T value) {
}

private static record EndpointUi(AccessType access, DisplayType display, String iconUrl, @Nullable String unit) {
Expand Down
4 changes: 2 additions & 2 deletions ...freeboxos/src/main/java/org/openhab/binding/freeboxos/internal/api/rest/LoginManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public class LoginManager extends RestManager {
private static final String SESSION = "session";
private static final String AUTHORIZE_ACTION = "authorize";
private static final String LOGOUT = "logout";
private static final int GRANT_DELAY_MIN = 3;
private static final int GRANT_DELAY_SEC = 180;

private final Mac mac;

Expand Down Expand Up @@ -145,7 +145,7 @@ private Status trackAuthorize(int trackId) throws FreeboxException {
public String grant() throws FreeboxException {
Authorization authorize = post(new AuthorizeData(APP_ID, BUNDLE), AuthResponse.class, AUTHORIZE_ACTION);
Status track = Status.PENDING;
ZonedDateTime timeLimit = ZonedDateTime.now().plusMinutes(GRANT_DELAY_MIN);
ZonedDateTime timeLimit = ZonedDateTime.now().plusSeconds(GRANT_DELAY_SEC);
try {
while (Status.PENDING.equals(track) && ZonedDateTime.now().isBefore(timeLimit)) {
Thread.sleep(2000);
Expand Down
2 changes: 1 addition & 1 deletion ...reeboxos/src/main/java/org/openhab/binding/freeboxos/internal/api/rest/PlayerManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ private static enum StbType {
}

/**
* @return a string like eg : '17/api/v8'
* @return a string like eg: '17/api/v8'
*/
private @Nullable String baseUrl() {
String api = apiVersion;
Expand Down
2 changes: 1 addition & 1 deletion ....freeboxos/src/main/java/org/openhab/binding/freeboxos/internal/api/rest/RestManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public RestManager(FreeboxOsSession session, LoginManager.Permission required, U
this.uriBuilder = uri;
this.session = session;
if (required != LoginManager.Permission.NONE && !session.hasPermission(required)) {
throw new PermissionException(required, "Permission missing : %s", required.toString());
throw new PermissionException(required, "Permission missing: %s", required.toString());
}
}

Expand Down
8 changes: 4 additions & 4 deletions ...boxos/src/main/java/org/openhab/binding/freeboxos/internal/api/rest/WebSocketManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ public void onWebSocketConnect(@NonNullByDefault({}) Session wsSession) {
try {
wsSession.getRemote().sendString(apiHandler.serialize(REGISTRATION));
} catch (IOException e) {
logger.warn("Error connecting to websocket : {}", e.getMessage());
logger.warn("Error connecting to websocket: {}", e.getMessage());
}
}

Expand All @@ -133,7 +133,7 @@ public void onWebSocketText(@NonNullByDefault({}) String message) {
handleNotification(result);
break;
default:
logger.info("Unhandled notification received : {}", result.action);
logger.warn("Unhandled notification received: {}", result.action);
}
}
}
Expand All @@ -160,7 +160,7 @@ private void handleNotification(WebSocketResponse result) {
}
break;
default:
logger.warn("Unhandled event received : {}", result.getEvent());
logger.warn("Unhandled event received: {}", result.getEvent());
}
} else {
logger.warn("Empty json element in notification");
Expand All @@ -175,7 +175,7 @@ public void onWebSocketClose(int statusCode, @NonNullByDefault({}) String reason

@Override
public void onWebSocketError(@NonNullByDefault({}) Throwable cause) {
logger.warn("Error on websocket : {}", cause.getMessage());
logger.warn("Error on websocket: {}", cause.getMessage());
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion ...main/java/org/openhab/binding/freeboxos/internal/discovery/FreeboxOsDiscoveryService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ protected void startScan() {
discoverHosts(localHandler, bridgeUID, lanHosts);
}
} catch (FreeboxException e) {
logger.warn("Error while requesting data for things discovery : {}", e.getMessage());
logger.warn("Error while requesting data for things discovery: {}", e.getMessage());
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion ...xos/src/main/java/org/openhab/binding/freeboxos/internal/handler/ActivePlayerHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public void reboot() {
try {
getManager(PlayerManager.class).reboot(getClientId());
} catch (FreeboxException e) {
logger.warn("Error rebooting : {}", e.getMessage());
logger.warn("Error rebooting: {}", e.getMessage());
}
});
}
Expand Down
99 changes: 49 additions & 50 deletions ...oxos/src/main/java/org/openhab/binding/freeboxos/internal/handler/ApiConsumerHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@

import java.math.BigDecimal;
import java.time.ZonedDateTime;
import java.util.HashMap;
import java.util.Hashtable;
import java.util.Map;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;

Expand Down Expand Up @@ -67,8 +67,8 @@
@NonNullByDefault
abstract class ApiConsumerHandler extends BaseThingHandler implements ApiConsumerIntf {
private final Logger logger = LoggerFactory.getLogger(ApiConsumerHandler.class);
private final Map<String, ScheduledFuture<?>> jobs = new HashMap<>();

private @Nullable ScheduledFuture<?> globalJob;
private @Nullable ServiceRegistration<?> reg;

ApiConsumerHandler(Thing thing) {
Expand All @@ -77,7 +77,6 @@ abstract class ApiConsumerHandler extends BaseThingHandler implements ApiConsume

@Override
public void initialize() {
logger.debug("Initializing handler for thing {}", getThing().getUID());
FreeboxOsHandler bridgeHandler = checkBridgeHandler();
if (bridgeHandler == null) {
return;
Expand All @@ -90,7 +89,7 @@ public void initialize() {
checkAirMediaCapabilities(properties);
updateProperties(properties);
} catch (FreeboxException e) {
logger.warn("Error getting thing {} properties : {}", thing.getUID(), e.getMessage());
logger.warn("Error getting thing {} properties: {}", thing.getUID(), e.getMessage());
}
}

Expand Down Expand Up @@ -118,7 +117,7 @@ private void configureMediaSink(FreeboxOsHandler bridgeHandler, String upnpName)
}
}
} catch (FreeboxException e) {
logger.warn("Unable to retrieve Media Receivers");
logger.warn("Unable to retrieve Media Receivers: {}", e.getMessage());
}
}

Expand All @@ -134,11 +133,10 @@ public <T extends RestManager> T getManager(Class<T> clazz) throws FreeboxExcept

@Override
public void bridgeStatusChanged(ThingStatusInfo bridgeStatusInfo) {
logger.debug("Thing {}: bridge status changed to {}", getThing().getUID(), bridgeStatusInfo);
if (checkBridgeHandler() != null) {
startRefreshJob();
} else {
stopRefreshJob();
stopJobs();
}
}

Expand All @@ -152,7 +150,7 @@ public void handleCommand(ChannelUID channelUID, Command command) {
logger.debug("Unexpected command {} on channel {}", command, channelUID.getId());
}
} catch (FreeboxException e) {
logger.warn("Error handling command : {}", e.getMessage());
logger.warn("Error handling command: {}", e.getMessage());
}
}

Expand Down Expand Up @@ -186,8 +184,7 @@ private void checkAirMediaCapabilities(Map<String, String> properties) throws Fr

@Override
public void dispose() {
logger.debug("Disposing handler for thing {}", getThing().getUID());
stopRefreshJob();
stopJobs();
ServiceRegistration<?> localReg = reg;
if (localReg != null) {
localReg.unregister();
Expand All @@ -196,47 +193,54 @@ public void dispose() {
}

private void startRefreshJob() {
ScheduledFuture<?> job = globalJob;
if (job == null || job.isCancelled()) {
int refreshInterval = getConfigAs(ApiConsumerConfiguration.class).refreshInterval;
logger.debug("Scheduling state update every {} seconds for thing {}...", refreshInterval,
getThing().getUID());
ThingStatusDetail detail = thing.getStatusInfo().getStatusDetail();
if (ThingStatusDetail.DUTY_CYCLE.equals(detail)) {
boolean rebooting = true;
while (rebooting) {
try {
internalPoll();
rebooting = false;
} catch (FreeboxException ignore) {
try {
Thread.sleep(20000);
} catch (InterruptedException e) {
rebooting = false;
}
}
}
removeJob("GlobalJob");

int refreshInterval = getConfigAs(ApiConsumerConfiguration.class).refreshInterval;
logger.debug("Scheduling state update every {} seconds for thing {}...", refreshInterval, getThing().getUID());

ThingStatusDetail detail = thing.getStatusInfo().getStatusDetail();
if (ThingStatusDetail.DUTY_CYCLE.equals(detail)) {
try {
internalPoll();
} catch (FreeboxException ignore) {
// An exception is normal if the box is rebooting then let's try again later...
addJob("Initialize", this::initialize, 10, TimeUnit.SECONDS);
return;
}
}

globalJob = scheduler.scheduleWithFixedDelay(() -> {
try {
internalPoll();
} catch (FreeboxException e) {
logger.warn("Error polling thing {} : {}", getThing().getUID(), e.getMessage());
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, e.getMessage());
}
}, 0, refreshInterval, TimeUnit.SECONDS);
addJob("GlobalJob", () -> {
try {
internalPoll();
} catch (FreeboxException e) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, e.getMessage());
}
}, 0, refreshInterval, TimeUnit.SECONDS);
}

private void removeJob(String name) {
ScheduledFuture<?> existing = jobs.get(name);
if (existing != null && !existing.isCancelled()) {
existing.cancel(true);
}
}

@Override
public void stopRefreshJob() {
ScheduledFuture<?> job = globalJob;
if (job != null && !job.isCancelled()) {
logger.debug("Stop scheduled state update for thing {}", getThing().getUID());
job.cancel(true);
globalJob = null;
}
public void addJob(String name, Runnable command, long initialDelay, long delay, TimeUnit unit) {
removeJob(name);
jobs.put(name, scheduler.scheduleWithFixedDelay(command, initialDelay, delay, unit));
}

@Override
public void addJob(String name, Runnable command, long delay, TimeUnit unit) {
removeJob(name);
jobs.put(name, scheduler.schedule(command, delay, unit));
}

@Override
public void stopJobs() {
jobs.keySet().forEach(name -> removeJob(name));
jobs.clear();
}

protected boolean internalHandleCommand(String channelId, Command command) throws FreeboxException {
Expand Down Expand Up @@ -330,11 +334,6 @@ public void updateProperties(@Nullable Map<String, String> properties) {
super.updateProperties(properties);
}

@Override
public ScheduledExecutorService getScheduler() {
return scheduler;
}

@Override
public Configuration getConfig() {
return super.getConfig();
Expand Down
10 changes: 6 additions & 4 deletions ...eeboxos/src/main/java/org/openhab/binding/freeboxos/internal/handler/ApiConsumerIntf.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

import java.math.BigDecimal;
import java.util.Map;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
Expand All @@ -37,8 +37,6 @@
@NonNullByDefault
public interface ApiConsumerIntf extends ThingHandler {

ScheduledExecutorService getScheduler();

Map<String, String> editProperties();

Configuration getConfig();
Expand All @@ -47,7 +45,11 @@ public interface ApiConsumerIntf extends ThingHandler {

void updateStatus(ThingStatus status, ThingStatusDetail statusDetail, @Nullable String description);

void stopRefreshJob();
void stopJobs();

void addJob(String name, Runnable command, long initialDelay, long delay, TimeUnit unit);

void addJob(String name, Runnable command, long delay, TimeUnit unit);

default int getClientId() {
return ((BigDecimal) getConfig().get(ClientConfiguration.ID)).intValue();
Expand Down
3 changes: 1 addition & 2 deletions ...g.freeboxos/src/main/java/org/openhab/binding/freeboxos/internal/handler/CallHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,8 @@ private void updateCallChannels(Call call) {
public void emptyQueue() {
try {
getManager(CallManager.class).emptyQueue();
logger.info("Call log succesfully cleared");
} catch (FreeboxException e) {
logger.warn("Error clearing call logs : {}", e.getMessage());
logger.warn("Error clearing call logs: {}", e.getMessage());
}
}

Expand Down
Loading

0 comments on commit f57f7d3

Please sign in to comment.