[miele] Fix mDNS issue where hub repeatedly disappears from, resp. reappears in, the Inbox.

[andrewfg]

BACKGROUND

There is an mDNS issue where the Miele hub repeatedly disappears from, resp. reappears in, the Inbox (see issue description in openhab/openhab-core#1835). This PR resolves that issue. Fixes #11837

DEPENDENCY

This PR depends on openhab/openhab-core#2635 => so that PR must be merged prior to this one. (And for that reason the CI build process will fail until that respective core PR has been merged).

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

[jlaur]

@andrewfg - I can create an issue for the issue specific to the Miele XGW 3000 gateway.

[jlaur]

I have tested this PR in combination with openhab/openhab-core#2635 on a fresh 3.3 snapshot installation, and it works correctly. Previously the gateway would be removed from the inbox every two minutes and readded shortly after. Now:

Restarted openHAB, manually removed thing from inbox:
2021-12-20 22:22:11.068 [INFO ] [openhab.event.InboxRemovedEvent ] - Discovery Result with UID 'miele:xgw3000:Miele_XGW3000' has been removed.

Was discovered again:
2021-12-20 22:22:20.934 [INFO ] [openhab.event.InboxAddedEvent ] - Discovery Result with UID 'miele:xgw3000:Miele_XGW3000' has been added.

Then 5-6 minutes of silence (normally it's removed and readded every two minutes). Unplugged network cable from gateway:
2021-12-20 22:29:39.937 [INFO ] [openhab.event.InboxRemovedEvent ] - Discovery Result with UID 'miele:xgw3000:Miele_XGW3000' has been removed.

And plugged in again:
2021-12-20 22:30:52.140 [INFO ] [openhab.event.InboxAddedEvent ] - Discovery Result with UID 'miele:xgw3000:Miele_XGW3000' has been added.

[jlaur]

@andrewfg - I can create an issue for the issue specific to the Miele XGW 3000 gateway.

#11837 created, can be linked.

[kaikreuzer]

If this is more or less a firmware bug of the XGW3000 (and I can confirm that I am seeing the same with mine), I wonder if this needs to be exposed to the user at all as a parameter - it might be rather confusing as it is highly technical.
If we consider this a bug fix for the firmware behavior, we could easily just define the grace period as a constant in the code, wdyt?

[jlaur]

If this is more or less a firmware bug of the XGW3000 (and I can confirm that I am seeing the same with mine), I wonder if this needs to be exposed to the user at all as a parameter - it might be rather confusing as it is highly technical. If we consider this a bug fix for the firmware behavior, we could easily just define the grace period as a constant in the code, wdyt?

@kaikreuzer - I agree. I had the same thought, but was unsure about how much technical stuff is usually hidden in the binding configurations (e.g. in other bindings). I don't think the few XGW 3000 users would ever need to change this value. For me it quite consistently disappears for just a few seconds (~3 seconds) every two minutes. Do you see the same? The value could even be lowered to 15 seconds, I think - not that it matters much, only it would be removed a bit quicker when actually disconnected.

[kaikreuzer]

Do you see the same?

Yes, I had just tested it with a test openHAB installation, since my production one does not create inbox entries (since I have a thing in place already).

[jlaur]

Do you see the same?

Yes, I had just tested it with a test openHAB installation, since my production one does not create inbox entries (since I have a thing in place already).

Hmm, I see it happening on my production installation as well even with a thing in place. Is that a known difference or bug with file-based configuration? I'm pretty sure the representation property is correct.

Clarification: I see it happening in logs. In inbox it's ignored.

[andrewfg]

wonder if this needs to be exposed to the user at all as a parameter

@kaikreuzer many thanks for your feedback! Apparently I can't win: the code is copy/pasted from the code I wrote for the Philips Hue binding in #9985 -- where I originally wrote it with a hard coded constant, but one of your esteemed colleagues asked me to change it to a config param. But I am happy either way. :)

[kaikreuzer]

Haha, it can be hard for a contributor to deal with the contradicting feedback from different maintainers. 😄

I guess you refer to this comment: #9985 (comment)
I fully support this one from @cweitkamp - in that situation, it sounds as if it isn't really clear, which value might help, so hardcoding might have been an issue as it does not allow to experiment with it. But @cweitkamp also only suggested it to be a configuration for the discovery, so that it can be tweaked through a runtime.cfg entry, but not boldly exposing it to the user as a configuration value in the UI. As in our case here it is also specifically a setting for the discovery service, I'd also claim that it should not be a parameter for the binding itself (Background: I would have loved for a long long time already to extract different UPnP+mDNS discovery services into an "installed by default" bundle, so that the setup wizard could directly identify devices in your home without the need of installing all the bindings first.).

Now the problem is indeed that for hue a different solution has been implemented already and I am always for consistency. So either we keep things here as they are right now (definitely the simplest solution) or we would have to also adapt the hue binding accordingly (and by this leave the path to a dedicated discovery bundle open). Happy to hear your opinions here!

[andrewfg]

^
@kaikreuzer yes that is the comment I was referring to.

And after reading it again, I see now that I had mis-interpreted the suggestion of @cweitkamp concerning an entry in $OPENHAB_CONF/services/runtime.cfg and wrongly implemented a binding config param instead. (Maybe because I didn't know how to write the code to parse entries in runtime.cfg)

I am quite happy to modify both this PR and also raise a new PR for the Hue in order to make them mutually compatible (note: I just did a search to check there are no other bindings who have implemented this).

I would also modify the PR for the binding developer guide documentation to include a code example based on runtime.cfg.

In retrospect, I think the runtime.cfg entry is probably the best one overall. => Please confirm. And if so, can you please give me a pointer how to code it?

Obviously there is a small risk that changing to runtime.cfg for the Hue binding might upset an existing user who had customised a config param value different than the default. But since the default has always worked perfectly for me, I think the risk of that happening is actually quite small.

I would have loved for a long long time already to extract different UPnP+mDNS discovery services into an "installed by default" bundle, so that the setup wizard could directly identify devices in your home without the need of installing all the bindings first.)

Yay! Great idea!

[andrewfg]

can you please give me a pointer how to code it?

Ok. I see that the properties will have been passed in the component's activate() method.

[kaikreuzer]

Right, all you'd need to do is to add (configurationPid = "discovery.miele") to this line and the parameters that are provided as discovery.miele:gracePeriod=15 in e.g. runtime.cfg would then be automatically passed to the activate(@Nullable Map<String, Object> configProperties) method. No need to directly deal with the ConfigAdmin service anymore. 😎

[cweitkamp]

we would have to also adapt the hue binding accordingly

While talking about the hue binding ... I am working on support for the new Hue API v2. Some preparations e.g. moving discovery from UPnP to mDNS are done in #11842. Let me know if I should touch configuration for it in advance.

[kaikreuzer]

@cweitkamp I guess it makes sense if you do that change in #11842 then - there is no urgency for it.

[andrewfg]

all you'd need to do is to add

@kaikreuzer shall I take this as your agreement to change the code in this PR to the runtime.cfg model?

[kaikreuzer]

@andrewfg As you seem to like the idea: Yes, nothing should stop you from doing so!

[andrewfg]

^
But will that eventually match what you guys might do on the hue? I suppose, (to answer my own question), if you approve this PR before doing any work on the Hue, you can just copy paste the approved code to the Hue PR anyway. Or??

[lolodomo]

I just say for the hue binfong, I never encountered the problem before the parameter was added.
The grace period was useless with my hurting bridge.
What is the default parameter value? 0 ?

[andrewfg]

What is the default parameter value? 0 ?

50 seconds. Am I correct in understanding that this default value works just fine for you?

[lolodomo]

As it works for me without a grace period, of course it will work with a grace period... but with a useless delay.

I just checked and my parameter is set to 50. So would make sense iny case to set it to 0.
By the way, it is not very important.

My concern was just to keep the parameter for the hue binding.

[jlaur]

@andrewfg, @lolodomo - as much as this discussion is relevant, I believe it is mostly relevant for the Hue binding? In that case maybe continue the discussion in context of #11842. That way it will be more transparent to anyone following that PR and/or with knowledge of the specific Hue issue before UPnP grace period was introduced.

For the Miele binding I think we have established that a default value of 15 seconds will be appropriate, since it seems to be a general issue with the latest (and probably last) version of the XGW 3000 firmware, 2.4.2. Both @kaikreuzer and I have observed it disappear only for a few seconds. However, since now configurable, users can quickly adapt in the unlikely event that a new firmware version would ever be released, which would change anything. Or in case of specific issues caused by some internal network problems that might need additional grace period.

[jlaur]

@andrewfg - would it be possible for you to make new jars available for download?

• org.openhab.core.config.discovery.mdns-3.3.0-SNAPSHOT.jar
• org.openhab.binding.miele-3.3.0-SNAPSHOT.jar

Then I will perform a (hopefully) final test with the latest version of the code.

[andrewfg]

continue the discussion in context

Ok. Done => #11842 (comment)

[andrewfg]

would it be possible for you to make new jars available for download?
org.openhab.core.config.discovery.mdns-3.3.0-SNAPSHOT.jar
org.openhab.binding.miele-3.3.0-SNAPSHOT.jar

• The former has already been merged into the core, so if you update your test system to today's SNAPSHOT you get it.
• The latter is here https://github.com/andrewfg/openhab-addons/tree/jar-uploads/bundles/org.openhab.binding.miele

[andrewfg]

I would have loved for a long long time already to extract different UPnP+mDNS discovery services into an "installed by default" bundle

@kaikreuzer I think this a brilliant idea, and I would be happy to contribute to it. Do you have already an Issue opened somewhere which mentions this. So I could perhaps add to the discussion. Or if there is no such Issue yet, do you mind if I create a new one?

[cweitkamp]

LGTM. Thanks,

@kaikreuzer What about you?

[kaikreuzer]

I don't think the few XGW 3000 users would ever need to change this value.

I wonder if we shouldn't also simply wipe the section in the README - if we consider it a technical bug fix, we can treat this as a "hidden" feature - mentioned in the code and available to the ones that know about it, but not officially exposing it to the users. Wdyt?

Do you have already an Issue opened somewhere which mentions this.

No, I haven't. I don't have any clear idea on how it could be implemented (and whether the issue should go in core or addons repo). But please feel free to open an issue yourself and start a discussion around it!

[jlaur]

@andrewfg - I have tested the new version and it still works. :-) I can add that 8 second grace period seems to be the sweet spot for me with a fresh and performant openHAB installation having nothing else to do. So I hope this verifies 15 seconds as a sensible value for production systems.

[cweitkamp]

Do you have already an Issue opened somewhere which mentions this.

No, I haven't. I don't have any clear idea on how it could be implemented (and whether the issue should go in core or addons repo). But please feel free to open an issue yourself and start a discussion around it!

From my pov a first step could be to introduce a console command extension (or a REST interface) which helps an advanced user to list all available mDNS services or UPnP devices on the local network manually. Currently I am using Linux shell avahi-discover to identify mDNS services. We already have such a tool for transport-serial (see openhab/openhab-core#1172). Wdyt?

[kaikreuzer]

@andrewfg & @cweitkamp I have created openhab/openhab-core#2645 for that discussion so that we don't have to go too much off-topic here. 😄

[jlaur]

I don't think the few XGW 3000 users would ever need to change this value.

I wonder if we shouldn't also simply wipe the section in the README - if we consider it a technical bug fix, we can treat this as a "hidden" feature - mentioned in the code and available to the ones that know about it, but not officially exposing it to the users. Wdyt?

In general I think undocumented features are better than no features at all. And that documented features are better than undocumented features. :-) In this case I'm in doubt. It would be preferable if it would be handled centrally in core instead of all bindings having to implement this configuration option, and with that document it for all bindings having it in place. This is however not the topic for this PR.

So I think we should keep the documentation, and if it evolves further, we can adapt. Probably no one will ever have to change this, but this is mostly because we presumably have so few users. For the Hue binding some users had concrete problems (with UPnP) and needed it. However, for the Hue binding it could be discovered by users by going to the binding configuration. For the new implementation where you have to add it to a configuration file, users won't have the same possibility of finding it without documentation.

[kaikreuzer]

Alright, then let's keep user docs in place.

Many thanks for the fix and your patience, @andrewfg!

[jlaur]

While talking about the hue binding ... I am working on support for the new Hue API v2. Some preparations e.g. moving discovery from UPnP to mDNS are done in #11842. Let me know if I should touch configuration for it in advance.

@cweitkamp - out of curiosity, did you have anything more prepared after #11842? I'm asking since @andrewfg has created #13570 so just wanted to check that nothing is lost or conflicting.

[andrewfg]

@cweitkamp - out of curiosity, did you have anything more prepared after #11842? I'm asking since @andrewfg has created #13570 so just wanted to check that nothing is lost or conflicting.

I did ask @cweitkamp already, and he said he was rather busy at the moment, so it was Ok for me to proceed..

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/openhab-marketing-is-lacking/149280/25