Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[mqtt] allow disabling discovery #8077

Merged
merged 3 commits into from
Jul 23, 2020
Merged

[mqtt] allow disabling discovery #8077

merged 3 commits into from
Jul 23, 2020

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented Jul 4, 2020

Fixes #8056

This adds a parameter to the broker to disable discovery. By default this is set to false (so discovery is enabled), which is the same behaviour like before.

Signed-off-by: Jan N. Klug [email protected]

@J-N-K
allow disabling discovery
f631643 
Signed-off-by: Jan N. Klug <[email protected]>
@J-N-K J-N-K added the enhancement An enhancement or new feature for an existing add-on label Jul 4, 2020
@J-N-K J-N-K requested a review from davidgraeff as a code owner July 4, 2020 18:44
@TravisBuddy
Copy link

Travis tests were successful

Hey @J-N-K,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@kaikreuzer
Copy link
Member

This all sounds fishy to me, so please let me try to understand the real issue first.

In general:

  • Every discovery service can be deactivated using the discovery.<id> pid.
  • Discovery services should just silently operate and if there is nothing relevant for them to be seen, they must not cause any errors or side effects.

If those two statements are taken into account, I would assume there would not be any problem with #8056, is this correct?
If so, I would claim that we rather need a bug fix instead of introducing a feature as a workaround.

@J-N-K
Copy link
Member Author

J-N-K commented Jul 12, 2020

The problem seems to be that the discovery service „silently“ listens to what is sent by the broker. This requires a subscription to a topic or a group of topics. Some brokers close the connection if they receive subscriptions for topics they don’t support (IMO broken, but out of our range to fix). So the only solution is to prevent subscription to „unwanted“ topics on these brokers and that requires disabling discovery. But only for that broker, not in general (there might be a second broker used for homie devices).

@kaikreuzer
Copy link
Member

Ok, thanks for the explanation.
It feels a bit weird to have double-negation here: Enabling this flag disables a feature - it would imho be cleaner to have a flag "discovery", which can be enabled (true) and disabled (false), where the default setting would be true. I don't insist on it, but wanted to raise it. Wdyt?

@J-N-K
Copy link
Member Author

J-N-K commented Jul 22, 2020

Agreed. I‘ll change to „discoveryEnabled“ with a default „true“, so nothing changes for users.

@J-N-K
address review comments
dfc408e 
Signed-off-by: Jan N. Klug <[email protected]>
@TravisBuddy
Copy link

Travis tests were successful

Hey @J-N-K,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

kaikreuzer
kaikreuzer requested changes Jul 23, 2020
View reviewed changes
bundles/org.openhab.binding.mqtt/README.md Outdated
By default discovery services (like homie or homeassistant) are enabled on a broker.
This behaviour can be controlled with a configuration parameter.

* __enableDiscovery__:If set to true, enables discovery on this broker, if set to false, disables discovery services om this broker.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* __enableDiscovery__:If set to true, enables discovery on this broker, if set to false, disables discovery services om this broker.
* __enableDiscovery__:If set to true, enables discovery on this broker, if set to false, disables discovery services on this broker.

...openhab.binding.mqtt/src/main/java/org/openhab/binding/mqtt/handler/SystemBrokerHandler.java Outdated
@@ -50,6 +50,7 @@
protected final MqttService service;

protected String brokerID = "";
protected boolean discoveryDisabled = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still saying "disabled" here

bundles/org.openhab.binding.mqtt/src/main/resources/ESH-INF/thing/thing-types.xml Outdated
@@ -181,6 +187,12 @@
<label>Broker ID</label>
<description>Each system wide configured MQTT broker has a unique broker ID.</description>
</parameter>
<parameter name="disableDiscovery" type="boolean">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still saying "disable" here

@J-N-K
address review comments
2828d7c 
Signed-off-by: Jan N. Klug <[email protected]>
@kaikreuzer kaikreuzer added this to the 2.5.8 milestone Jul 23, 2020
@kaikreuzer kaikreuzer merged commit ba915f4 into openhab:2.5.x Jul 23, 2020
CSchlipp pushed a commit to CSchlipp/openhab-addons that referenced this pull request Jul 26, 2020
@J-N-K @CSchlipp
[mqtt] allow disabling discovery (openhab#8077)
8153efc 
* allow disabling discovery

Signed-off-by: Jan N. Klug <[email protected]>
Signed-off-by: CSchlipp <[email protected]>
@J-N-K J-N-K deleted the mqtt-onoff branch August 1, 2020 15:17
@No3x No3x mentioned this pull request Aug 2, 2020
Closed
MPH80 pushed a commit to MPH80/openhab-addons that referenced this pull request Aug 3, 2020
@J-N-K @MPH80
[mqtt] allow disabling discovery (openhab#8077)
594c15d 
* allow disabling discovery

Signed-off-by: Jan N. Klug <[email protected]>
Signed-off-by: MPH80 <[email protected]>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
@J-N-K @andrewfg
[mqtt] allow disabling discovery (openhab#8077)
0651b30 
* allow disabling discovery

Signed-off-by: Jan N. Klug <[email protected]>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
@J-N-K @andrewfg
[mqtt] allow disabling discovery (openhab#8077)
f7e9163 
* allow disabling discovery

Signed-off-by: Jan N. Klug <[email protected]>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
@J-N-K @andrewfg
[mqtt] allow disabling discovery (openhab#8077)
08417ee 
* allow disabling discovery

Signed-off-by: Jan N. Klug <[email protected]>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
@J-N-K @andrewfg
[mqtt] allow disabling discovery (openhab#8077)
b53b9ac 
* allow disabling discovery

Signed-off-by: Jan N. Klug <[email protected]>
DaanMeijer pushed a commit to DaanMeijer/openhab-addons that referenced this pull request Sep 1, 2020
@J-N-K @DaanMeijer
[mqtt] allow disabling discovery (openhab#8077)
dfbf03e 
* allow disabling discovery

Signed-off-by: Jan N. Klug <[email protected]>
Signed-off-by: Daan Meijer <[email protected]>
markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Sep 19, 2020
@J-N-K @markus7017
[mqtt] allow disabling discovery (openhab#8077)
520d1a5 
* allow disabling discovery

Signed-off-by: Jan N. Klug <[email protected]>
@openhab-bot
Copy link
Collaborator

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

https://community.openhab.org/t/integrate-dyson-pure-cool-link/40416/92

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaikreuzer kaikreuzer kaikreuzer approved these changes

@davidgraeff davidgraeff Awaiting requested review from davidgraeff davidgraeff is a code owner

Assignees
No one assigned
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Milestone
2.5.8
Development

Successfully merging this pull request may close these issues.

[mqtt] Cannot connect to TheThingsNetwork because of HomeAssistant discovery
4 participants
@J-N-K @TravisBuddy @kaikreuzer @openhab-bot