[WIP] [fronius] Binding restructuring: channel naming, deprecations, battery control schedules - #21220
[WIP] [fronius] Binding restructuring: channel naming, deprecations, battery control schedules#21220joni1993 wants to merge 11 commits into
Conversation
|
I tested the config API validation on my GEN24 (firmware 1.41.10-1) to answer open question 1:
So the API itself validates nothing, the validation only exists in the web UI client. When I opened the battery management page afterwards, the three rules were shown in red and could not be saved, with these messages:
Conclusion: the binding has to validate on its own. My proposal for the limit channels: refuse the write when entries of that schedule type exist that are not the all-time entry managed by the channel, and log a hint to use the actions instead. Otherwise a channel write could silently produce a table that the web UI flags as invalid. I would also let the addSchedule action log a warning when the new entry overlaps or contradicts existing entries, since it can create exactly the red rules shown above. |
Very nice! |
|
Found two more config API quirks while testing the validation changes on my GEN24 (firmware 1.41.10-1).
var LT = Java.type('java.time.LocalTime');
var QT = Java.type('org.openhab.core.library.types.QuantityType');
var b = actions.thingActions('fronius', 'fronius:battery:XXXXXXXX:battery');
console.info('Add 1: ' + b.addSchedule(LT.of(10, 0), LT.of(11, 0), 'CHARGE_MAX', new QT('300 W')));
console.info('Add 2: ' + b.addSchedule(LT.of(12, 0), LT.of(13, 0), 'CHARGE_MAX', new QT('400 W')));
console.info('Add 3: ' + b.addSchedule(LT.of(14, 0), LT.of(15, 0), 'CHARGE_MAX', new QT('500 W')));
console.info('Add 4: ' + b.addSchedule(LT.of(16, 0), LT.of(17, 0), 'CHARGE_MAX', new QT('600 W')));
Mitigated in cd24948, 4043131 and b64ce80: the config API client now checks the HTTP status and retries gateway errors, and setTimeOfUse reads the settings back until they match the written ones, so a subsequent read-modify-write is guaranteed to be based on the new settings and a lost write fails visibly instead of silently. With these changes, four back-to-back adds land reliably on my system. I deliberately did not add an unconditional delay between config API requests: the happy path costs one extra read, and waiting only happens as retry backoff when the inverter actually reports an error or still returns the previous settings. Opinions welcome, also on whether a small fixed delay would be worth it on top. |
|
@joni1993 Please rebase. |
As discussed in the review: since this PR already introduces breaking changes, the channel ids of all things are aligned with the coding guidelines (kebab-case) in the same go. - Released channels keep their old ids as deprecated channels: the old channel types are marked advanced and labelled DEPRECATED, so unlinked deprecated channels are hidden, while existing links keep working. The handlers serve the deprecated ids with the value of their replacement, and a warning is logged on initialization for each linked deprecated channel to point users to the new id. - Channels that have not been released yet (added by this PR) are renamed directly, without a deprecated counterpart. - The channel types of the new ids follow the naming conventions as well; the old types remain for the deprecated channels. Types of channels whose id was already conformant (e.g. the battery soc) are left untouched to avoid changing stored things. - Update instructions add the new channels to existing things (powerinverter v3, battery v2, meter v1, ohmpilot v1). - The old ohmpilot statecode channel is replaced by status-code (not state-code) for consistency with the powerinverter, as both use the same channel type. Signed-off-by: Christian Jonak-Möchel <christian@jonak.org>
Adds min-charge-power, max-charge-power, min-discharge-power and max-discharge-power channels to the battery thing, showing the limit currently in effect: the first active time of use entry (in table order, matching the priority shown in the inverter web UI) whose weekday and time range match the current time determines the value; without a matching entry the channel is UNDEF. Writing such a channel replaces all time of use entries of its schedule type with a single all-time entry, preserving the entries of the other schedule types - unlike the reset based simple actions, so that the four channels do not erase each other. Also adds a getTimeOfUseSchedules action returning the whole time of use table for use in scripts. Signed-off-by: Christian Jonak-Möchel <christian@jonak.org>
…ot provide them GEN24/Tauro/Verto inverters report null for the day and year energy. Gson maps the JSON null to 0 for the primitive value field of ValueUnit, so the channels showed 0 kWh, which reads as a real measurement. The value field is now nullable with a hasValue() accessor, and the day and year energy channels report NULL when no value is provided. Signed-off-by: Christian Jonak-Möchel <christian@jonak.org>
…e exist The config API accepts overlapping and even contradicting time of use entries, only the inverter web UI validates them and then refuses to save the settings at all. Writing a limit channel therefore must not silently replace existing schedules of its type: the write is now refused with a hint to the battery control actions, unless the only entry of the type is the all-time entry managed by the channel itself. The check runs on the table read the write needs anyway, no state is cached. Concurrent changes through the inverter web UI can still race the check, as the config API has no transactions. Signed-off-by: Christian Jonak-Möchel <christian@jonak.org>
…tries The config API accepts overlapping and contradicting time of use entries, but the inverter web UI then marks the rules as invalid and refuses to save the settings. The addSchedule action now logs a warning when the new entry overlaps an entry of the same type, or contradicts an overlapping entry of the opposite min/max type, so users notice before opening the web UI. Signed-off-by: Christian Jonak-Möchel <christian@jonak.org>
The framework's auto-update predicted the new state from the command, so a refused write still showed the commanded value until the next refresh cycle. With the veto auto-update policy the state only changes when the binding updates it after a successful write, so a refused write keeps the previous value. Signed-off-by: Christian Jonak-Möchel <christian@jonak.org>
…I rules Verified against the inverter web UI (GEN24, firmware 1.41.10-1): entries that only touch (one ends exactly when the other starts) are valid, charge and discharge entries must not overlap unless both are maximum power limits, and the all-time limit channels can conflict with entries of other schedule types, so the channel write path now warns about conflicts too. Signed-off-by: Christian Jonak-Möchel <christian@jonak.org>
…use limit When multiple entries of the same schedule type are active at once (the inverter web UI rejects such tables, but the config API accepts them), the inverter applies the most restrictive entry, not the first one in the table: measured on a GEN24 (firmware 1.41.10-1) with two all-time DISCHARGE_MAX entries of 100 W and 2000 W, the battery discharged at ~100 W regardless of the entry order. Signed-off-by: Christian Jonak-Möchel <christian@jonak.org>
…arsed Signed-off-by: Christian Jonak-Möchel <christian@jonak.org>
…gateway errors The inverter answers with 502 Bad Gateway when config API requests arrive in quick succession, which previously surfaced as a JSON parse error. Signed-off-by: Christian Jonak-Möchel <christian@jonak.org>
Reads shortly after a write can return the previous settings, so a subsequent read-modify-write could silently undo the write. Signed-off-by: Christian Jonak-Möchel <christian@jonak.org>
5fbff52 to
65a99ab
Compare
| @@ -23,37 +23,232 @@ | |||
| </update-channel> | |||
| </instruction-set> | |||
| <instruction-set targetVersion="2"> | |||
There was a problem hiding this comment.
'old' instruction sets should not be changed unless they contain a typo/bug. and from what i see that is not the case here.
Note
Rebased onto main after #21193 was merged.
Description
Follow-up to #21193, as agreed in #21193 (comment): align the channel ids of the whole binding with the coding guidelines, without breaking existing setups.
1. Rename all channels to kebab-case, keep the old ids as deprecated channels
Released channels keep their old ids as deprecated channels: the old channel types are marked advanced and labelled DEPRECATED, existing links keep working, and a warning pointing to the new id is logged for each linked deprecated channel. Channels introduced by #21193 are renamed directly since they were never released. Update instructions add the new channels to existing things (powerinverter v3, battery v2, meter v1, ohmpilot v1).
Full rename table
powerinverter
battery
meter
ohmpilot
min-charge-power,max-charge-power,min-discharge-powerandmax-discharge-poweron the battery thing show the limit currently in effect: the most restrictive active time of use entry matching the current weekday and time (verified on the device, the inverter applies the most restrictive entry, not the first one), UNDEF otherwise. The table is read within the existing battery settings refresh cycle, reusing its session. Writing sets an all-time entry and is refused while schedules of that type exist, since replacing them would destroy the user's schedules. AgetTimeOfUseSchedules()action returns the table for scripts.3. Validation and config API robustness
The config API accepts overlapping and contradicting time of use entries, only the web UI validates them (see the comments below for the tested rules). The binding now warns when a new entry conflicts with existing ones: same-type overlap, min/max contradiction within charging or discharging, and charge/discharge overlap unless both are maximum limits. Entries that only touch do not count as overlapping. The limit channels use the veto auto-update policy so a refused write does not show the commanded value.
Testing also surfaced two config API quirks (details in the comments): reads shortly after a write can return the previous settings, which made quick successive addSchedule calls silently lose entries, and the inverter answers with 502 when requests arrive in quick succession. The client now checks the HTTP status and retries gateway errors, and written time of use settings are read back for verification.
4. Bugfix
day-energyandyear-energyreport NULL instead of 0 kWh when the inverter does not provide them (GEN24/Tauro/Verto).Open questions
Write semantics of the limit channelsAnswered below: the API accepts everything, only the web UI validates. Writing is refused while schedules of that type exist, and conflict warnings follow the web UI rules.batterySocof thesocchannel) keep their old ids to avoid modifying stored things. OK, or rename them via update-channel instructions?powerflowinverter1power/powerflowinverter1socchannels be removed? Maybe together with this rename.