[wip] Allow configuration parameter updates to be suppressed

[andrewfg]

Note: quick and dirty proof of concept; further refinement required; not tested..

Resolves #3753

This PR adds an xml configuration parameter attribute dynamic to suppress the parameter from being dynamically updated by discovery services.

Signed-off-by: Andrew Fiddian-Green software@whitebear.ch

[Nadahar]

Quoting myself from #3753:

This has been discussed in various places and I don't remember them all, but I believe to remember that actually very few bindings benefit from this - for the rest it's more of a bug. So, if a setting is introduced, I believe it should be false by default and instead activated for those (few) that benefits from it.

But, there's more. I'm working on overhauling the Chromecast binding (it hasn't moved for a while now because of other things), and it has such a need. Chromecasts will actually create "virtual devices" with random ports when you create groups, and if one of the devices in the group go offline, the "group device" might change both its IP and port. To keep track of this group device, you must use updated information from mDNS. But, you also have cases where (real) devices must be configured by IP that isn't overridden.

I've concluded that the only way to manage this properly is for the binding itself to subscribe to mDNS announcements. The "update functionality" from discovery is too primitive and won't allow you to differentiate between different situations. My plan has been to let the user configure either a unique ID for the device or the IP address. If the unique ID is used, updates are applied, if the IP is used, updates are not applied (because it switching to the IPv6 address can also make things stop working).

My question is, if a similar situation exists for other bindings that use this, that it will never actually work properly/fully using the discovery mechanism. In short, will the discovery mechanism ever be "desirable" for all use cases for a particular binding?

I'm also thinking that perhaps it should be a per-Thing setting instead of a binding property. I'm pretty sure that even in the bindings where it is "used as a feature", it can also act as a bug. But, I guess there is no way to make a Thing setting that applies to all Things (with discovery)?

[andrewfg]

I'm also thinking that perhaps it should be a per-Thing setting instead of a binding property

In this PR it is neither "per binding", nor "per Thing", nor indeed a "setting". Instead it is per xml configuration parameter definition per a given thing-type xml.

EDIT: so just for the avoidance of doubt: the addon author can define in their thing-type xml exactly which configuration parameters of which thing-types shall be subject to automatic updating by discovery services and which not. So with a particular binding you may have some thing types which have some config params which are (say) not subject to automatic updating. See link => LINE 331

[Nadahar]

In this PR it is neither "per binding", nor "per Thing", nor indeed a "setting". Instead it is per xml configuration parameter definition per a given thing-type xml.

EDIT: so just for the avoidance of doubt: the addon author can define in their thing-type xml exactly which configuration parameters of which thing-types shall be subject to automatic updating by discovery services and which not. So with a particular binding you may have some thing types which have some config params which are (say) not subject to automatic updating.

It's what I meant by "per binding". The point is that the add-on author must decide, while for example in the case with the Chromecast, there are situations where both variants are desired. So, I was saying that perhaps it would make more sense as a user setting somehow - per Thing, not Thing-type. But, it might not be easy to do.

[andrewfg]

perhaps it would make more sense as a user setting somehow - per Thing, not Thing-type

Probably we need both.

per Thing

And not per Thing .. it needs finer granularity -- per config param of a thing (-type)

[Nadahar]

And not per Thing .. it needs finer granularity -- per config param of a thing (-type)

What I meant was "per Thing instance" - so that you could e.g. have one Chromecast that updates and one that doesn't - depending on if it's a dynamically created group or a device with a fixed (DHCP assigned) IP.

I'm just not sure if there is any mechanism to do it on that level? We can't add this as a configuration parameter to all Things..?

[andrewfg]

depending on if it's a dynamically created group

Currently I am thinking only of the static thing-types. I have not (yet) thought about dynamically created things based on dynamically created thing-types. But let me think about it..

[Nadahar]

Currently I am thinking only of the static thing-types. I have not (yet) thought about dynamically created things based on dynamically created thing-types. But let me think about it..

That wasn't what I meant, but that's also a case to think about. With Chromecasts (and I bet other devices too), the devices themselves can create "dynamic devices" (that are announced via mDNS) in addition to the physical devices. Those need to be dynamically updated as their IP and port will change when the Chromecasts "feel like it" (they have "elections" over who is the "leader" of a group, and when the group leader changes, so does the IP and port of the group).

[andrewfg]

Those need to be dynamically updated as their IP and port will change

So the current behaviour works fine for these already?

.. this PR is focussing on suppressing the current behaviour when it is specifically NOT required.

[Nadahar]

So the current behaviour works fine for these already?

.. this PR is focussing on suppressing the current behaviour when it is specifically NOT required.

No, which is my point, what is a "feature" for one device, can be a "bug" for another, even for the same type of device.

In the case of Chromecasts, you can't control what the devices announce via mDNS. They just love IPv6 for example, and I've seen examples of "feature" leading to the configured IPv4 address being replaced by a IPv6 address. The problem is that the IPv6 address isn't actually "in use", the network only uses IPv4. So, automatically "updating" the IP to the IPv6 address results in losing connection with the device.

So, for the very same type of device, it can be desirable and not be. Which is what I'm trying to say. This isn't something the binding developer can predict, it depends on conditions on the local network. Which is why the only way to get it to work "correctly" for everybody, is to give the user the option to control it.

[andrewfg]

So are you saying that there is no worth in this PR that allows developers to suppress auto update on certain configuration parameters on certain thing types? And that the only solution that should be pursued is your hoped for user based manual suppression? Personally I think that this PR is a worthwhile step forward. And also, like you, I am not sure if your solution is even actually possible.

[Nadahar]

So are you saying that there is no worth in this PR that allows developers to suppress auto update on certain configuration parameters on certain thing types? And that the only solution that should be pursued is your hoped for user based manual suppression? Personally I think that this PR is a worthwhile step forward. And also, like you, I am not sure if your solution is even actually possible.

I'm saying that I can only see it working "for everybody" if the users can decide. How to get there, I'm not sure. I still think that there might be no way around the bindings handling it themselves to make it work optimally. As such, this PR could be a useful tool, since bindings that do implement this "properly" could then turn if off for its thing-types.

In the end, whether the user want configuration settings to be updated automatically based on "some service" depends quite a lot on the design of their local network - and to some extent on the behavior of the devices. I know that it doesn't exclusively relate to network configuration, like with the mentioned Astro location (but why would that move?), but by and large, I think it applies to IP and port.

[mherwege]

@andrewfg I am not sure I see the point in this PR. If a binding author doesn't want the parameters to update, he can have a shadow copy of the properties he does not want updated. The first time the thing is initialized, these will be empty. The binding then copies over values. Automatic updates will not impact these copied values. So there is a workaround. Admitted, your logic could simplify it.
The default logic is to indeed copy parameters and properties from the re-discovered thing into the existing thing. And that is for good reason and simplies a lot in many cases. For one, my home automation system stays connected if the network goes down, restarts and gives my home automation hub an new IP. I know Homeassistant needs to be reconfigured if that happens. But as @Nadahar states, it may create issues in very specific cases. And in these cases, it should be a user option to disable it.

There should be a configuration parameter that allows to disable background discovery on a binding level (it was there in the past). It is not exposed in the UI to my knowledge. But I think what you would want is a thing configuration parameter that toggles automatic updates, not background discovery. I don't see an issue with create such a parameter. But it is a bit of work:

1. extend the thing XML
2. extend the thing fields
3. extend the DTO's and REST API's
4. extend the UI to show the parameter on the thing configuration page
   Not impossible, but some work.

[Nadahar]

The binding then copies over values. Automatic updates will not impact these copied values. So there is a workaround. Admitted, your logic could simplify it.

I must say that I think having all (affected) bindings implement such a workaround seems a bit optimistic. I get that it works well for you, it does not for me. I always control my network, so I have reserved fixed IPs for devices that I want to reach in DHCP. They will thus always come back with the same IPv4 address. IPv6 is actively disabled everywhere possible, and isn't routed, because of the security implications of using it. But, not everything will let you disable IPv6, some will "self assign" one and then announce that IP. When the configuration "auto updates" they become unreachable.

I'm sure there are other situations where problems arise as well. I'd say that when you manually configure a device, you don't expect the configuration values to be overwritten. So, while I see the "convenience" of being able to track devices that are assigned new IPs, I think the implementation is shady. It should have been something you'd actively enable, not something that just does what it does without your permission or knowledge.

There should be a configuration parameter that allows to disable background discovery on a binding level (it was there in the past).

Nobody says that you don't want background discovery if you don't want the configuration to be updated. Those are completely separate issues. Discovery finds devices for you, and puts them in your inbox. You then manually choose if you want to make those discoveries into Things. From that point on, they should be unafilliated with discovery as I see it. There's no reason for a user to assume that "new discovery results" will interfere and change the Things you have already configured.

[andrewfg]

But .. it may create issues in very specific cases. And in these cases, it should be a user option to disable it.

@mherwege I appreciate your inputs. But .. ..

The place where I discovered this issue is in the HomeKit binding, where the mDNS discovery (due to a bug that @Nadahar has found in OH Core) may "discover" a thing before the mDNS ServiceInfo has actually been fully qualified. That means that the discovery service would wrongly update the Thing's Config Param to have an IP address of "null". Thus causing the Thing wrongly to go offline.

Also there was another similar issue where the mDNS discovery might wrongly update the Thing's Config Param to have an ipV6 address, which since the HomeKit protocol only works on ipV4, also causes the Thing wrongly to go offline.

My initial thought, (and hence the reason for this PR), was that I could most easily suppress such unwanted shenanigans via a switch in the Thing's config xml description. (But in the meantime I solved it another way via code in the mDNS discovery participant instead).

---

I think what you would want is a thing configuration parameter that toggles automatic updates
But it is a bit of work:

1. extend the thing XML
2. extend the thing fields
3. extend the DTO's and REST API's
4. extend the UI to show the parameter on the thing configuration page

Not impossible, but some work.

Indeed. And I think that this PR is indeed actually the work for 1., 2., and 3. that you cited above. Or?? .. So the "only" additional work is item 4. above .. which I would anyway defer to you ;) ..

[holgerfriedrich]

I very much appreciate that you picked up this topic!

I had it on my to-do list since end of last year when we ran into issues that autodetect reconfigured existing things in KNX binding (basically breaking those things which needed a manual config change after the initial detection).

What I actually like to have for KNX is to have this setting per parameter.

Let me give some context:
KNX bridge things can be either a ROUTER or a TUNNEL. Routers typically expose both and it it up to the user to select one. On top, it both ROUTER and TUNNEL can be configured without or with security. Users need to manually change from ROUTER to SECUREROUTER and TUNNEL to SECURETUNNEL after detection. The corresponding config field should never be touched after the initial detection. But for new devices in the Inbox, I would like to set the initial value according to the detection.

Other fields of the config should be updated. The things have a unique serial, so we can identify the hardware and update other configuration parameters like the IP address if it changes over time. This is already happening automatically if I use the serial as unique identification and add the IP during detection.

The automatic update is already happening in the background - the binding developer IMHO cannot do much to avoid it besides not adding the respective config parameters.

Someone - I thinks it was Kai - made the proposal to extend the xml, as you just did. But maybe we need to change this from thing to parameter level.

[andrewfg]

What I actually like to have for KNX is to have this setting per parameter.

That is actually what the current code does (to be strict is per parameter instance).

[mherwege]

@holgerfriedrich I believe @andrewfg proposal is on parameter level. The challenge I see is with the UI. That’s a much more challenging enhancement then doing it on thing level. The UI then has to have a switch for every parameter to turn this on/off for every possible type of parameter.

That’s I believe where I disagree with @andrewfg (I preferred thing level, not thing config parameter). But as I understand it for the KNX case you would also want that.

@andrewfg I think where we are not in line is that your dynamic parameter is static in the thing type. It is not controlled by a UI flag you can switch on/off per individual thing config parameter. The binding developer fixes it. Controlling this in the UI for each parameter I think is making it far heavier than needed. And also think about controlling this flag in textual thing definitions then.
I am also not convinced static should become the default. That is a breaking change and my result in things not working as expected anymore for those relying on it. And it would require updates to all bindings that have this as acceptable behavior, with automatic migrations to be implemented.

@holgerfriedrich In the binding, can’t you use a shadow configuration parameter that, once set, overrules the main discovered one in the binding logic?

[andrewfg]

we are not in line is that your dynamic parameter is static in the thing type

Sort of. I imagine it could be read only on unmanaged things, but perhaps modifiable on managed ones. Or ??

But indeed the basis of your concern is that we are talking essentially about a "configuration parameter for a configuration parameter". In thing type xml we do already configure such config params by applying an xml attribute on the <parameter> xml element .. for example the type or required attributes..

<config-description>
    <parameter name="baseURL" type="text" required="true" dynamic="false">
        <label>Base URL</label>
	<description>The URL set here can be extended in the channel configuration.</description>
	<context>url</context>
    </parameter>
<config-description>

EDIT however I think there is not really a problem here. When the Thing is initialized the dynamic="false" can be read from the xml into the respective DTO, and then into the java configuration parameter class instance itself. And from the UI we should be able to access that and modify it again via the DTO. So in other words the xml can pre-set it, but the UI can override that preset. Or??

[Nadahar]

I must just repeat what I have said previously. I don't think there is a general solution that fits every use case here. Different bindings have different challenges. Many bindings aren't even affected, does this apply to bindings that don't use mDNS or UPnP discovery at all? Perhaps SDDP as well, but there are a limited number of ways IPs and ports can be "detected". I think those are the two parameters really in play here - I know that the "location" in Astro has been used as an example as well, but I frankly think that people can manage to update this themselves every time they buy a new house.

So, when narrowing it down to the bindings and parameters where it's actually relevant, it might not be that many. For some bindings it's clearly undesirable, in other situations it will depend on the user's network setup. The exact way it is applied will probably have different nuances in different bindings.

Let's not forget that this only works if the ThingUID happens to be whatever discovery suggests as well. It's a "hidden option" that users have no idea that they are choosing to enable or disable when they choose the ThingUID.

I think the only "proper" way to manage this is that the bindings does it themselves, independent of discovery. They can subscribe to updates from the same services, and apply updates as appropriate. They can make the configuration options necessary to let the users decide.

I know that this is a "longer path" and that a "quicker fix" is desired, I just don't see that a "quicker fix" can ever be satisfactory. A way to disable this per binding (by the binding author) could then be a way to transition bindings that implement this themselves gradually, without breaking existing functionality.

[holgerfriedrich]

My request is quite simple for the UI - no change needed at all. For me it would be sufficient to have it decided by the binding developer and hard coded in the XML. Basically marking all config parameters which should not be touched by the automatic update process.

[Nadahar]

For me it would be sufficient to have it decided by the binding developer and hard coded in the XML. Basically marking all config parameters which should not be touched by the automatic update process.

The problem is that if you look at the different needs overall, that won't do.

[Nadahar]

Let's also not forget that ConfigDescriptionParameters are used for much more than just Thing configuration. I think modifying those to solve this problem is a bad solution. What does "dynamic" mean for the unit parameter for a widget for example? How are users to interpret that?

[Nadahar]

For reference, I ran a grep over all official bindings that has MDNSDiscoveryParticipant|SddpDiscoveryParticipant|UpnpDiscoveryParticipant in one of it's *.java files (so, somewhat reasonable to expect that they use one of them):

airgradient
amplipi
androiddebugbridge
androidtv
asuswrt
atlona
autelis
avmfritz
benqprojector
bondhome
boschshc
bosesoundtouch
chromecast
deconz
denonmarantz
digitalstrom
dirigera
dlinksmarthome
draytonwiser
enigma2
enphase
epsonprojector
freeboxos
froniuswattpilot
fsinternetradio
hdpowerview
heos
homekit
homematic
hpprinter
hue
huesync
hyperion
ipp
kaleidescape
km200
kodi
konnected
lametrictime
lgwebos
linktap
loxone
lutron
magentatv
mecmeter
miele
minecraft
mpd
mynice
nanoleaf
neeo
neohub
onkyo
openwebnet
panasonicbdp
pioneeravr
pulseaudio
remoteopenhab
roku
samsungtv
shelly
siemenshvac
sonos
sonyaudio
sonyprojector
squeezebox
tado
tivo
tradfri
upnpcontrol
vizio
volumio
wemo
wled
yamahareceiver
zwavejs

It was somewhat more than I expected, but it's not at sure certain that all are affected. I believe that they must also generate "reproducable" UIDs and have a "representation property" that uniquely matches the device for this to take effect.

[mherwege]

I believe that they must also generate "reproducable" UIDs

I still wonder if this is correct.

The representation property was introduced to make sure the same device was not discovered twice with a different UID. Before that introduction, the match for equality was on UID.
It looks like an omission to me if the existing thing update mechanims was not changed to respect this same rule and I would call it a bug. If there is a representation property, that should be the only check. Still, we should probably not change that for now as it gives a workaround to avoid automatic updating.

[mherwege]

EDIT however I think there is not really a problem here. When the Thing is initialized the dynamic="false" can be read from the xml into the respective DTO, and then into the java configuration parameter class instance itself. And from the UI we should be able to access that and modify it again via the DTO. So in other words the xml can pre-set it, but the UI can override that preset. Or??

In theory yes. But know there are 20 different parameter UI classes with different presentation and update behaviour. Introducing an extra UI flag on each of them is, not only complex, but will also make the user experience worse. These parameter classes are not just used for the thing configuration parameters, they are used everywhere in the UI.
Additionally, there is another demand to have some extra logic in these, to be able to not overwrite default values and be able to reset to default, which seems to work for some of these parameter controls, but not all. I think that is something much more important to warrant an extra UI control than what we are talking about here.
That's why I felt this is something can be put in the UI on a thing level, not on a configuration parameter level.

Lastly, I think you need a mechanism to also control it with text based configurations. They are no different. Once the thing is loaded, configuration parameters still may get overwritten in memory. I don't think there is a check if the thing is managed before doing so. Of course, that check could be introduced in the code to avoid overwriting text defined things. That however breaks bindings that rely on some parameters to be discovered to work, even if you have defined the rest in a configuration file (some parameters cannot be known before discovery).

[seime]

For reference, I ran a grep over all official bindings that has ...
...
It was somewhat more than I expected

The list is in fact longer, we have non-official bindings as well where some rely on these protocols.

[Nadahar]

The list is in fact longer, we have non-official bindings as well where some rely on these protocols.

Of course, although I'd say that "rely on" is a strong term. If it's about IP addresses, this is just a problem for networks where devices as just assigned random IPs from the DHCP range. I have all devices on fixed IPs, as I would expect anybody that wants to have any control over their network to do, and as such, there is no desire or need for discovery to "mess with" the configuration.

I still wonder if this is correct.

The representation property was introduced to make sure the same device was not discovered twice with a different UID. Before that introduction, the match for equality was on UID. It looks like an omission to me if the existing thing update mechanims was not changed to respect this same rule and I would call it a bug. If there is a representation property, that should be the only check. Still, we should probably not change that for now as it gives a workaround to avoid automatic updating.

I don't remember the exact source of this information at the moment, but I'm fairly sure that it's true. This is the code in question:

openhab-core/bundles/org.openhab.core.config.discovery/src/main/java/org/openhab/core/config/discovery/internal/PersistentInbox.java

Lines 294 to 307 in 60331d1

	} else if (managedThingProvider.get(thingUID) != null) {
	// only try to update properties if thing is managed
	logger.debug(
	"Discovery result with thing '{}' not added as inbox entry. It is already present as thing in the ThingRegistry.",
	thingUID);
	boolean updated = synchronizeConfiguration(discoveryResult.getThingTypeUID(),
	discoveryResult.getProperties(), thing.getConfiguration());
	if (updated) {
	logger.debug("The configuration for thing '{}' is updated...", thingUID);
	managedThingProvider.update(thing);
	}
	}

As you can see, ThingUID is the very "criteria" being used to decide if "it's the same Thing". Also, these "dynamic updates" are only applied to managed Things, so there are a lot of Things out there that work just fine without this "feature" as well - or there would be a host of problems reported by users with file-based configurations.

I don't think there is a check if the thing is managed before doing so.

It seems like it is. That said, I completely agree that there should be "functional parity" between managed and unmanaged here, and I must say that I consider the current implementation quite broken. It's a "hidden" feature that will help some and cause frustration for some, and it's quite random whether it "works" for you or not.

As I've stated before, I think that it should ideally be handled by each binding that needs it. The next best option would be a per-Thing setting that the user controls, that is exposed/shown only for Things that utilize some of the discovery methods that does this. I'm just not sure if I see a way that can be implemented, an "injected configuration parameter" that is somehow done without the knowledge/participation of the binding itself. Maybe one of you can come up with something clever.

Alternatively, there could be defined a "magic property" that is interpreted by core if it exists, and is used to determine if configuration parameters should be overwritten. Any bindings that want/needs to enable "dynamic" updates would then need to include this "magic property" as a configuration parameter, but wouldn't actually have to do anything with it, because core would evaluate it if it exists.

[mherwege]

I don't remember the exact source of this information at the moment, but I'm fairly sure that it's true. This is the code in question:

I confirm. It looks like the representation property only hides new inbox entries that have the same representation property. But for an existing thing, only the thingID has an impact for updating the configuration parameters. So if you change it, it will not happen anymore. I find that a contradiction. Either you consider them the same, or you don't.
I think if we introduce a flag to supress automatic updates, we should then also fix this so automatic updates work for things with the same representation property irrespective of the thingID.

[Nadahar]

I think if we introduce a flag to supress automatic updates, we should then also fix this so automatic updates work for things with the same representation property irrespective of the thingID.

I agree that it this becomes user controllable, it should work based on representation property, not ThingUID, I also think that it should be enabled for non-managed Things in that case. But, I'm still not sure that I see a way to handle making it configurable...