[knx] Refactoring of KnxCoreTypeMapper and UOM Support

[holgerfriedrich]

This is a merge of smarthome/j code to current code base.
It will include several PRs from smarthome/j related to UOM and refactoring of KnxCoreTypeMapper.

Carryover from
smarthomej/addons#107
smarthomej/addons#114
smarthomej/addons#203
smarthomej/addons#206
smarthomej/addons#208
smarthomej/addons#219
smarthomej/addons#226
smarthomej/addons#230
smarthomej/addons#279
smarthomej/addons#299
smarthomej/addons#330
smarthomej/addons#349

Also-by: Jan N. Klug [email protected]

Refs: #14502

[J-N-K]

I didn't check in detail if everything is ok, just a comment while scrolling though the code. But I did not see anything obviously wrong.

[holgerfriedrich]

@J-N-K I am just integrating smarthomej/addons/#208 which requires ColorUtil. How should I proceed with it? For now add it to the KNX binding, unchanged or stripped to a minimum?

[jlaur]

@J-N-K I am just integrating smarthomej/addons/#208 which requires ColorUtil. How should I proceed with it? For now add it to the KNX binding, unchanged or stripped to a minimum?

See also comment about ColorUtil here: #14124 (comment)

[holgerfriedrich]

Thanks, @jlaur. It looks like HSBType from core could cover 2 of the 3 occurrences (though the implementation might be different), but it is lacking support for xyY color format used by DPT_Colour_xyY 242.600.

[jlaur]

It looks like HSBType from core could cover 2 of the 3 occurrences (though the implementation might be different), but it is lacking support for xyY color format used by DPT_Colour_xyY 242.600.

I didn't analyze it further since I was busy with something else at the time, so just took note. Maybe it would be worth adding the missing format to the core HSBType implementation? My initial thought was to compare unit test coverage in the two implementations and possibly detect any differences by new coverage, or have proof that the implementations indeed produce same results.

[J-N-K]

They don‘t provide the same results, otherwise I wouldn’t have implemented it in another way :-)

I would suggest to adjust core to the implementation on SmartHome/J (including the tests).

[J-N-K]

Regarding color: see openhab/openhab-core#3434

[holgerfriedrich]

@J-N-K this PR has become quite huge, as it includes the complete restructuring of KNXCoreTypeMapper and the introduction of UoM. Functionally, it seems complete. The other carryover tasks can be handled separately.

I have kept the work split into several commits to match the changes in smarthome/j and added additional commits to fix the unit tests for UoM we had added in this repo only.

One open point is HSB color transform with (x,y,Y) - this is currently failing until we have ColorUtil in the core.

I did not have time yet to check it with my KNX installation.

[jlaur]

@holgerfriedrich - LGTM except for #14534 (comment). FYI, @kaikreuzer has reviewed and merged openhab/openhab-core#3434 less than an hour ago, so can be completed now. 🙂

[holgerfriedrich]

@jlaur I just pushed then new version which uses ColorUtil. I think it will only build after the next core snapshot is built tomorrow morning.

[jlaur]

LGTM

[holgerfriedrich]

Ok, then let's wait for tomorrow. I will rebuild.

Regarding the squash merge of this PR, please take care of the references to the original PRs, please keep @J-N-K in the joint sign-off.
I thought of a rebase commit (as long as we had ~10 commits from the carryover PRs). but with the additional intermediate commits it is too much for the history and probably should be squashed during merge. The single commits can still be found in this ticket.

[florian-h05]

@holgerfriedrich & @J-N-K
Huge thanks for your work on the KNX binding, so far works great on my production system. I have checked my (few) -control channels, they still work.

One thing that I noted was that I had a few DPTs wrongly set in my knx.things file, I corrected them and the log said Loading model knx.things (as usual), but it seems that the thing was not updated. If I check inside MainUI, the old DPT is still displayed.

I am not sure whether that‘s a core or a KNX binding problem, but when I change my yamaha.things file, the Yamaha devices go OFFLINE and then ONLINE, my KNX devices don‘t (and they did in the past).

[J-N-K]

Interesting that this error is back, I'm pretty sure I fixed that one. I'll have a look but I have some other things I'm working on before.

[florian-h05]

@J-N-K Any progress regarding this issue?

[J-N-K]

Totally forgot about it. Do you still see it with latest snapshot? There was an issue with the file watcher some time ago that could have caused that problem, but it's fixed now.

[florian-h05]

Unfortunately, this issue persists. The file change was noticed all the time and is still noticed ([el.core.internal.ModelRepositoryImpl] - Loading model 'knx.things'), but the KNX devices/Things don't update.

I am not also encountering another weird behaviour: my -control channels have stopped working, the handleCommand method of DeviceThingHandler is not called for control channels. I have just checked, with SNAPSHOT 3390 from 31th of March, they work.

[J-N-K]

I don't normally use things files. I tried and it works for me when I change the channel-name, but not the configuration or the channel label. Can you confirm that this is specific to the KNX binding? I'm asking because when I only change the label, the updated method in the thing registry is not called and that should affect all bindings, not only KNX.

[florian-h05]

You are right, changing channel name works, but neither channel label nor configuration change work for KNX and MQTT, so it must be unspecific to the KNX binding. And it seems that you already found the cause.

[florian-h05]

Stays just the other problem: My -control channels have stopped working, they worked with this PR. I have just checked, with SNAPSHOT 3390 from 31.03.2023 5AM they work. So this PR can‘t be the cause, but there were two PRs since then. I will try to find out which of those caused that regression.

[J-N-K]

I have no idea what causes this behavior. But it’s a core issue, not an addons-issue. Please open a bug report there.