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.

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

Temporary fix for the client_id generation (fixes #8315) #8336

Merged
merged 4 commits into from
Jul 5, 2017
Merged

Conversation

fabaff
Copy link
Member

@fabaff fabaff commented Jul 4, 2017

Description:

MQTT 3.1.1 delegates the generation of a client ID to the server. With paho-mqtt-1.3.0 and the embedded MQTT server it only works if client_id is provided.

This temporary fix allows to run the embedded MQTT server without setting a client_id.

Related issue (if applicable): fixes #8315

Example entry for configuration.yaml (if applicable):

mqtt:

Checklist:

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass

import random
client_id = conf.get(
CONF_CLIENT_ID, 'home-assistant-{}'.format(random.randrange(0, 1000)))
#client_id = conf.get(CONF_CLIENT_ID)

Choose a reason for hiding this comment

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

block comment should start with '# '

# Remove the generation of the client_id after it's fixed in HBMQTT
import random
client_id = conf.get(
CONF_CLIENT_ID, 'home-assistant-{}'.format(random.randrange(0, 1000)))
Copy link
Contributor

@karlw00t karlw00t Jul 4, 2017

Choose a reason for hiding this comment

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

Should we check to make sure the protocol is 3.1.1? I think with 3.1, zero length clientIds are supported by the pano-mqtt client. That being said, it just generates a random clientId for you, but who knows if someone depends on that.

Another option is to set the protocol to 3.1.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we check to make sure the protocol is 3.1.1?

We could but I see no benefit. Isn't it highly unlikely that somebody depends on a random generated ID? My assumption was that if you depend on the client ID then it's configured already.

Another option is to set the protocol to 3.1.

As far as I remember is HBMQTT only supporting MQTT 3.1.1.

Copy link
Member

Choose a reason for hiding this comment

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

Yes HBMQTT support only 3.1.1

@balloobbot balloobbot added the core label Jul 5, 2017
@@ -315,6 +315,10 @@ def async_setup(hass, config):
client_cert = conf.get(CONF_CLIENT_CERT)
tls_insecure = conf.get(CONF_TLS_INSECURE)
protocol = conf[CONF_PROTOCOL]

Choose a reason for hiding this comment

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

blank line contains whitespace

@balloob balloob added this to the 0.48.1 milestone Jul 5, 2017
@balloob
Copy link
Member

balloob commented Jul 5, 2017

I tweaked the fix to only apply if we are starting the built-in MQTT broker.

@balloob balloob merged commit 0ecceb6 into dev Jul 5, 2017
@balloob balloob deleted the client-id-mqtt branch July 5, 2017 04:47
balloob pushed a commit that referenced this pull request Jul 5, 2017
* Temporary fix for the client_id generation (fixes #8315)

* Fix comment

* Move client id setting.

* Lint
@balloob balloob mentioned this pull request Jul 5, 2017
@benmccoy
Copy link

benmccoy commented Jul 5, 2017

this didnt fix the issue for me... had to add client_id: foo to get this working on 0.48.1

@heinemml
Copy link
Contributor

heinemml commented Jul 6, 2017

The intended "tweak" by @balloob made the fix only apply when you start the internal broker with an embedded: configuration. But the empty mqtt: case is left out. This would be the elif broker_config case.

heinemml added a commit to heinemml/home-assistant that referenced this pull request Jul 6, 2017
This applies what was the intended fix in home-assistant#8336.

moves the fallback for setting client_id to the case when no mqtt config was provided at all. This should reflect the most common use case that fails.

This commit should be reverted when hbmqtt is fixed to allow empty client_id again.
heinemml added a commit to heinemml/home-assistant that referenced this pull request Jul 6, 2017
This applies what was the intended fix in home-assistant#8336.

moves the fallback for setting client_id to the case when no mqtt config was provided at all. This should reflect the most common use case that fails.

This commit is a workaround and should be reverted when hbmqtt is fixed to allow empty client_id again.
balloob pushed a commit that referenced this pull request Jul 6, 2017
This applies what was the intended fix in #8336.

moves the fallback for setting client_id to the case when no mqtt config was provided at all. This should reflect the most common use case that fails.

This commit is a workaround and should be reverted when hbmqtt is fixed to allow empty client_id again.
@balloob balloob mentioned this pull request Jul 13, 2017
dethpickle pushed a commit to dethpickle/home-assistant that referenced this pull request Aug 18, 2017
home-assistant#8336)

* Temporary fix for the client_id generation (fixes home-assistant#8315)

* Fix comment

* Move client id setting.

* Lint
dethpickle pushed a commit to dethpickle/home-assistant that referenced this pull request Aug 18, 2017
This applies what was the intended fix in home-assistant#8336.

moves the fallback for setting client_id to the case when no mqtt config was provided at all. This should reflect the most common use case that fails.

This commit is a workaround and should be reverted when hbmqtt is fixed to allow empty client_id again.
@home-assistant home-assistant locked and limited conversation to collaborators Oct 20, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Embedded MQTT Broker failing on 0.48.0
9 participants