[emerald] Initial contribution for Emerald Heat Pump Hot Water Systems - #21477
[emerald] Initial contribution for Emerald Heat Pump Hot Water Systems#21477psmedley wants to merge 43 commits into
Conversation
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
There was a problem hiding this comment.
Pull request overview
Adds an initial openHAB binding intended to support Emerald Heat Pump Hot Water Systems, including basic Thing/channel metadata, a Thing handler, and build/packaging integration for the add-on.
Changes:
- Registers a new binding module in the reactor (
bundles/pom.xml), BOM (bom/openhab-addons/pom.xml), and CODEOWNERS. - Introduces binding metadata (add-on descriptor, thing/channel types, i18n) and a first implementation of a Thing handler and API DTOs.
- Adds a Karaf feature definition (and an AWS CRT runtime bundle) plus a PEM certificate resource.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| CODEOWNERS | Adds a codeowner entry for the new binding. |
| bundles/pom.xml | Adds the binding module to the bundles reactor and includes formatting changes. |
| bundles/org.openhab.binding.emerald/src/main/resources/SFSRootCAG2.pem | Adds a PEM certificate resource. |
| bundles/org.openhab.binding.emerald/src/main/resources/OH-INF/thing/thing-types.xml | Defines bridge/thing/channel types for the binding. |
| bundles/org.openhab.binding.emerald/src/main/resources/OH-INF/i18n/emeraldhws.properties | Adds default (en) i18n strings for the binding. |
| bundles/org.openhab.binding.emerald/src/main/resources/OH-INF/addon/addon.xml | Declares the add-on metadata. |
| bundles/org.openhab.binding.emerald/src/main/java/org/openhab/binding/emerald/internal/EmeraldHWSHandler.java | Implements a Thing handler with command handling and MQTT/state parsing. |
| bundles/org.openhab.binding.emerald/src/main/java/org/openhab/binding/emerald/internal/EmeraldHWSConfiguration.java | Adds Thing configuration DTO (uuid). |
| bundles/org.openhab.binding.emerald/src/main/java/org/openhab/binding/emerald/internal/api/Login.java | Adds a login response DTO for the Emerald API. |
| bundles/org.openhab.binding.emerald/src/main/java/org/openhab/binding/emerald/internal/api/List.java | Adds a list/device response DTO for the Emerald API. |
| bundles/org.openhab.binding.emerald/src/main/feature/feature.xml | Adds a Karaf feature to install the binding bundle and AWS CRT runtime bundle. |
| bundles/org.openhab.binding.emerald/pom.xml | Adds the binding’s Maven module definition and dependencies. |
| bundles/org.openhab.binding.emerald/NOTICE | Adds the binding NOTICE file. |
| bom/openhab-addons/pom.xml | Adds the binding artifact to the add-ons BOM. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
|
This is almost ready for review, need to review SAT, and also having problem on my produciton machine with: |
Signed-off-by: Paul Smedley <paul@smedley.id.au>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 10 comments.
Suppressed comments (14)
bundles/org.openhab.binding.emerald/src/main/java/org/openhab/binding/emerald/internal/EmeraldWebTargets.java:167
- invoke(...) unconditionally calls request.header(headerKey, headerValue) even when headerKey/headerValue are null (e.g., getToken calls invoke(..., null, null, ...)). Jetty will throw a NullPointerException, breaking authentication.
Request request = httpClient.newRequest(uri).method(method).header("accept", "*/*")
.header("content-type", "application/json")
.header("user-agent",
"EmeraldPlanet/2.5.3 (com.emerald-ems.customer; build:5; iOS 17.2.1) Alamofire/5.4.1")
.header("accept-language", "en-GB;q=1.0, en-AU;q=0.9").header(headerKey, headerValue)
.timeout(TIMEOUT_MS, TimeUnit.MILLISECONDS)
.content(new StringContentProvider(params), "application/json");
bundles/org.openhab.binding.emerald/src/main/java/org/openhab/binding/emerald/internal/EmeraldWebTargets.java:186
- The exception wrapping here loses the real cause/message (String.format("{}", ...) produces "{}") and it also swallows InterruptedException without restoring the interrupt flag.
} catch (TimeoutException | ExecutionException | InterruptedException ex) {
throw new EmeraldCommunicationException(String.format("{}", ex.getLocalizedMessage(), ex));
}
bundles/org.openhab.binding.emerald/src/main/java/org/openhab/binding/emerald/internal/EmeraldHWSHandler.java:68
- These locals are assigned from
@Nullablefields but are declared non-null, which breaks the binding's null-annotation contract (and can fail strict null analysis). Mark them@Nullable(or use var with proper annotations).
EmeraldAccountHandler localBridgeHandler = bridgeHandler;
EmeraldHWSConfiguration localConfig = config;
bundles/org.openhab.binding.emerald/src/main/java/org/openhab/binding/emerald/internal/EmeraldAccountHandler.java:197
- Casting child.getHandler() directly to EmeraldHWSHandler can throw ClassCastException (or NPE) if the handler isn't initialized or the child isn't an HWS thing. Use an instanceof check before calling updateFromMqtt.
getThing().getThings().forEach(child -> {
EmeraldHWSHandler handler = (EmeraldHWSHandler) child.getHandler();
if (handler != null) {
EmeraldHWSConfiguration childConfig = child.getConfiguration()
.as(EmeraldHWSConfiguration.class);
if (!childConfig.uuid.isEmpty() && topic.contains(childConfig.uuid)) {
handler.updateFromMqtt(payload);
}
}
});
bundles/org.openhab.binding.emerald/src/main/java/org/openhab/binding/emerald/internal/EmeraldAccountHandler.java:434
- Casting thing.getHandler() directly to EmeraldHWSHandler can throw ClassCastException (or NPE) if the handler isn't initialized or the child isn't an HWS thing. Use an instanceof check before calling updateChannels.
getThing().getThings().forEach(thing -> {
EmeraldHWSHandler handler = (EmeraldHWSHandler) thing.getHandler();
if (handler != null) {
handler.updateChannels();
}
});
bundles/org.openhab.binding.emerald/src/main/java/org/openhab/binding/emerald/internal/EmeraldAccountHandler.java:425
- Logging every discovered heat pump at INFO can be noisy in normal operation (especially if polling is added later). This looks more appropriate at DEBUG.
for (int j = 0; j < localList.info.property[i].heatpump.length; j++) {
logger.info("Found Heat Pump id = {}", localList.info.property[i].heatpump[j].id);
}
bundles/org.openhab.binding.emerald/README.md:15
- This sentence says the bridge needs a "username and email", but the binding config (and table) are email + password. This is confusing for users.
As a minimum, a username and email are needed:
bundles/org.openhab.binding.emerald/README.md:35
- The mode options documented here don't match the channel options defined in thing-types.xml (Boost/Normal/Quiet). Documentation should match the actual supported values.
| mode | Number | Mode the HWS is set to (Normal/Economy/Boost) |
bundles/org.openhab.binding.emerald/README.md:61
- The channel UID examples don't match the Thing IDs used in the example emerald.things snippet (bridge id "account", thing id "hws"). As written, users copying this will get "channel not found" errors.
Switch Emerald_Power { channel="emerald:hws:Emerald:power" }
Number Emerald_Mode { channel="emerald:hws:Emerald:mode" }
Number:Temperature Emerald_Current_Temperature { channel="emerald:hws:Emerald:current-temperature" }
Number:Temperature Emerald_Set_Temperature { channel="emerald:hws:Emerald:set-temperature" }
Number Emerald_Fault { channel="emerald:hws:Emerald:fault" }
Switch Emerald_Defrost { channel="emerald:hws:Emerald:defrost" }
Number Emerald_Work_State { channel="emerald:hws:Emerald:work-state" }
bundles/org.openhab.binding.emerald/src/main/java/org/openhab/binding/emerald/internal/EmeraldWebTargets.java:132
- invokeAws sets the Content-Type header to application/x-amz-json-1.1, but then .content(..., "application/json") will set/override the request content type. For AWS Cognito Identity requests, the content type should stay application/x-amz-json-1.1 or the call may fail.
Request request = httpClient.newRequest(uri).method(HttpMethod.POST)
.header("content-type", "application/x-amz-json-1.1").header("x-amz-target", amzTarget)
.timeout(TIMEOUT_MS, TimeUnit.MILLISECONDS)
.content(new StringContentProvider(payload), "application/json");
bundles/org.openhab.binding.emerald/src/main/java/org/openhab/binding/emerald/internal/EmeraldHWSHandler.java:131
- This local is assigned from a
@Nullablefield but is declared non-null, which breaks the null-annotation contract and can fail strict null analysis.
protected @Nullable EmeraldList getApi() {
EmeraldAccountHandler localBridge = bridgeHandler;
if (localBridge == null) {
return null;
}
bundles/org.openhab.binding.emerald/src/main/java/org/openhab/binding/emerald/internal/EmeraldHWSHandler.java:165
- The API DTOs are parsed from external JSON and can have null sub-objects/arrays (pollData already checks localList.info/property for null). These loops should defensively check api.info and api.info.property to avoid NullPointerExceptions.
EmeraldList api = getApi();
int found = 0;
if (api != null) {
for (int i = 0; i < api.info.property.length; i++) {
for (int j = 0; j < api.info.property[i].heatpump.length; j++) {
if (localConfig.uuid.equals(api.info.property[i].heatpump[j].id)) {
bundles/org.openhab.binding.emerald/src/main/java/org/openhab/binding/emerald/internal/discovery/EmeraldDiscoveryService.java:70
- Same null-safety concern as EmeraldHWSHandler: api.info/property come from external JSON and can be null. Guard before iterating to avoid NPEs during discovery.
private void discover() {
EmeraldList api = getApi();
if (api != null) {
HashMap<String, Object> properties = new HashMap<>();
for (int i = 0; i < api.info.property.length; i++) {
for (int j = 0; j < api.info.property[i].heatpump.length; j++) {
properties.put("uuid", api.info.property[i].heatpump[j].id);
bundles/org.openhab.binding.emerald/src/main/java/org/openhab/binding/emerald/internal/EmeraldHWSHandler.java:215
- Same null-safety issue as initialize(): updateChannels() iterates api.info.property without guarding against null sub-objects/arrays parsed from external JSON, which can cause NPEs.
EmeraldList api = getApi();
if (api != null) {
for (int i = 0; i < api.info.property.length; i++) {
for (int j = 0; j < api.info.property[i].heatpump.length; j++) {
if (localConfig.uuid.equals(api.info.property[i].heatpump[j].id)) {
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
Adds support to openHAB for the range of Emerald Hot Water Systems which are available for sale in Australia and Ne Zealand.