[panamaxfurman] Initial contribution #12895
Conversation
…BOLT-CV1 or BlueBOLT-CV2 interface card installed Signed-off-by: Dave Badia <dbadia@gmail.com>
There was a problem hiding this comment.
Thanks for the PR!
As i'm a beginner reviewer do not take this as a full review. Some other reviewer will probably pick this up for a more extensive review.
I noticed the logging is not always been used as described in: https://www.openhab.org/docs/developer/guidelines.html#f-logging. There is also alot of inline comment.
There are also some SAT errors that need attention.
|
|
||
| The Telnet interface was chosen for implementation due to the following: | ||
|
|
||
| * Telnet interface supports "push" notifications from the device which provide instant and more reliable status information as opposed to repetitive polling of state as required by the HTTP interface |
There was a problem hiding this comment.
Please put the 'feature' part if this text to the top of this file to the summary of this binding. It is not needed to explain to end-users about who this or that protocol is used. That is more a technical perspective.
| ## Notes | ||
|
|
||
| The Panamax/Furamn Power Conditioner devices actually support 3 different protocols: | ||
| | Interface | Supported? | Interface Card | |
|
|
||
| ### `telnet` Thing Configuration | ||
|
|
||
| | Name | Type | Description | Default | Required | Advanced | |
|
|
||
| ## Channels | ||
|
|
||
| | Channel | Type | Read/Write | Description | |
|
|
||
| ## Thing Configuration | ||
|
|
||
| ### `telnet` Thing Configuration |
There was a problem hiding this comment.
I don't think telnet is a good name for this thing. If you ever want to switch to a different protocol (RS-232 or HTTP) this name no longer fits. I would sugggest to change it to something like 'conditioner' or whatever suits the best.
| wasLastBrand = false; | ||
| // assume this is the model | ||
| if (data.trim().split(" ").length > 1) { | ||
| logger.warn("wasLastBrand was true but model had multiple parts: {}", data); |
| * @author Dave Badia - Initial contribution | ||
| * | ||
| */ | ||
| // Can't use @NonNullByDefault here, causes compilation errors |
There was a problem hiding this comment.
Maybe some more experienced java developer can help you out here?
| @@ -0,0 +1,24 @@ | |||
| binding.panamaxfurman.name = PanamaxFurman binding | |||
| binding.panamaxfurman.description = Binding for Panamax/Furman power conditioners which have the BlueBOLT-CV1 or BlueBOLT-CV2 interface card installed. | |||
There was a problem hiding this comment.
Please use the mvn command to add default translations.
some of this file are filled, some not.
| </channel-group-type> | ||
|
|
||
| <channel-group-type id="outletControlsTelnet"> | ||
| <label>Outlet Controls Telenet</label> |
| public class PanamaxFurmanOutletGenerateAndParseTest { | ||
|
|
||
| @ParameterizedTest | ||
| @CsvSource(value = { "1,outlet1#power", "null, blah" }, nullValues = { "null" }) |
| |---------|--------|------------|-----------------------------| | ||
| | Brand | String | R | Brand of the power conditioner| | ||
| | Model | String | R | Model of the power conditioner| | ||
| | Firmware Version | String | R | Firmware version of the power conditioner| |
There was a problem hiding this comment.
Since Brand, Model and Firmware Version are just static metadata (rather than say a voltage measurement) that will not be useful in rules, these do not need to be a channel. They can be a Thing Property instead. Here is an example of how Thing Properties look for another binding when displaying similar data:
| <parent> | ||
| <groupId>org.openhab.addons.bundles</groupId> | ||
| <artifactId>org.openhab.addons.reactor.bundles</artifactId> | ||
| <version>3.3.0-SNAPSHOT</version> |
There was a problem hiding this comment.
| <version>3.3.0-SNAPSHOT</version> | |
| <version>4.2.0-SNAPSHOT</version> |
| @@ -0,0 +1,10 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
This needs to be adapted to the new addon.xml, the PR to adapt the developer docs is not merged yet, so if you need more details, the corresponding issue is here: openhab/openhab-core#2058
But you can also update your branch and look at another binding's addon.xml and adapt it to this initial contribution.
|
I've moved this work to a new branch on my fork. I'm going to close this PR and open a new one after I make some updates based on this feedback |
New binding for Panamax/Furman power conditioners which have the BlueBOLT-CV1 or BlueBOLT-CV2 interface card installed
Precompiled binary available here
https://github.com/dbadia/openhab-panamaxfurman-compiled