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.

Sign up for GitHub

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

Jump to bottom

[omnikinverter] Add extra parameters (temperature, AC frequency, AC current, AC voltage and total hours running) #14244

Merged
merged 20 commits into from
Feb 2, 2023
Merged

[omnikinverter] Add extra parameters (temperature, AC frequency, AC current, AC voltage and total hours running) #14244

merged 20 commits into from
Feb 2, 2023

Conversation

leansoft
Copy link
Contributor

@leansoft leansoft commented Jan 18, 2023 •
edited
Loading

Added temperature sensor to the Omnikinvert addon:

  • [omnikinverter] Added temperature sensor information
  • [omnikinverter] Added extra AC information (frequency, voltage and current)
  • [omnikinverter] Added the total hours of generating power.
  • [omnikinverter] Fixed channel-types to prevent issue when adding items

Description

This PR adds an improvement and bugfix to the omnikinverter.

@leansoft
[omnikinverter] Added temprature sensor
a1ea5f7 
Signed-off-by: Dave <leansoft@users.noreply.github.com>
@leansoft
[Omnikinverter] Bugfix for wrong channeltype item-type
1abbcec 
Prevents 'Received HTTP PUT request with an invalid item name' when adding items
Signed-off-by: Dave <leansoft@users.noreply.github.com>
jlaur
jlaur requested changes Jan 18, 2023
View reviewed changes
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

Thanks for the channel and UoM fixes as well. I have provided some minor feedback mostly about typos.

.../src/main/java/org/openhab/binding/omnikinverter/internal/OmnikInverterBindingConstants.java Outdated Show resolved Hide resolved
bundles/org.openhab.binding.omnikinverter/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.omnikinverter/README.md Outdated Show resolved Hide resolved
...kinverter/src/main/java/org/openhab/binding/omnikinverter/internal/OmnikInverterMessage.java Outdated Show resolved Hide resolved
bundles/org.openhab.binding.omnikinverter/src/main/resources/OH-INF/thing/thing-types.xml Outdated Show resolved Hide resolved
bundles/org.openhab.binding.omnikinverter/src/main/resources/OH-INF/thing/thing-types.xml Outdated Show resolved Hide resolved
...kinverter/src/main/java/org/openhab/binding/omnikinverter/internal/OmnikInverterMessage.java Show resolved Hide resolved
@jlaur jlaur added the enhancement An enhancement or new feature for an existing add-on label Jan 18, 2023
leansoft and others added 8 commits January 19, 2023 09:53
@leansoft @jlaur
Update bundles/org.openhab.binding.omnikinverter/src/main/java/org/op…
a67dbea 
…enhab/binding/omnikinverter/internal/OmnikInverterBindingConstants.java


Signed-off-by: Dave <leansoft@users.noreply.github.com>

Co-authored-by: Jacob Laursen <jacob-github@vindvejr.dk>
@leansoft @jlaur
Update bundles/org.openhab.binding.omnikinverter/README.md
1ac65fb 
Signed-off-by: Dave <leansoft@users.noreply.github.com>

Co-authored-by: Jacob Laursen <jacob-github@vindvejr.dk>
@leansoft @jlaur
Update bundles/org.openhab.binding.omnikinverter/src/main/java/org/op…
f5e4567 
…enhab/binding/omnikinverter/internal/OmnikInverterMessage.java


Signed-off-by: Dave <leansoft@users.noreply.github.com>

Co-authored-by: Jacob Laursen <jacob-github@vindvejr.dk>
@leansoft @jlaur
Update bundles/org.openhab.binding.omnikinverter/src/main/resources/O…
a7a0249 
…H-INF/thing/thing-types.xml


Signed-off-by: Dave <leansoft@users.noreply.github.com>

Co-authored-by: Jacob Laursen <jacob-github@vindvejr.dk>
@leansoft @jlaur
Update bundles/org.openhab.binding.omnikinverter/src/main/resources/O…
9a37972 
…H-INF/thing/thing-types.xml


Signed-off-by: Dave <leansoft@users.noreply.github.com>

Co-authored-by: Jacob Laursen <jacob-github@vindvejr.dk>
@leansoft
[omnikinverter] README.md alignment fix
4dd77b4 
openhab#14244 (comment)
Signed-off-by: Dave <leansoft@users.noreply.github.com>
@leansoft
[omnikinverter] Fixed typo
8b99c49 
Signed-off-by: Dave <leansoft@users.noreply.github.com>
@leansoft
[omnikinverter] Added test coverage for temperature
b6b422a 
Signed-off-by: Dave <leansoft@users.noreply.github.com>
leansoft and others added 3 commits January 19, 2023 13:41
@leansoft
[omnikinverter] Added extra data for AC and hours running
a834fda 
[omnikinverter] Added extra data for AC and hours running (#1)
Signed-off-by: Dave <leansoft@users.noreply.github.com>
@leansoft @jlaur
Update bundles/org.openhab.binding.omnikinverter/src/main/resources/O…
ebb6178 
…H-INF/thing/thing-types.xml


Signed-off-by: Dave <leansoft@users.noreply.github.com>

Co-authored-by: Jacob Laursen <jacob-github@vindvejr.dk>
@leansoft @jlaur
Update bundles/org.openhab.binding.omnikinverter/src/main/resources/O…
f30f30f 
…H-INF/thing/thing-types.xml


Signed-off-by: Dave <leansoft@users.noreply.github.com>

Co-authored-by: Jacob Laursen <jacob-github@vindvejr.dk>
@leansoft leansoft changed the title [omnikinverter] added temperature of sensor [omnikinverter] added extra parameters (temprature, AC frequency, current and voltage and total hours running) Jan 19, 2023
@leansoft leansoft changed the title [omnikinverter] added extra parameters (temprature, AC frequency, current and voltage and total hours running) [omnikinverter] added extra parameters (temperature, AC frequency, AC current, AC voltage and total hours running) Jan 19, 2023
@leansoft leansoft requested a review from jlaur January 19, 2023 12:50
@leansoft
[omnikinverter] Updated readme for new parameters
d5d80a0 
Signed-off-by: Dave <leansoft@users.noreply.github.com>
@leansoft
[omnikinverter] Updated things-types to use System State Channel Types
108ec4c 
Signed-off-by: Dave <leansoft@users.noreply.github.com>
@leansoft
[omnikinverter] Formatting
9e08e04 
Removed newline
Signed-off-by: Dave <leansoft@users.noreply.github.com>
@leansoft leansoft requested a review from jlaur January 19, 2023 13:18
jlaur
jlaur requested changes Jan 20, 2023
View reviewed changes
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

A few additional comments for new changes. Please also regenerate the I18N properties file - see https://www.openhab.org/docs/developer/utils/i18n.html#generating-i18n-properties-file

bundles/org.openhab.binding.omnikinverter/src/main/resources/OH-INF/thing/thing-types.xml Outdated Show resolved Hide resolved
bundles/org.openhab.binding.omnikinverter/src/main/resources/OH-INF/thing/thing-types.xml Outdated Show resolved Hide resolved
bundles/org.openhab.binding.omnikinverter/src/main/resources/OH-INF/thing/thing-types.xml Outdated Show resolved Hide resolved
bundles/org.openhab.binding.omnikinverter/src/main/resources/OH-INF/thing/thing-types.xml Outdated
<label>Instantaneous Power</label>
<description>The instantaneous power generation for output 1</description>
</channel>
<channel id="powerAC1" typeId="system.electric-power">
Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually a breaking change until openhab/openhab-core#2590 has been resolved, unfortunately. Even though the channel type is fully compatible, I believe this could be a problem for existing things configured in the UI.

You would either need to revert the channel type changes for existing channels or add a note here:
/~https://github.com/openhab/openhab-distro/blob/84f0e7d06e23befbb7a9bebe36783fc5f1665f18/distributions/openhab/src/main/resources/bin/update.lst#L95-L99

IMHO your refactoring/simplification makes sense, so either solution is fine by me.

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 Reverted this in f25b9b2
I also build it against 3.4.2 with the XML and everything works.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean that changing the underlying channel type for an existing channel is a breaking change (when the previous channel type id no longer exists). I'm fine with it, but it needs to be mentioned in the openhab-distro notes that users may have to re-create their things. You should be able to reproduce it if you go back to the previous version before your changes and create an item for one of these old channels - in the UI, so it will be managed. Then upgrade to this version.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unresolving this for visibility until PR towards openhab-distro has been created. @leansoft, please remember about this.

bundles/org.openhab.binding.omnikinverter/src/main/resources/OH-INF/thing/thing-types.xml Outdated Show resolved Hide resolved
@leansoft
[omnikinverter] Changes as requested bij PR (#2)
f25b9b2 
* [Omnik] Added AC Voltage, Currrent and Frequency

Signed-off-by: Dave <leansoft@users.noreply.github.com>

* [Omnik] Added AC Voltage, Currrent and Frequency and total running hours

Signed-off-by: Dave <leansoft@users.noreply.github.com>

* [omnikinverter] Fixes for wrong method names

Signed-off-by: Dave <leansoft@users.noreply.github.com>

* [omnikinverter] Adjusted test to omnik test output

Signed-off-by: Dave <leansoft@users.noreply.github.com>

* [omnikinverter] XML syntax fix

Signed-off-by: Dave <leansoft@users.noreply.github.com>

* [omnikconverter] Reverted channel changes

Signed-off-by: Dave <leansoft@users.noreply.github.com>

* [omnikconverter] Reverted channel to things-types to prevent backward issues

Signed-off-by: Dave <leansoft@users.noreply.github.com>

* [omnikconverter] Regenerated Generating I18N properties file

As requested in PR
Signed-off-by: Dave <leansoft@users.noreply.github.com>

Signed-off-by: Dave <leansoft@users.noreply.github.com>
@leansoft leansoft requested a review from jlaur January 24, 2023 13:12
jlaur
jlaur requested changes Jan 27, 2023
View reviewed changes
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

One more small iteration. Sorry for being absent the last few days.

...es/org.openhab.binding.omnikinverter/src/main/resources/OH-INF/i18n/omnikinverter.properties Outdated Show resolved Hide resolved
bundles/org.openhab.binding.omnikinverter/README.md Outdated Show resolved Hide resolved
...es/org.openhab.binding.omnikinverter/src/main/resources/OH-INF/i18n/omnikinverter.properties Outdated Show resolved Hide resolved
jlaur
jlaur approved these changes Feb 2, 2023
View reviewed changes
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

LGTM

@jlaur jlaur merged commit baf077d into openhab:main Feb 2, 2023
@jlaur jlaur added this to the 4.0 milestone Feb 2, 2023
@jlaur jlaur changed the title [omnikinverter] added extra parameters (temperature, AC frequency, AC current, AC voltage and total hours running) [omnikinverter] Add extra parameters (temperature, AC frequency, AC current, AC voltage and total hours running) Feb 2, 2023
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Feb 28, 2023
@leansoft @nemerdaud
[omnikinverter] added extra parameters (temperature, AC frequency, AC…
5059d1a 
… current, AC voltage and total hours running) (openhab#14244)

* [omnikinverter] Added temperature sensor
* [Omnikinverter] Bugfix for wrong channeltype item-type

Prevents 'Received HTTP PUT request with an invalid item name' when adding items

* [Omnik] Added AC Voltage, Currrent and Frequency and total running hours

Signed-off-by: Dave <leansoft@users.noreply.github.com>
renescherer pushed a commit to renescherer/openhab-addons that referenced this pull request Mar 23, 2023
@leansoft @renescherer
[omnikinverter] added extra parameters (temperature, AC frequency, AC…
99ac28c 
… current, AC voltage and total hours running) (openhab#14244)

* [omnikinverter] Added temperature sensor
* [Omnikinverter] Bugfix for wrong channeltype item-type

Prevents 'Received HTTP PUT request with an invalid item name' when adding items

* [Omnik] Added AC Voltage, Currrent and Frequency and total running hours

Signed-off-by: Dave <leansoft@users.noreply.github.com>
FordPrfkt pushed a commit to FordPrfkt/openhab-addons that referenced this pull request Apr 20, 2023
@leansoft @FordPrfkt
[omnikinverter] added extra parameters (temperature, AC frequency, AC…
a421250 
… current, AC voltage and total hours running) (openhab#14244)

* [omnikinverter] Added temperature sensor
* [Omnikinverter] Bugfix for wrong channeltype item-type

Prevents 'Received HTTP PUT request with an invalid item name' when adding items

* [Omnik] Added AC Voltage, Currrent and Frequency and total running hours

Signed-off-by: Dave <leansoft@users.noreply.github.com>
@lolodomo lolodomo mentioned this pull request May 5, 2023
Closed
29 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlaur jlaur jlaur approved these changes

Assignees
No one assigned
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
2 participants
@leansoft @jlaur