Skip to content

Commit

Permalink
[argoclima] 1st round of review remarks (editorials)
Browse files Browse the repository at this point in the history
Signed-off-by: Mateusz Bronk <[email protected]>
  • Loading branch information
mbronk committed Aug 25, 2023
1 parent b3501d8 commit 7cc19e8
Show file tree
Hide file tree
Showing 12 changed files with 97 additions and 92 deletions.
2 changes: 0 additions & 2 deletions bundles/org.openhab.binding.argoclima/.gitignore

This file was deleted.

38 changes: 19 additions & 19 deletions bundles/org.openhab.binding.argoclima/README.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# ArgoClima Binding

The binding provides support for [ArgoClima](https://argoclima.com/en/) WiFi-enabled air conditioning devices which use ***Argo Web APP*** for control.
The binding provides support for [ArgoClima](https://argoclima.com/en/) Wi-Fi-enabled air conditioning devices which use ***Argo Web APP*** for control.
Refer to [Argo Web APP details](#argo-web-app-details) section for an example.

> ***IMPORTANT:*** The same vendor also manufactures HVAC devices supported by a [phone application](http://smart.argoclima.com/EN/).
Expand All @@ -17,9 +17,9 @@ See also [Argo protocol details](#argo-protocol-details) to find out more about
## Supported Things

- `argoclima-remote`: Represents a HVAC device which is controlled remotely - through vendor's web application
- `argoclima-local`: Represents a locally available device, which OpenHab interacts with directly *(or indirectly, through a stub server)*. Refer to [Connection Modes](#connection-modes) for more details.
- `argoclima-local`: Represents a locally available device, which openHAB interacts with directly *(or indirectly, through a stub server)*. Refer to [Connection Modes](#connection-modes) for more details.

The binding has been primarily developed and tested using [Ulisse 13 DCI ECO WiFi](https://argoclima.com/en/prodotti/argo-ulisse-eco/) device.
The binding has been primarily developed and tested using [Ulisse 13 DCI ECO Wi-Fi](https://argoclima.com/en/prodotti/argo-ulisse-eco/) device.

## Discovery

Expand Down Expand Up @@ -151,7 +151,7 @@ Both thing types are functionally equivalent and support the same channels.


Thing argoclima:argoclima-local:argoHvacLocalWithPassthroughPlusDirectEx "Argo HVAC (accessible both indirectly and directly, via pass-through mode, with explicit options)" [
hostname="192.168.0.3", // Direct address of the device (reachable from OpenHab)
hostname="192.168.0.3", // Direct address of the device (reachable from openHAB)
connectionMode="REMOTE_API_PROXY",

localDevicePort=1001,
Expand Down Expand Up @@ -302,31 +302,31 @@ Both thing types are functionally equivalent and support the same channels.
}
```

## Connection modes
## Connection Modes

### Basic modes
### Basic Modes

These modes assume the HVAC device is connected directly to the Internet.
This is the default (vendor-recommended) configuration which does not require any special network-level changes.
There are [security considerations](#argo-protocol-details) when using these modes, though.

#### Basic: Local connection

The device is locally available through the LAN. OpenHab sends commands directly (and polls for status).
The device is locally available through the LAN. openHAB sends commands directly (and polls for status).
![Basic local connection diagram](doc/Argoclima_connection_Basic_LOCAL_CONNECTION.png)

#### Basic: Remote connection

The device may not be locally available through the LAN (or the user wants silent/no-beep behavior). OpenHab sends commands to a remote vendor's service (and polls it for device status), while the device also talks to the same service and eventually learns the updates.
The device may not be locally available through the LAN (or the user wants silent/no-beep behavior). openHAB sends commands to a remote vendor's service (and polls it for device status), while the device also talks to the same service and eventually learns the updates.
Commands are delayed in this mode.
![Basic remote connection diagram](doc/Argoclima_connection_Basic_REMOTE_CONNECTION.png)
### Advanced modes
### Advanced Modes
In these modes, the HVAC traffic which is originally targeted towards vendor's servers is **rerouted on the network layer**, and flows to OpenHab instead.
In these modes, the HVAC traffic which is originally targeted towards vendor's servers is **rerouted on the network layer**, and flows to openHAB instead.
This is possible as the device does use plain HTTP (no TLS, certificate pinning etc.)

The following ```nftables``` snippet provides an example rule redirecting traffic to ```31.14.128.210``` (Argo server) to a local OpenHab instance listening at ```192.168.0.15:8239```.
The following ```nftables``` snippet provides an example rule redirecting traffic to ```31.14.128.210``` (Argo server) to a local openHAB instance listening at ```192.168.0.15:8239```.
Please note this is **not** a complete configuration, and an actual secure configuration requires a few more rules and network setup.

```text
Expand All @@ -346,17 +346,17 @@ Please note this forwarding rule would need to be accompanied with other traffic

#### Advanced: Local connection with pass-through proxy

In this mode OpenHab is acting as an **almost** transparent proxy, and does a pass-through of device-side messages and remote-side responses (a man-in-the-middle).
This allows to have the device fully controllable via OpenHab **as well as** vendor's application (at the expense of security!).
Possible other use of this mode is for firmware update or ad-hoc controlling some settings which are not easily accessible via OpenHab.
> ***IMPORTANT***: Most of the time, OpenHab serves as a fully transparent proxy, not interfering with the traffic, **except for** cases when cloud has no updates for the device while OpenHab **has** a command pending send to the device.
In this mode openHAB is acting as an **almost** transparent proxy, and does a pass-through of device-side messages and remote-side responses (a man-in-the-middle).
This allows to have the device fully controllable via openHAB **as well as** vendor's application (at the expense of security!).
Possible other use of this mode is for firmware update or ad-hoc controlling some settings which are not easily accessible via openHAB.
> ***IMPORTANT***: Most of the time, openHAB serves as a fully transparent proxy, not interfering with the traffic, **except for** cases when cloud has no updates for the device while openHAB **has** a command pending send to the device.
> In such case, the binding injects it into the communication flow as-if it was cloud-issued!
![Advanced local connection diagram: REMOTE_API_PROXY mode](doc/Argoclima_connection_Advanced_REMOTE_API_PROXY.png)

#### Advanced: Local connection with STUB (simulated server) - RECOMMENDED

In this mode OpenHab is simulating vendor's server (which is out of the picture).
In this mode openHAB is simulating vendor's server (which is out of the picture).
The HVAC is functioning fully locally.
This is the recommended mode for maximum security.

Expand Down Expand Up @@ -386,16 +386,16 @@ This is true even if the device is desired to be controlled via local APIs only!
The Argo APIs support all functionalities of the device's remote, and two-way communication, except for **room temperature** (iFeel) provided from external sensor (the device's remote control).
Its value is read-only in the API protocol.

While using the temperature sensor built-in into the device is sufficient for most cases, for certain scenarios it may be desired to be able to set the room temperature from OpenHab (for example: to have multi-sensor aggregate temperature display on the built-in HVAC display).
While using the temperature sensor built-in into the device is sufficient for most cases, for certain scenarios it may be desired to be able to set the room temperature from openHAB (for example: to have multi-sensor aggregate temperature display on the built-in HVAC display).

Since the iFeel temperature updates **have to** be sent via infrared, there are two notable options to consider:

- **Use the original remote**:
As long as the remote control is pointing towards the HVAC and A/C status on the remote is ```ON``` (with iFeel option enabled), it will periodically beam current temperature (which the HVAC will accept if its iFeel option is ```ON```, which can be conficured through the binding).

- **Use OpenHab-controlled (custom) IR Blaster**:
- **Use openHAB-controlled (custom) IR Blaster**:

The Argo protocol is fully supported by the [IRremoteESP8266](https://github.com/crankyoldgit/IRremoteESP8266) library, and [Tasmota](https://github.com/arendst/Tasmota). Hence, any [Tasmota-IR](https://tasmota.github.io/docs/Tasmota-IR/)-compatible device would be able to send iFeel data to the HVAC (and the device itself can be controlled via OpenHab's [MQTT Binding](https://www.openhab.org/addons/bindings/mqtt/)).
The Argo protocol is fully supported by the [IRremoteESP8266](https://github.com/crankyoldgit/IRremoteESP8266) library, and [Tasmota](https://github.com/arendst/Tasmota). Hence, any [Tasmota-IR](https://tasmota.github.io/docs/Tasmota-IR/)-compatible device would be able to send iFeel data to the HVAC (and the device itself can be controlled via openHAB's [MQTT Binding](https://www.openhab.org/addons/bindings/mqtt/)).

For example, (by default) the following command sent to ```cmnd/<tasmota_dev_id>/irhvac``` channel does send the iFeel temperature.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ protected String pollForCurrentStatusFromDeviceSync(URL url) throws ArgoLocalApi
var cause = Optional.ofNullable(ex.getCause());
if (cause.isPresent() && cause.get() instanceof EOFException) {
throw new ArgoLocalApiCommunicationException(
"Device did not respond on its socket (EOF). Check that the device is correctly communicating with Argo servers (or OpenHab stub server)");
"Device did not respond on its socket (EOF). Check that the device is correctly communicating with Argo servers (or openHAB stub server)");
}
throw new ArgoLocalApiCommunicationException(
"Device communication error: " + Objects.requireNonNullElse(ex.getCause(), ex).getMessage(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public class ArgoClimaLocalDevice extends ArgoClimaDeviceApiBase {
* @param localDeviceIpAddress Optional, local subnet IP of the device (ex. if behind NAT). Used to match
* intercepted responses (in indirect mode) to this thing. This may be
* {@link ArgoClimaConfigurationLocal#getMatchAnyIncomingDeviceIp() bypassed}
* @param cpuId Optional, CPUID of the WiFi chip of the device. If provided, will be used to match intercepted
* @param cpuId Optional, CPUID of the Wi-Fi chip of the device. If provided, will be used to match intercepted
* responses (in indirect mode) to this thing
* @param client The common HTTP client used for issuing direct requests
* @param timeZoneProvider System-wide TZ provider, for parsing/displaying local dates
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@
* actual vendor's servers)
*
* <p>
* Use of this mode is actually NOT recommended for advanced users as cleartext device & WiFi passwords are sent to Argo
* servers through unencrypted HTTP connection (sic!). If the Argo UI access is desired (ex. for FW update or IR
* remote-like experience), consider using this mode only on a dedicated WiFi network (and possibly through VPN)
* Use of this mode is actually NOT recommended for advanced users as cleartext device & Wi-Fi passwords are sent to
* Argo servers through unencrypted HTTP connection (sic!). If the Argo UI access is desired (ex. for FW update or IR
* remote-like experience), consider using this mode only on a dedicated Wi-Fi network (and possibly through VPN)
*
* @author Mateusz Bronk - Initial contribution
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ Duration getLastSeenDelta() {
}

/**
* Return the properties in a map (ready to pass on to OpenHab engine)
* Return the properties in a map (ready to pass on to openHAB engine)
*
* @param timeZoneProvider TZ provider, for parsing date/time values
* @return Properties map
Expand Down Expand Up @@ -219,7 +219,7 @@ public void throwIfStatusIsStale() throws ArgoLocalApiCommunicationException {
var delta = this.properties.getLastSeenDelta();
if (delta.toSeconds() > ArgoClimaConfigurationRemote.LAST_SEEN_UNAVAILABILITY_THRESHOLD.toSeconds()) {
throw new ArgoLocalApiCommunicationException(MessageFormat.format(
"Device was last seen {0} mins ago (threshold is set at {1} min). Please ensure the HVAC is connected to WiFi and communicating with Argo servers",
"Device was last seen {0} mins ago (threshold is set at {1} min). Please ensure the HVAC is connected to Wi-Fi and communicating with Argo servers",
delta.toMinutes(), ArgoClimaConfigurationRemote.LAST_SEEN_UNAVAILABILITY_THRESHOLD.toMinutes()));
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,13 @@ public State fromDeviceResponse(List<String> responseElements) {
return UnDefType.NULL; // Write-only elements do not have any state reported
}

public State fromDeviceCommand(List<String> responseElements) {
if (this.type == DataElementType.READ_WRITE || this.type == DataElementType.WRITE_ONLY) {
return this.rawValue.updateFromApiResponse(responseElements.get(statusUpdateRequestIndex));
}
return UnDefType.NULL; // Write-only elements do not have any state reported
}

/**
* Output this element's currently-stored value in OH framework-compatible representation
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@
* targeting Argo remote server are instead targeted at OpenHAB instance!
* <p>
* IMPORTANT: Argo HVAC, even when functioning in full-local mode (controlled directly, via local IP), **requires**
* connection to a "remote" server, and will drop WiFi connection if it doesn't receive a valid protocolar response.
* connection to a "remote" server, and will drop Wi-Fi connection if it doesn't receive a valid protocolar response.
* Hence in order to isolate HVAC from OEM's server, having a simulated/stubbed local API server is required even for
* using the local APIs only
*
Expand Down Expand Up @@ -94,7 +94,7 @@ public enum DeviceRequestType {
/** UI Update? - NOTE: Not known when the device sends this... */
GET_UI_UPD,

/** WiFi firmware update request */
/** Wi-Fi firmware update request */
GET_OU_FW,

/** Unit firmware update request */
Expand Down Expand Up @@ -175,7 +175,7 @@ public void handle(@Nullable String target, @Nullable Request baseRequest, @Null
} else {
Optional<ContentResponse> upstreamResponse = Optional.empty();
try {
// CONSIDER: This implementation does NOT do any request pre-processing (ex. scrambling WiFi
// CONSIDER: This implementation does NOT do any request pre-processing (ex. scrambling Wi-Fi
// password Argo has no need of knowing). It may be a nice enhancement in the future
upstreamResponse = Optional.of(passthroughClient.get().passthroughRequest(baseRequest, body));
} catch (InterruptedException | TimeoutException | ExecutionException e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,10 @@ public class UiFlgSetupParam {
/** The binary value upon conversion (empty on conversion failure) */
private Optional<byte[]> bytes = Optional.empty();

/** The WiFi SSID embedded on bytes 0..31 of the blob (or empty - on parse failure) */
/** The Wi-Fi SSID embedded on bytes 0..31 of the blob (or empty - on parse failure) */
public Optional<String> wifiSSID = Optional.empty();

/** The WiFi password embedded on bytes 32..63 of the blob (or empty - on parse failure) */
/** The Wi-Fi password embedded on bytes 32..63 of the blob (or empty - on parse failure) */
public Optional<String> wifiPassword = Optional.empty();

/** The UI username embedded on bytes 64..79 of the blob (or empty - on parse failure) */
Expand All @@ -73,12 +73,12 @@ public class UiFlgSetupParam {
public Optional<String> localIP = Optional.empty();

/**
* The installed(?) WiFi firmware version embedded on bytes 132-137 of the blob (or empty - on parse failure)
* The installed(?) Wi-Fi firmware version embedded on bytes 132-137 of the blob (or empty - on parse failure)
*/
public Optional<String> wifiVersionInstalled = Optional.empty();

/**
* The available(?) WiFi firmware version embedded on bytes 138-143 of the blob (or empty - on parse failure)
* The available(?) Wi-Fi firmware version embedded on bytes 138-143 of the blob (or empty - on parse failure)
*/
public Optional<String> wifiVersionAvailable = Optional.empty();

Expand Down Expand Up @@ -192,7 +192,7 @@ public String toString() {
/** The {@code FW_OU} part of the request. Carries current version of unit's firmware */
public final String unitFirmware;

/** The {@code FW_UI} part of the request. Carries current version of WiFi firmware */
/** The {@code FW_UI} part of the request. Carries current version of Wi-Fi firmware */
public final String wifiFirmware;

/** The {@code CPU_ID} part of the request. Carries a unique HVAC device chip ID */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public static final class UiFlgResponsePreamble {
/** Unknown purpose, always one */
public int flag2alwaysOne = 1;

/** Request to update WiFi firmware of the device */
/** Request to update Wi-Fi firmware of the device */
public int flag3updateWifiFW = 0;

/** Request to update Unit firmware of the device */
Expand Down
Loading

0 comments on commit 7cc19e8

Please sign in to comment.