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

[sonnen] Fix channel types, Energy should be Power #15384

Merged
merged 3 commits into from
Nov 3, 2023
Merged

[sonnen] Fix channel types, Energy should be Power #15384

merged 3 commits into from
Nov 3, 2023

Conversation

lochmueller
Copy link
Contributor

Also adapt the documentation and drop the batteryFeedIn in the documentation, which is not part of the binding.

Fix #15365 / #15365

The binding define different things/channels in "Number:Energy" but these are regular "W" entries in the binding. So "Number:Power" is the right channel type. This fix should avoid the warnings in the log, that the binding try to put a "W" into the "Number:Energy" item: Failed to update item 'Sonnen_Battery_Consumption' because '721 W' could not be converted to the item unit 'kWh'

@lochmueller lochmueller requested a review from chingon007 as a code owner August 8, 2023 11:52
chingon007
chingon007 approved these changes Aug 8, 2023
View reviewed changes
Copy link
Contributor

@chingon007 chingon007 left a comment

Choose a reason for hiding this comment

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

Ok, looks fine

@lochmueller
[sonnen] Fix wrong types. Energy should be Power
6637737 
Also adapt the documentation and drop the batteryFeedIn in the documentation
which is not part of the binding.

Fix openhab#15365

Signed-off-by: Tim Lochmüller <tim@fruit-lab.de>
@lochmueller
Copy link
Contributor Author

I added the "Signed-off-by"-thing to my commit. Sorry for the second round... is my first contribution to openHAB.

lsiepel
lsiepel reviewed Aug 8, 2023
View reviewed changes
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

LGTM

Only one comment, an upgrade instruction.xml could be added
https://www.openhab.org/docs/developer/bindings/thing-xml.html#updating-thing-types

@lsiepel lsiepel changed the title [sonnen] Fix wrong types. Energy should be Power [sonnen] Fix channel types, Energy should be Power Aug 8, 2023
jlaur
jlaur requested changes Aug 8, 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 fix!

As @lsiepel mentioned, update instructions are needed to fix the channel item types for managed things.

See the linked documentation, and perhaps a similar example in #15002.

@jlaur jlaur added the bug An unexpected problem or unintended behavior of an add-on label Aug 8, 2023
@lochmueller
[sonnen] Add update instructions for changed channels
c1901da 
Fix openhab#15365

Signed-off-by: Tim Lochmüller <tim@fruit-lab.de>
@lochmueller
Copy link
Contributor Author

I added the instructions. I hope "targetVersion" => 2 is the right one? Regards, Tim

@lochmueller lochmueller requested a review from jlaur August 9, 2023 12:15
@jlaur
Copy link
Contributor

jlaur commented Aug 9, 2023

I added the instructions. I hope "targetVersion" => 2 is the right one?

It seems right, but you also need to update it here:

openhab-addons/bundles/org.openhab.binding.sonnen/src/main/resources/OH-INF/thing/thing-types.xml

Line 35 in 3b9b023

<property name="thingTypeVersion">1</property>

It would be really nice if you could give it a test run in order to avoid regressions. You would need to create a managed Thing with the version prior to your changes. Then uninstall the binding and drop your JAR into the addons directory. The item type for those channels should then automatically change to Number:Power for your existing Thing.

@lochmueller
[sonnen] Add update instructions for changed channels
e3b2a80 
Fix openhab#15365

Signed-off-by: Tim Lochmüller <tim@fruit-lab.de>
lsiepel
lsiepel approved these changes Aug 9, 2023
View reviewed changes
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

LGTM
I leave the rest to the addons maintainers.

@jlaur
Copy link
Contributor

jlaur commented Aug 12, 2023

It would be really nice if you could give it a test run in order to avoid regressions.

Just to be clear: Everything looks fine, I was only waiting for a confirmation of successful migration test. Let me know, and we can merge this PR. 🙂

@jlaur jlaur added the awaiting feedback Awaiting feedback from the pull request author label Oct 7, 2023
@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/sonnen-battery-binding/130488/26

@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/sonnen-battery-binding/130488/32

@lolodomo
Copy link
Contributor

lolodomo commented Nov 3, 2023

@jlaur : I assume noone wants to test the migration ! So maybe you can merge if you think the code looks fine.

@jlaur jlaur merged commit 888f438 into openhab:main Nov 3, 2023
@jlaur jlaur added this to the 4.1 milestone Nov 3, 2023
@jlaur jlaur removed the awaiting feedback Awaiting feedback from the pull request author label Nov 3, 2023
@jlaur
Copy link
Contributor

jlaur commented Nov 3, 2023

I assume noone wants to test the migration ! So maybe you can merge if you think the code looks fine.

All right, let's give it a wider audience. Fingers crossed. 🙂

@chingon007
Copy link
Contributor

I tested it. It works. The change is indeed correct. Please merge

andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Nov 26, 2023
@lochmueller @andrewfg
[sonnen] Fix channel types, Energy should be Power (openhab#15384)
2d5ce5a 
Also adapt the documentation and drop the batteryFeedIn in the documentation
which is not part of the binding.

Fix openhab#15365

Signed-off-by: Tim Lochmüller <tim@fruit-lab.de>
austvik pushed a commit to austvik/openhab-addons that referenced this pull request Mar 27, 2024
@lochmueller @austvik
[sonnen] Fix channel types, Energy should be Power (openhab#15384)
4d8943a 
Also adapt the documentation and drop the batteryFeedIn in the documentation
which is not part of the binding.

Fix openhab#15365

Signed-off-by: Tim Lochmüller <tim@fruit-lab.de>
Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chingon007 chingon007 chingon007 approved these changes

@lsiepel lsiepel lsiepel approved these changes

@jlaur jlaur Awaiting requested review from jlaur

Assignees
No one assigned
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Milestone
4.1
Development

Successfully merging this pull request may close these issues.

[sonnen] batteryFeedIn, batteryCharging, ... description in Watt is not matching type Number:Energy
7 participants
@lochmueller @jlaur @openhab-bot @lolodomo @chingon007 @timbms @lsiepel