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

[kermi] Initial contribution #15687

Closed
wants to merge 19 commits into from
Closed

Conversation

col-panic
Copy link

@col-panic col-panic commented Oct 2, 2023

Kermi Binding

  • [kermi] Initial contribution - read-only values

Description

This binding connects to Kermi x-center controller for heat pumps.

@col-panic col-panic marked this pull request as ready for review October 2, 2023 15:46
@col-panic col-panic requested a review from a team as a code owner October 2, 2023 15:46
@florian-h05 florian-h05 changed the title #15680 Contribute kermi heat-pump manager binding [kermi] Initial contribution Oct 3, 2023
@florian-h05 florian-h05 added the new binding If someone has started to work on a binding. For a new binding PR. label Oct 3, 2023
Signed-off-by: Marco Descher <marco@descher.at>
Signed-off-by: Marco Descher <marco@descher.at>
Signed-off-by: Marco Descher <marco@descher.at>
Signed-off-by: Marco Descher <marco@descher.at>
@jlaur
Copy link
Contributor

jlaur commented Oct 17, 2023

@col-panic - thanks for your contribution! Until this PR will picked up for review, you could already have a look at the checkstyle warnings. See target/code-analysis/report.html.

@col-panic
Copy link
Author

I fixed some of those. Some of them don't make sense to me. Why is lang3.Stringutils provided, but should not be used?

@jlaur
Copy link
Contributor

jlaur commented Nov 11, 2023

Why is lang3.Stringutils provided, but should not be used?

See #7722.

Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

Partly review, hope this is usefull to you. As I'm no maintaner consider this just as suggestions and to speed up the proces.


* a franchised version of the Kermi heatpump, namely the [Heizbösch MOZART13AC-RW60](https://www.boesch.at/produkte/heizen/waermepumpe/luft/modulierende-luft-wasser-waermepumpe-mozart-aussenaufstellung~495589) heatpump manager version _1.6.0.118_ .

No official documentation could be found or gathered. This plug-in is based
Copy link
Contributor

Choose a reason for hiding this comment

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

Every sentence has to be on its own line. Please adjust all in this file.

Number:Power Current_Power_Inverter {channel="kermi:heatpump:heatpumpbridge:heatpump:Rubin_CurrentPowerInverter"}
```

# Changelog
Copy link
Contributor

Choose a reason for hiding this comment

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

changelog is not needed as git is used for release notes.

* Support string values for datapointType = 3
* Support string values for datapointType = 4

# ToDo / Future Tasks
Copy link
Contributor

Choose a reason for hiding this comment

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

please implement or create github issues for each todo item.

```
// kermi.items
Number:Temperature Drinking_water_temperature {channel="kermi:drinkingwater-heating:heatpumpbridge:dwheating:BufferSystem_TweTemperatureActual"}

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change


Number:Temperature Heating_current_temperature_buffer {channel="kermi:room-heating:heatpumpbridge:rheating:BufferSystem_HeatingTemperatureActual"}
Number:Temperature Cooling_current_temperature_buffer {channel="kermi:room-heating:heatpumpbridge:rheating:BufferSystem_CoolingTemperatureActual"}

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

@SuppressWarnings("null")
public synchronized String executeUrl(String httpMethod, String url, String content, String contentType)
throws KermiCommunicationException {
if (StringUtils.isBlank(hostname)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (StringUtils.isBlank(hostname)) {
if (hostname == null || hostname.isBlank()) {

}

private String getBaseApiUrl() {
return "http://" + hostname + "/api/";
Copy link
Contributor

Choose a reason for hiding this comment

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

Not 100% sure, but maybe you can re-use something from the binding constants class as this api url creation /parsing looks like duplicate functionality.

? new ByteArrayInputStream(content.getBytes(StandardCharsets.UTF_8))
: null;
result = HttpUtil.executeUrl(httpMethod, url, httpHeaders, inputStream, contentType, 5000);
logger.debug("[{} {}] {}", httpMethod, url, result);
Copy link
Contributor

Choose a reason for hiding this comment

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

when logging extensive details as results/body etc it is advised to use trace level.

if (iex != null && iex.getCause() instanceof HttpResponseException) {
HttpResponseException hre = (HttpResponseException) iex.getCause();
if (401 == hre.getResponse().getStatus()) {
logger.debug("Perform login");
Copy link
Contributor

Choose a reason for hiding this comment

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

better to move this debugging inside the performlogin method itself as that method is responsible for such a task.

}

logger.debug("HTTP error on attempt #{} {}", attemptCount, url);
Thread.sleep(500 * attemptCount);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure you want to block the thread? why not use the scheduler? If this code is called on binding initilization, this is a no go. (i can check, but have to go now)

Co-authored-by: Wouter Born <github@maindrain.net>
Signed-off-by: Marco Descher <marco@descher.at>
@lsiepel
Copy link
Contributor

lsiepel commented Dec 29, 2023

@col-panic are you able to proceed with the first comments and fix the build? If not, let me know and see what we can do to get this fixed.

@col-panic
Copy link
Author

Hey @lsiepel feel free to perform changes. I've been using the code for a while for myself. At the moment I do not see a time slot to continue! It would be best, however, if someone could co-test the code if he has the same device.

@col-panic
Copy link
Author

As of the current info, this PR will be closed in the future in favour of #16329

@lsiepel lsiepel closed this Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new binding If someone has started to work on a binding. For a new binding PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants