-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[nibeheatpump] Added device support for SMO40 control #9376
Conversation
@paulianttila can you take a look? @MarkusGafner can you fix the sign-off in your commit? You could click on the DCO check details below to see what's wrong and how to fix it. |
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.
Just few typos, otherwise LGTM
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.
@paulianttila Thanks for reviewing in advance!
@MarkusGafner Thanks for your contribution! Here comes my review feedback.
bundles/org.openhab.binding.nibeheatpump/src/main/resources/OH-INF/thing/SMO40-types.xml
Outdated
Show resolved
Hide resolved
<item-type>Number</item-type> | ||
<label>External ERS 1 accessory GQ2 speed</label> | ||
<description>Indicates the speed of the GQ2 fan speed on the ERS accessory.</description> | ||
<state pattern="%d %" readOnly="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.
This needs to be escaped. Please check all.
<state pattern="%d %" readOnly="true"> | |
<state pattern="%d %%" readOnly="true"> |
<channel-type id="smo40-48376" advanced="true"> | ||
<item-type>Switch</item-type> | ||
<label>Op mode circ.pump Heat Slave 7</label> | ||
<description></descr |
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.
Are the spaces in the pattern intentional?
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.
The label string is given by the manufacturer
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.
Seems like GitHub mixed up some lines in this review. I was refering to line 8110:
<state min="-10" max="10" step="1" pattern="%d " readOnly="false">
<channel-type id="smo40-48376" advanced="true"> | ||
<item-type>Switch</item-type> | ||
<label>Op mode circ.pump Heat Slave 7</label> | ||
<description></descr |
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.
Is this file generated code? Actually labels are expected to be as short as possible. Guideline is 2-3 words with up to 25 chars. See https://www.openhab.org/docs/developer/bindings/thing-xml.html#formatting-labels-and-descriptions
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.
As written before, the strings are given by the manufacturer and I won't change them.
<description>UDP port to send write commands to the NibeGW.</description> | ||
<default>10000</default> | ||
</parameter> | ||
<parameter name="refreshInterval" type="integer"> |
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.
<parameter name="refreshInterval" type="integer"> | |
<parameter name="refreshInterval" type="integer" min="0" unit="s"> |
|
||
<config-description> | ||
<parameter name="serialPort" type="text" required="true"> | ||
<label>Serial Port</label> |
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.
You could specify <context>serial-port</context>
to get a free validation.
<label>Serial Port</label> | ||
<description>Serial port to connect to the heat pump.</description> | ||
</parameter> | ||
<parameter name="refreshInterval" type="integer"> |
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.
<parameter name="refreshInterval" type="integer"> | |
<parameter name="refreshInterval" type="integer" min="0" unit="s"> |
<default>false</default> | ||
</parameter> | ||
<parameter name="enableWriteCommandsToRegisters" type="text"> | ||
<label>Register List for Write Commands</label> |
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.
See above. Label length.
</channel-groups> | ||
|
||
<config-description> | ||
<parameter name="refreshInterval" type="integer"> |
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.
<parameter name="refreshInterval" type="integer"> | |
<parameter name="refreshInterval" type="integer" min="0" unit="s"> |
Your changes look good! Can you fix the DCO check? You can click on details below to view the necessary commands. |
Signed-off-by: Markus Gafner <[email protected]>
Signed-off-by: Markus Gafner <[email protected]>
Co-authored-by: Fabian Wolter <[email protected]> Signed-off-by: Markus Gafner <[email protected]>
…-INF/thing/SMO40-types.xml Co-authored-by: Fabian Wolter <[email protected]> Signed-off-by: Markus Gafner <[email protected]>
Signed-off-by: Markus Gafner <[email protected]>
Signed-off-by: Markus Gafner <[email protected]>
Signed-off-by: Markus Gafner <[email protected]>
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.
LGTM
As OH3 is already feature-freezed, your PR will be merged as soon as OH3 is released. Your changes will go into OH3.1, then.
Signed-off-by: Markus Gafner <[email protected]>
Signed-off-by: Markus Gafner <[email protected]> Signed-off-by: Joseph Hagberg <[email protected]>
Signed-off-by: Markus Gafner <[email protected]>
Signed-off-by: Markus Gafner <[email protected]>
Added the support of a new heatpump. Therefore all files got created and modified, as described in the repository, to communicate with this device. Tested with the system in the last 2 months without any problems