-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[onecta] Initial contribution #16730
base: main
Are you sure you want to change the base?
Conversation
Thanks for your contribution. As there are many PR's currently being reviewed, it might take some time, evantually it will be picked up by one of the maintainers. Can you remove the ecobee files? Add yourself to the codewoner file and perform some kind of self-review with this checklist in mind it will speed up the review process if easy spotted issues can be fixed upfront. Edit: Also the build fails and you might want to check SAT warnings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial comments after half way review. Ping me when you are ready for resuming
...ing.onecta/src/main/java/org/openhab/binding/onecta/internal/api/OnectaConnectionClient.java
Outdated
Show resolved
Hide resolved
...ing.onecta/src/main/java/org/openhab/binding/onecta/internal/api/OnectaConnectionClient.java
Outdated
Show resolved
Hide resolved
...necta/src/main/java/org/openhab/binding/onecta/internal/api/dto/units/FanOperationModes.java
Outdated
Show resolved
Hide resolved
...g.onecta/src/main/java/org/openhab/binding/onecta/internal/api/dto/units/OperationModes.java
Outdated
Show resolved
Hide resolved
...onecta/src/main/java/org/openhab/binding/onecta/internal/api/dto/units/SensoryDataValue.java
Outdated
Show resolved
Hide resolved
Hi @lsiepel, But can you please help me. I'am struggeling with the First commit. This commit is not mine. I tried to delete it, but I don't think it went well. What should I do with this ? Greetings Alexander |
If you refer to DCO, some guidance is here: The builds fail to spotless: That commit that you refer to does not seem to be a problem. |
Thanks than i will ignore it. |
Commits, reviewers and dco are fixed, now it needs to build. Also some comments to look at. |
Signed-off-by: Alexander Drent <Alex@Drent-ict.nl>
Solving naming-convention Signed-off-by: Alexander Drent <Alex@Drent-ict.nl>
Solving naming-convention Signed-off-by: Alexander Drent <Alex@Drent-ict.nl>
Solving naming-convention Signed-off-by: Alexander Drent <Alex@Drent-ict.nl>
Hi fellow developers, Yesterday I ran into the problem that I could no longer build my project locally. [ERROR] Failed to execute goal org.apache.karaf.tooling:karaf-maven-plugin:4.4.5:verify (karaf-feature-verification) on project org.openhab.binding.onecta: Feature resolution failed for [openhab-binding-onecta/4.2.0.SNAPSHOT] To solve this I performed a "Sync Fork". And now, fortunately, it works again locally. My question is, is this the right way? Or is there a better way? Greetings Alexander |
Some changes (karaf update) have been applied to openhab-addons/main and that sometimes breaks other builds. In that case you have to pull the changes from main into your fork and into your feature branch. That is what you did with this commit: Merge branch 'openhab:main' into onecta' So yes this seems perfectly fine! |
- Solving naming-convention - Moved rawdataLogging in Bridge to trace logging - Removed Refreshtoken, openhabhost and stubdatafile from Bridge - refreshUnitsData in OnectaConnectionClient.java refactored Signed-off-by: Alexander Drent <Alex@Drent-ict.nl>
- Add Timeout to HttpClients Signed-off-by: Alexander Drent <Alex@Drent-ict.nl>
- Moved 3 Channels to Properties Signed-off-by: Alexander Drent <Alex@Drent-ict.nl>
- Remove generic exceptions Signed-off-by: Alexander Drent <Alex@Drent-ict.nl>
Hi, I'm in doubt, which is better ?
or
I prefer the one with the catch NullPointerException greetings alexander |
- All If statements with { } - some changes in exceptions Signed-off-by: Alexander Drent <Alex@Drent-ict.nl>
I would propose to use return Optional.ofNullable(getManagementPoint(this.managementPointType))
.map(mp -> mp.getTemperatureControl())
.map(tc -> tc.getValue()
...
.orElse(null); You can also replace the Here are some references: |
- Due to removal of refreshtoken from config property some refactoring needed creating connection to Onecta Signed-off-by: Alexander Drent <Alex@Drent-ict.nl>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2nd part of the review, still not fully covered, but don't want to hold it.
@@ -0,0 +1,18 @@ | |||
done - Refresh token bewaren |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this file from the PR. Also mark it as WIP/DRAFT if not all features are finished. It is also fine to have smallerPR's in thre future, but all breaking changes should be in there now. (like seperating units/watertank)
public static final String CONFIG_PAR_LOGRAWDATA = "rawdataLogging"; | ||
public static final String CONFIG_PAR_STUBDATAFILE = "stubdataFile"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be removed
- use logging (trace) if you need to see the response from the device
- use Unit Tests to upload data files that are supplied to the binding
private @Nullable static HttpClient httpClient = null; | ||
|
||
public void setHttpClientFactory(HttpClientFactory httpClientFactory) { | ||
if (this.httpClientFactory == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would assume that in all cases when this method is called, it should set the httpClientFactory
, so the null check is not needed.
} | ||
|
||
public void setBridgeThing(Thing bridgeThing) { | ||
if (this.bridgeThing == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would assume that in all cases when this method is called, it should set the bridgeThing
, so the null check is not needed.
import org.openhab.core.thing.Thing; | ||
|
||
/** | ||
* The {@link OnectaConfiguration} class contains fields mapping thing configuration parameters. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class does not hold any fields mapping thing configuration parameters.
It seems to have a handle to httpclientfactory etc and that are not configuration parameters.
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, e.getMessage()); | ||
} | ||
|
||
pollingJob = scheduler.scheduleWithFixedDelay(this::pollDevices, 10, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pollingJob = scheduler.scheduleWithFixedDelay(this::pollDevices, 10, | |
pollingJob = scheduler.scheduleWithFixedDelay(this::pollDevices, 0 |
Is there a need to wait 10 seconds to execture the first run?
if (getThing().getStatus().equals(ThingStatus.OFFLINE)) { | ||
try { | ||
logger.debug("Try to restore connection "); | ||
onectaConnectionClient.restoreConnecton(); | ||
|
||
if (onectaConnectionClient.isOnline()) { | ||
updateStatus(ThingStatus.ONLINE); | ||
} | ||
} catch (DaikinCommunicationException e) { | ||
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, | ||
"Try to restore connection. See log for more information. "); | ||
} | ||
} | ||
|
||
if (getThing().getStatus().equals(ThingStatus.ONLINE)) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (getThing().getStatus().equals(ThingStatus.OFFLINE)) { | |
try { | |
logger.debug("Try to restore connection "); | |
onectaConnectionClient.restoreConnecton(); | |
if (onectaConnectionClient.isOnline()) { | |
updateStatus(ThingStatus.ONLINE); | |
} | |
} catch (DaikinCommunicationException e) { | |
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, | |
"Try to restore connection. See log for more information. "); | |
} | |
} | |
if (getThing().getStatus().equals(ThingStatus.ONLINE)) { |
I would not rely on the ThingStatus to determine if the connection has to be restored or not. The handler should not need to have deepr knowledge of the onectaConnectionClient.
The onectaConnectionClient.refreshUnitsData();
method should have some sanity check and throw an exception like DaikinCommunicationException
if there is an unrecoverable issue.
List<Thing> things = getThing().getThings(); | ||
for (Thing t : things) { | ||
// BaseThingHandler handler; | ||
if (t.getStatus().equals(ThingStatus.ONLINE)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (t.getStatus().equals(ThingStatus.ONLINE)) { |
if (getThing().getStatus().equals(ThingStatus.ONLINE)) { | ||
|
||
try { | ||
onectaConnectionClient.refreshUnitsData(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onectaConnectionClient.refreshUnitsData(); | |
onectaConnectionClient.refreshUnitsData(); | |
updateStatus(ThingStatus.ONLINE); |
if (t.getStatus().equals(ThingStatus.ONLINE)) { | ||
|
||
if (t.getThingTypeUID().equals(THING_TYPE_CLIMATECONTROL)) { | ||
OnectaDeviceHandler onectaDeviceHandler = (OnectaDeviceHandler) t.getHandler(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If all these handlers have an shared interface, this code could be simplified by casting the t.getHandler() to that interface and call refreshdevice. as this is the same for all channels.
@@ -132,7 +132,7 @@ public void pollDevicesOnlineTest() throws NoSuchMethodException, InvocationTarg | |||
Method privateMethod = OnectaBridgeHandler.class.getDeclaredMethod("pollDevices"); | |||
privateMethod.setAccessible(true); | |||
|
|||
when(onectaConnectionClientMock.isOnline()).thenReturn(true); | |||
// when(onectaConnectionClientMock.isOnline()).thenReturn(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't commit commented code. Either uncomment it or remove it.
if (!onectaSignInClient.isOnline()) { | ||
onectaSignInClient.signIn(); | ||
} | ||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't commit commented code. Either uncomment it or remove it.
} | ||
} catch (DaikinCommunicationException e) { | ||
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, | ||
"Try to restore connection. See log for more information. "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposal: Trying to restore connection. See log for more information.
<-- note that there is an additional space in your message at the end.
Also, messages displayed on the UI should be externalized to a .properties
file in order to be translatable. Put them into a file located at src/main/resources/OH-INF/i18n/onecta.properties
. Instead of a literal string you can pass @text/my.property.key
here, which will look up my.property.key
in the i18n file.
New Binding for Daikin using Onecta.
With the newer Daikin units it is no longer possible to control them directly (as is done in the other Daikin binding). The units can only be connected to the Daikin cloud called Onecta.
The units can then ‘only’ be controlled with the Onecta app on a phone or tablet.
This binding makes it possible to still control the units with OpenHAB. It’s now done by connecting the binding to Daikin’s Onecta.
The unit information is then received from the Daikin cloud just like the app. Commands to the units also run via the Daikin Cloud.
Older units can also be controlled with this binding as long as they are registered in Onecta.
Description
Please give a few sentences describing the overall goals of the pull request.
Give enough details to make the improvement and changes of the PR understandable
to both developers and tech-savy users.
Please keep the following in mind:
New Binding
I like to have the new binding official added to the openhab lib.
https://community.openhab.org/t/daikin-onecta-cloud-binding-4-0-0-0-5-0-0-0
https://www.openhab.org/docs/developer/guidelines.html
I hope so. this is my first contribution and everything is new.
https://www.openhab.org/docs/developer/bindings/#include-the-binding-in-the-build
https://www.openhab.org/docs/developer/contributing.html#sign-your-work
At first this went wrong so I created a new Fork and copied my code to this. So the first commit is a large one.
Testing
Your pull request will automatically be built and available under the following folder:
https://openhab.jfrog.io/ui/native/libs-pullrequest-local/org/openhab/addons/bundles/
The Jar can be found at:
https://community.openhab.org/t/daikin-onecta-cloud-binding-4-0-0-0-5-0-0-0
Don't forget to submit a thread about your Add-on in the openHAB community:
https://community.openhab.org/c/add-ons
-->