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.

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

[tibber] Add channel for lastMeterProduction #14583

Merged
merged 1 commit into from
Mar 14, 2023
Merged

Conversation

kjoglum
Copy link
Contributor

@kjoglum kjoglum commented Mar 11, 2023

Adding requested channel for lastMeterProduction, part of live measurements.

@kjoglum kjoglum added the enhancement An enhancement or new feature for an existing add-on label Mar 11, 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/tibber-binding/67019/214

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.

If you can fix the space, otherwise LGTM

bundles/org.openhab.binding.tibber/README.md Outdated Show resolved Hide resolved
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! LGTM. Besides @lsiepel's comment, please also add an update instruction for the new channel. See for example #14572. This will ensure that the channel is automatically added for managed things when upgrading to the new version without a need to recreate thing.

@kjoglum kjoglum force-pushed the tibber-3.0 branch 5 times, most recently from 47142b5 to 63511b7 Compare March 11, 2023 22:52
@kjoglum kjoglum requested a review from jlaur March 11, 2023 22:53
Comment on lines 8 to 10
<add-channel id="tomorrow_prices">
<type>tibber:tomorrow_prices</type>
</add-channel>
Copy link
Contributor

Choose a reason for hiding this comment

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

This channel was added already in 3.4: #13416. I'm not sure what will happen when adding channels that already exists.

If nothing bad will happen, in principle one could just add all channels here to "play it safe". If a user would upgrade straight from 3.3 to 4.0, the instruction above would actually help.

But in this case we wouldn't need the update instructions for "add-channel" at all (conceptually), since they could be added/updated from the thing definitions, and check/upgrade simply triggered when thingTypeVersion is bumped.

@J-N-K - I could use some input here. The next question that will come up will be whether to bump thingTypeVersion per release, milestone or 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.

Being unaware of the update feature, I added the latest channels added believing to achieve what you describes wrt different version upgrades. Will update accordingly based on @J-N-K’s answer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going away for some days, so made the update instructions applicable only for the lastMeterProduction channel.

Copy link
Member

Choose a reason for hiding this comment

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

Don't add channels that could already be present, calling .withChannel on a ThingBuilder will result in an IllegalArgumentException when the channel is already present. You can use update, because removing a non-existing channel does not result in an error.

Signed-off-by: kjoglum <stiankj@online.no>
@kjoglum kjoglum requested a review from jlaur March 14, 2023 05:23
@jlaur jlaur mentioned this pull request Mar 14, 2023
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 5963246 into openhab:main Mar 14, 2023
@jlaur jlaur added this to the 4.0 milestone Mar 14, 2023
renescherer pushed a commit to renescherer/openhab-addons that referenced this pull request Mar 23, 2023
Signed-off-by: kjoglum <stiankj@online.no>
leifbladt pushed a commit to leifbladt/openhab-addons that referenced this pull request Apr 14, 2023
Signed-off-by: kjoglum <stiankj@online.no>
FordPrfkt pushed a commit to FordPrfkt/openhab-addons that referenced this pull request Apr 20, 2023
Signed-off-by: kjoglum <stiankj@online.no>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants