Skip to content
This repository has been archived by the owner on May 17, 2021. It is now read-only.

[Comfoair] Expanded binding with new commands #5773

Merged
merged 61 commits into from
Feb 22, 2019
Merged

[Comfoair] Expanded binding with new commands #5773

merged 61 commits into from
Feb 22, 2019

Conversation

gieemek
Copy link
Contributor

@gieemek gieemek commented Jan 15, 2019

Expended comfoair binding (originaly written by Holger Hees). Contains many new read and write commands, and gives full control as the original control panels (CC Ease or CC Luxe).

@9037568 9037568 changed the title Expanded comfoair binding [Comfoair] Expanded binding with new commands Jan 17, 2019
9037568
9037568 suggested changes Jan 20, 2019
View reviewed changes
Copy link
Contributor

@9037568 9037568 left a comment

Choose a reason for hiding this comment

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

In addition to addressing the inline comments, please make sure to run all code through the Eclipse formatter.

You've edited the wrong README.md file. Please move your changes from the root file to the file for this binding. I'll review the changes to the README after that...

...hab.binding.comfoair/src/main/java/org/openhab/binding/comfoair/ComfoAirBindingProvider.java Outdated
@@ -1,5 +1,5 @@
/**
* Copyright (c) 2010-2019 by the respective copyright holders.
* Copyright (c) 2010-2018 by the respective copyright holders.
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace all the 2018 values with 2019 throughout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

...hab.binding.comfoair/src/main/java/org/openhab/binding/comfoair/ComfoAirBindingProvider.java Outdated
@@ -21,6 +21,8 @@
*
* @author Holger Hees
* @since 1.3.0
* @author Grzegorz Miasko
* @since 1.14.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete this line. Please apply this change to all files that are not newly added with this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

...hab.binding.comfoair/src/main/java/org/openhab/binding/comfoair/ComfoAirBindingProvider.java Outdated
@@ -21,6 +21,8 @@
*
* @author Holger Hees
* @since 1.3.0
* @author Grzegorz Miasko
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this line below the previous @author line. Please apply this change to all files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this line according to the previous note.

....binding.comfoair/src/main/java/org/openhab/binding/comfoair/datatypes/ComfoAirDataType.java
@@ -1,41 +1,43 @@
/**
* Copyright (c) 2010-2019 by the respective copyright holders.
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert all the changes to this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

...b.binding.comfoair/src/main/java/org/openhab/binding/comfoair/datatypes/DataTypeBoolean.java
@@ -1,47 +1,50 @@
/**
* Copyright (c) 2010-2019 by the respective copyright holders.
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert all the changes to this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I introduced a new code after comments from other comfoair users

....binding.comfoair/src/main/java/org/openhab/binding/comfoair/internal/ComfoAirActivator.java
@@ -1,55 +1,57 @@
/**
* Copyright (c) 2010-2019 by the respective copyright holders.
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert all the changes to this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

...ab.binding.comfoair/src/main/java/org/openhab/binding/comfoair/internal/ComfoAirBinding.java Outdated

}
/**
* Copyright (c) 2010-2018 by the respective copyright holders.
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert to 2019.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed a code after comments from other comfoair users.

...ab.binding.comfoair/src/main/java/org/openhab/binding/comfoair/internal/ComfoAirBinding.java Outdated
* first command
*
* @param command
* @return int values
Copy link
Contributor

Choose a reason for hiding this comment

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

int[]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

...oair/src/main/java/org/openhab/binding/comfoair/internal/ComfoAirGenericBindingProvider.java
@@ -1,132 +1,134 @@
/**
* Copyright (c) 2010-2019 by the respective copyright holders.
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert all the changes to this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

...ng.comfoair/src/main/java/org/openhab/binding/comfoair/internal/InitializationException.java
@@ -1,49 +1,51 @@
/**
* Copyright (c) 2010-2019 by the respective copyright holders.
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert all the changes to this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@9037568
Copy link
Contributor

9037568 commented Jan 28, 2019

Like so:

image

9037568
9037568 suggested changes Jan 28, 2019
View reviewed changes
Copy link
Contributor

@9037568 9037568 left a comment

Choose a reason for hiding this comment

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

I think this should be the last set of changes.

bundles/binding/org.openhab.binding.comfoair/README.md


### Example

Copy link
Contributor

Choose a reason for hiding this comment

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

Headers should be followed by a blank line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

bundles/binding/org.openhab.binding.comfoair/README.md
## Item Configuration

The syntax of the binding configuration strings accepted is the following:
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Beginning code fences should have a blank line preceding them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

bundles/binding/org.openhab.binding.comfoair/README.md
The syntax of the binding configuration strings accepted is the following:
```
comfoair="<device-command>"
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Ending code fences should have a blank line following them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

bundles/binding/org.openhab.binding.comfoair/README.md
```
where `<device-command>` should be replaced with the ComfoAir command from the list below.

### List of commands.
Copy link
Contributor

Choose a reason for hiding this comment

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

Header again.
Also, headers should not end with punctuation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

bundles/binding/org.openhab.binding.comfoair/README.md
| Property | Default | Required | Description |
|----------|---------|:--------:|-------------|
| port | | Yes | Serial port which is connected to the Zehnder ComfoAir system, for example `/dev/ttyS0` on Linux or `COM1` on Windows |
| refresh | 60000 | No | refresh inverval in milliseconds
Copy link
Contributor

Choose a reason for hiding this comment

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

"interval"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

...nding.comfoair/src/main/java/org/openhab/binding/comfoair/datatypes/DataTypeTemperature.java Outdated
*/
public class DataTypeTemperature implements ComfoAirDataType {

private static final Logger logger = LoggerFactory.getLogger(DataTypeTemperature.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Loggers should not be static unless there's a reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

...nhab.binding.comfoair/src/main/java/org/openhab/binding/comfoair/datatypes/DataTypeVolt.java Outdated
*/
public class DataTypeVolt implements ComfoAirDataType {

private static final Logger logger = LoggerFactory.getLogger(DataTypeVolt.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Loggers should not be static unless there's a reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

...b.binding.comfoair/src/main/java/org/openhab/binding/comfoair/datatypes/DataTypeBoolean.java Outdated
*/
public class DataTypeBoolean implements ComfoAirDataType {

private static final Logger logger = LoggerFactory.getLogger(DataTypeBoolean.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Loggers should not be static unless there's a reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

...b.binding.comfoair/src/main/java/org/openhab/binding/comfoair/datatypes/DataTypeMessage.java Outdated
@@ -20,45 +22,53 @@
*/
public class DataTypeMessage implements ComfoAirDataType {

private static final Logger logger = LoggerFactory.getLogger(DataTypeMessage.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Loggers should not be static unless there's a reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

...ab.binding.comfoair/src/main/java/org/openhab/binding/comfoair/datatypes/DataTypeNumber.java Outdated
*/
public class DataTypeNumber implements ComfoAirDataType {

private static final Logger logger = LoggerFactory.getLogger(DataTypeNumber.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Loggers should not be static unless there's a reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@gieemek
Copy link
Contributor Author

gieemek commented Jan 28, 2019

I formatted org.openhab.binding.comfoair as you show above.

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/solved-comfoair-add-filter-reset-in-binding/25337/21

@9037568
Copy link
Contributor

9037568 commented Feb 14, 2019

All the calls to logger.warn in the file ComfoAirConnector.java need to be converted to logger.debug.
The last call to logger.warn in the file ComfoAirBinding.java needs to be converted to logger.debug.

All calls to logger.error need to be examined and converted to another type. Use the lowest appropriate one.

@gieemek
Copy link
Contributor Author

gieemek commented Feb 15, 2019

Done.
I changed some logger.debug to logger.trace too.

@9037568
Copy link
Contributor

9037568 commented Feb 16, 2019

One last item. Need to fix the null pointer exception defect reported in this community thread.

I'd recommend also reviewing Eclipse warnings to see if it's flagging any other possible occurrences...

2019-01-24 16:34:51.992 [vent.ItemStateChangedEvent] - comfoairFilterPeriod changed from 16 to 15
2019-01-24 16:34:51.979 [WARN ] [org.apache.karaf.services.eventadmin] - EventAdmin: Exception during event dispatch [org.osgi.service.event.Event [topic=openhab/command/comfoairFilterPeriod] {item=comfoairFilterPeriod, bridgemarker=true, command=15, timestamp=1548344091974} | {org.osgi.service.cm.ManagedService, org.osgi.service.event.EventHandler}={service.id=367, service.bundleid=227, service.scope=bundle, event.topics=openhab/*, service.pid=org.openhab.comfoair, component.name=org.openhab.binding.comfoair, component.id=224} | Bundle(org.openhab.binding.comfoair_1.13.0 [227])]
java.lang.NullPointerException: null
	at org.openhab.binding.comfoair.handling.ComfoAirCommandType.getChangeCommand(ComfoAirCommandType.java:393) ~[?:?]

@gieemek
Copy link
Contributor Author

gieemek commented Feb 18, 2019

Need to fix the null pointer exception defect reported in this community thread.

Done.

I'd recommend also reviewing Eclipse warnings to see if it's flagging any other possible occurrences...

I checked the Eclipse warnings and the only ones reported were for the MANIFEST.MF file.
But warnings are reported to such files (MANIFEST.MF) in all bindings.

@9037568
Copy link
Contributor

9037568 commented Feb 22, 2019

Thanks, @gieemek !

@9037568 9037568 merged commit 592989e into openhab:master Feb 22, 2019
@9037568 9037568 added this to the 1.14.0 milestone Feb 22, 2019
@BoBiene BoBiene mentioned this pull request Aug 27, 2019
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@9037568 9037568 9037568 requested changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
1.14.0
Development

Successfully merging this pull request may close these issues.
3 participants
@gieemek @9037568 @openhab-bot