[tibber] Rework binding (channel groups, UoM, Thing actions and more) - #18726
Conversation
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
|
@lsiepel I believe @kjoglum is no longer with us, I haven't heard anything from him in several months although I tried to contact him. Can you please reassign this review to someone else, yourself maybe? |
There was a problem hiding this comment.
Pull Request Overview
This PR reworks the Tibber binding for OpenHAB 5, consolidating channels into three groups, updating the spot-price logic, and adding comprehensive test coverage and migration instructions.
- Introduces three channel groups (
price,live,statistics) and updates thing-types and channel-types to match the OH5 style guide. - Adds new test resources and unit tests for spot prices, price schedules, laundry-curve parsing, and action APIs.
- Updates migration instructions (
instructions.xml) to remove old channels and add new ones, and replaces therefreshparameter withupdateHour.
Reviewed Changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/resources/OH-INF/update/instructions.xml | Updated migration instructions for OH5, removing old and adding new channels. |
| src/main/resources/OH-INF/thing/thing-types.xml | Removed legacy channels, defined channel groups, bumped thingTypeVersion to 3, added updateHour config. |
| src/main/resources/OH-INF/thing/price-group.xml | Defined the price channel group with new channel IDs. |
| src/main/resources/OH-INF/thing/live-group.xml | Defined the live channel group and its channels for pulse data. |
| src/main/resources/OH-INF/thing/statistics-group.xml | Defined the statistics channel group for energy/production stats. |
| src/test/resources/curve.json | Test resource for laundry-curve conversion (contains a JSON syntax issue). |
Comments suppressed due to low confidence (2)
bundles/org.openhab.binding.tibber/src/main/resources/OH-INF/thing/price-group.xml:10
- Channel ID
spot-pricedoes not match the migration instructions which addspot-prices. Please unify the ID names for consistency.
<channel id="spot-price" typeId="price">
bundles/org.openhab.binding.tibber/src/test/resources/curve.json:2
- Invalid JSON: an object literal cannot start with a bare array. Either remove the outer
{}or add a property name before the array to form valid JSON.
[
jlaur
left a comment
There was a problem hiding this comment.
Thank you for this major refactoring! I have had a look and posted some comments.
Since this is a breaking change removing all channels and providing new ones, I'm wondering if should target 5.0 or 5.1. It would have been good to have it included in some milestone releases. How much has the community already been involved in testing?
| logger.debug("WebSocket connected to Server"); | ||
| // String subScriptionMessage = String.format(handler.getTemplate(WEBSOCKET_SUBSCRIPTION_RESOURCE_PATH), | ||
| // config.homeid); | ||
| String subScriptionMessage = String.format(SUBSCRIPTION_MESSAGE, config.homeid); |
There was a problem hiding this comment.
Is there any reason to subscribe to events for unlinked channels? Consider populating a DTO and serializing this into the subscribe message.
There was a problem hiding this comment.
You're right. Can be done very granular.
To be honest this takes now too much effort
- check every time which channels are connected
- are channels added or removed
- unsubscribe then subribe again
- are channels added or removed
To be honest I cannot provide this in short timeframe
There was a problem hiding this comment.
This can be refined further later, let's leave it like this for now.
| if (!elem.isJsonNull()) { | ||
| return elem.getAsString(); | ||
| } else { | ||
| return NULL; |
There was a problem hiding this comment.
Wouldn't it be better to actually return null so that the string "null" doesn't become some reserved value?
There was a problem hiding this comment.
Renamed it to NULL_VALUE to avoid confusion with java null
There was a problem hiding this comment.
What I meant was the returning the literal string "null" as a reserved value prevents the actual string "null" to be normally returned. The method could return null in this case to be handled specially.
There was a problem hiding this comment.
We can come back to this in a follow-up PR.
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
|
@lsiepel - can you verify my latest commits? |
| public static final String SCHEDULE_CONTAINER = "{\"size\":%s, \"cost\":%s, \"schedule\":%s}"; | ||
| public static final String QUERY_CONTAINER = "{\"query\":\"%s\"}"; | ||
| public static final String WEBSOCKET_URL_QUERY = "{\"query\":\"{viewer {websocketSubscriptionUrl }}\"}"; | ||
| public static final String CONNECT_MESSAGE = "{\"type\":\"connection_init\", \"payload\":{\"token\":\"%s\"}}"; | ||
| public static final String DISCONNECT_MESSAGE = "{\"type\":\"connection_terminate\",\"payload\":null}"; | ||
| public static final String SUBSCRIPTION_MESSAGE = "{\"id\":\"1\",\"type\":\"subscribe\",\"payload\":{\"variables\":{},\"extensions\":{},\"operationName\":null,\"query\":\"subscription { liveMeasurement(homeId:\\\"%s\\\") { timestamp power lastMeterConsumption lastMeterProduction accumulatedConsumption accumulatedConsumptionLastHour accumulatedCost currency minPower averagePower maxPower voltagePhase1 voltagePhase2 voltagePhase3 currentL1 currentL2 currentL3 powerProduction accumulatedProduction accumulatedProductionLastHour minPowerProduction maxPowerProduction }}\"}}"; |
There was a problem hiding this comment.
For future refactoring it would be a step forward to create proper DTO's for these
|
|
||
| @Override | ||
| public String toString() { | ||
| return "{\"power\":" + powerWatts + ",\"duration\":" + durationSeconds + "}"; |
|
|
||
| @Override | ||
| public String toString() { | ||
| return "{\"price\":" + price + ",\"duration\":" + durationSeconds + ",\"level\":" + level + ",\"startsAt\":\"" |
I have created openhab/openhab-distro#1766 |
|
Ok looked at the comments that are left, i merge this now. Quick fixes usually tend (let me speak for myself) to create regressions. So if there are any post merge findings a new PR can be created easily. Thanks @weymann for your efforts on this refactoring! |
|
@weymann - thank you for the major rework in the binding! 👍 |
…openhab#18726) * use common httpclient Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
…openhab#18726) * use common httpclient Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com> Signed-off-by: Paul Smedley <paul@smedley.id.au>
…openhab#18726) * use common httpclient Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com> Signed-off-by: Scott Hanson <scooter_seh@yahoo.com>
…openhab#18726) * use common httpclient Signed-off-by: Bernd Weymann <bernd.weymann@gmail.com>
It's WIP but I tested now for 2 weeks and it's published on Marketplace. Please start review!On first hand PR shall solve issue #18506. Seeing all these
nullfields in the binding it's hard to determine which thread is settiing which variable to null so I decided for a rework.Rework Rationale
spot-pricesobtained once per day respecting retry machanismspot-priceswereNumber:Dimensionless- now tit's changed toNumber:EnergyPriceprice levelpolled every minute.price levelis a human readable String . Now it's a Number machine readable with< 0cheap and> 0expensive.price levelnow avaialble asTimeSerieslive_Rework contains:
updateHour- hour of day when prices shall be updated. Retries are performed accoring to Tibber API requirements.pricegroupspot-pricescurrent and forecast prices asNumber:EnergyPricelevelcurrent and forecast price levels asNumberwith 0=Normal, negative values cheaper, positve values expensiveaveragecurrent and forecast of 24h average prices asNumber:EnergyPricelivegroup for Tibber Pulse power, voltage and current valuesstatisticsgroup for Tibber Pulse energy valuesThingActionsadded for price calculations