[knx] New modifier to set mainGA write-only #16042
Conversation
Co-authored-by: Florian Hotze <florianh_dev@icloud.com> Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>
|
@florian-h05 I have taken over the javadoc improvement from #16003 and listed you as co-author. |
J-N-K
left a comment
There was a problem hiding this comment.
In general LGTM, just some comments.
| Each configuration parameter has a `mainGA` where commands are written to and optionally several `listeningGA`s. | ||
|
|
||
| The `dpt` element is optional. If omitted, the corresponding default value will be used (see the channel descriptions above). | ||
| The element `dpt` is optional. If omitted, the corresponding default value will be used (see the channel descriptions above). |
There was a problem hiding this comment.
I wonder if we should change that. A lot of issues especially with DPT9 would not have occurred if the DPT was mandatory. I would prefer to remove the "optional" brackets around the DPT and change this sentence as suggested.
| The element `dpt` is optional. If omitted, the corresponding default value will be used (see the channel descriptions above). | |
| The element `dpt` is highly recommended and change to a mandatory element in future versions. | |
| If omitted, the corresponding default value will be used (see the channel descriptions above). |
The code can be like it is now. In the next iteration (i.e. 4.2) we could print a warning for a missing DPT and remove support for "default" DPTs earliest in 4.3.
|
|
||
| private static final Pattern PATTERN_GA_CONFIGURATION = Pattern.compile( | ||
| "^((?<dpt>[1-9][0-9]{0,2}\\.[0-9]{3,5}):)?(?<read><)?(?<mainGA>[0-9]{1,5}(/[0-9]{1,4}){0,2})(?<listenGAs>(\\+(<?[0-9]{1,5}(/[0-9]{1,4}){0,2}))*)$"); | ||
| "^((?<dpt>[1-9][0-9]{0,2}\\.[0-9]{3,5}):)?(?<modifier>[<>])?(?<mainGA>[0-9]{1,5}(/[0-9]{1,4}){0,2})(?<listenGAs>(\\+(<?[0-9]{1,5}(/[0-9]{1,4}){0,2}))*)$"); |
There was a problem hiding this comment.
This matches either < or > but not both (>< or <>). Do you think there is a use-case for having a non-listening GA that is read from the bus?
There was a problem hiding this comment.
It is intended. I can't imagine a use case where I want to read at startup but ignore the response at the same time.
|
|
||
| The optional `<` sign tells whether the group address of the datapoint accepts read requests on the KNX bus (it does, if the sign is there). | ||
| All group addresses marked with `<` are read by openHAB during startup. | ||
| Group addresses marked with `<` are read by openHAB during startup, but typically this is useful for only one group address per definition. |
There was a problem hiding this comment.
| Group addresses marked with `<` are read by openHAB during startup, but typically this is useful for only one group address per definition. | |
| The group addresses marked with `<` are read by openHAB during startup. | |
| Future versions might support reading from one GA only. |
Or can you imagine a setup where reading several addresses for one state would be correct? The result would be non-deterministic, because the final state would depend on the device with the slowest response.
There was a problem hiding this comment.
No, I can't. The patch already introduces a warning on info level. The question is how to notify users who use the GUI.
There was a problem hiding this comment.
I agree here.
The question is how to notify users who use the GUI.
That's a good question in many cases ... I can imagine to use the log WebSocket to register to the logger of the add-on of the Thing you currently edit and show some alert in the UI if a warning or error message is received over the WebSocket. But this is just a general idea for the future ... in the mean time, I would say, it's best to have a look at your logs when changing configuration. (I always watch my logs when changing configuration.)
Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>
florian-h05
left a comment
There was a problem hiding this comment.
In general LGTM, just two minor comments.
...ng.knx/src/main/java/org/openhab/binding/knx/internal/channel/GroupAddressConfiguration.java
Show resolved
Hide resolved
|
@holgerfriedrich And do you mind also bringing d245572 to this PR? |
Co-authored-by: Florian Hotze <florianh_dev@icloud.com> Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
|
Many thanks for working on this together @holgerfriedrich and @J-N-K!! |
* [knx] New modifier to set mainGA write-only Co-authored-by: Florian Hotze <florianh_dev@icloud.com> Signed-off-by: Holger Friedrich <mail@holger-friedrich.de> Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
* [knx] New modifier to set mainGA write-only Co-authored-by: Florian Hotze <florianh_dev@icloud.com> Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>
Group address notation of the KNX binding currently uses a mainGA used for writing and reading from the bus, and additional listenGAs to listen to further addresses, e.g. status addresses.
https://www.openhab.org/addons/bindings/knx/#group-address-notation
mainGA is always listening, which is misleading or even wrong in certain situations, e.g. whenever commands are sent form device on the bus and the corresponding actor is locked and does not execute the command. In those cases OH should relay only on the listenGAs, not the mainGA.
This PR introduces a new optional modifier ">" to turn the mainGA write-only.
Alternative implementation to #16003, which maintains compatibility with existing installations.