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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Publish birth and will messages by default #37371

Merged
merged 2 commits into from Jul 4, 2020
Merged

Publish birth and will messages by default #37371

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f6dedcc
Publish birth and will messages by default
emontnemery Jul 2, 2020
6334fb0
Remove useless copy
emontnemery Jul 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 33 additions & 9 deletions homeassistant/components/mqtt/__init__.py
View file
Open in desktop
Expand Up @@ -108,7 +108,7 @@
DEFAULT_PORT = 1883
DEFAULT_KEEPALIVE = 60
DEFAULT_PROTOCOL = PROTOCOL_311
DEFAULT_DISCOVERY_PREFIX = "homeassistant"
DEFAULT_PREFIX = "homeassistant"
DEFAULT_TLS_PROTOCOL = "auto"
DEFAULT_PAYLOAD_AVAILABLE = "online"
DEFAULT_PAYLOAD_NOT_AVAILABLE = "offline"
Expand Down Expand Up @@ -137,10 +137,24 @@ def validate_device_has_at_least_one_identifier(value: ConfigType) -> ConfigType
"the MQTT broker configuration"
)

DEFAULT_BIRTH = {
ATTR_TOPIC: DEFAULT_PREFIX + "/status",
CONF_PAYLOAD: DEFAULT_PAYLOAD_AVAILABLE,
ATTR_QOS: DEFAULT_QOS,
ATTR_RETAIN: DEFAULT_RETAIN,
}

DEFAULT_WILL = {
ATTR_TOPIC: DEFAULT_PREFIX + "/status",
CONF_PAYLOAD: DEFAULT_PAYLOAD_NOT_AVAILABLE,
ATTR_QOS: DEFAULT_QOS,
ATTR_RETAIN: DEFAULT_RETAIN,
}

MQTT_WILL_BIRTH_SCHEMA = vol.Schema(
{
vol.Required(ATTR_TOPIC): valid_publish_topic,
vol.Required(ATTR_PAYLOAD, CONF_PAYLOAD): cv.string,
vol.Inclusive(ATTR_TOPIC, "topic_payload"): valid_publish_topic,
vol.Inclusive(ATTR_PAYLOAD, "topic_payload"): cv.string,
vol.Optional(ATTR_QOS, default=DEFAULT_QOS): _VALID_QOS_SCHEMA,
vol.Optional(ATTR_RETAIN, default=DEFAULT_RETAIN): cv.boolean,
},
Expand Down Expand Up @@ -186,13 +200,17 @@ def embedded_broker_deprecated(value):
vol.Optional(CONF_PROTOCOL, default=DEFAULT_PROTOCOL): vol.All(
cv.string, vol.In([PROTOCOL_31, PROTOCOL_311])
),
vol.Optional(CONF_WILL_MESSAGE): MQTT_WILL_BIRTH_SCHEMA,
vol.Optional(CONF_BIRTH_MESSAGE): MQTT_WILL_BIRTH_SCHEMA,
vol.Optional(
CONF_WILL_MESSAGE, default=DEFAULT_WILL
): MQTT_WILL_BIRTH_SCHEMA,
vol.Optional(
CONF_BIRTH_MESSAGE, default=DEFAULT_BIRTH
): MQTT_WILL_BIRTH_SCHEMA,
vol.Optional(CONF_DISCOVERY, default=DEFAULT_DISCOVERY): cv.boolean,
# discovery_prefix must be a valid publish topic because if no
# state topic is specified, it will be created with the given prefix.
vol.Optional(
CONF_DISCOVERY_PREFIX, default=DEFAULT_DISCOVERY_PREFIX
CONF_DISCOVERY_PREFIX, default=DEFAULT_PREFIX
): valid_publish_topic,
}
),
Expand Down Expand Up @@ -668,7 +686,10 @@ def init_client(self):
self._mqttc.on_disconnect = self._mqtt_on_disconnect
self._mqttc.on_message = self._mqtt_on_message

if CONF_WILL_MESSAGE in self.conf:
if (
CONF_WILL_MESSAGE in self.conf
and ATTR_TOPIC in self.conf[CONF_WILL_MESSAGE]
):
will_message = Message(**self.conf[CONF_WILL_MESSAGE])
else:
will_message = None
Expand Down Expand Up @@ -719,7 +740,7 @@ async def async_disconnect(self):

def stop():
"""Stop the MQTT client."""
self._mqttc.disconnect()
# Do not disconnect, we want the broker to always publish will
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't HomeAssistant set the DEFAULT_PAYLOAD_NOT_AVAILABLE explicitly and then disconnect gracefully here? Otherwise, it may take a while until the MQTT broker detects that the client disappeared, leading to an unnecessary delay...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The connection to the MQTT server is actively broken, and the broker will immediately publish the will.

What you describe is an issue if the connection is not actively broken, in which case the TCP connection will linger until it times out.

If you think this is not the case, please provide a way to reproduce the issue you suggest.

self._mqttc.loop_stop()

await self.hass.async_add_executor_job(stop)
Expand Down Expand Up @@ -818,7 +839,10 @@ def _mqtt_on_connect(self, _mqttc, _userdata, _flags, result_code: int) -> None:
max_qos = max(subscription.qos for subscription in subs)
self.hass.add_job(self._async_perform_subscription, topic, max_qos)

if CONF_BIRTH_MESSAGE in self.conf:
if (
CONF_BIRTH_MESSAGE in self.conf
and ATTR_TOPIC in self.conf[CONF_BIRTH_MESSAGE]
):
birth_message = Message(**self.conf[CONF_BIRTH_MESSAGE])
self.hass.add_job(
self.async_publish( # pylint: disable=no-value-for-parameter
Expand Down
58 changes: 57 additions & 1 deletion tests/components/mqtt/test_init.py
View file
Open in desktop
Expand Up @@ -755,7 +755,7 @@ def mock_tls_set(certificate, certfile=None, keyfile=None, tls_version=None):
}
],
)
async def test_birth_message(hass, mqtt_client_mock, mqtt_mock):
async def test_custom_birth_message(hass, mqtt_client_mock, mqtt_mock):
"""Test sending birth message."""
calls = []
mqtt_client_mock.publish.side_effect = lambda *args: calls.append(args)
Expand All @@ -764,6 +764,62 @@ async def test_birth_message(hass, mqtt_client_mock, mqtt_mock):
assert calls[-1] == ("birth", "birth", 0, False)


async def test_default_birth_message(hass, mqtt_client_mock, mqtt_mock):
"""Test sending birth message."""
calls = []
mqtt_client_mock.publish.side_effect = lambda *args: calls.append(args)
mqtt_mock._mqtt_on_connect(None, None, 0, 0)
await hass.async_block_till_done()
assert calls[-1] == ("homeassistant/status", "online", 0, False)


@pytest.mark.parametrize(
"mqtt_config", [{mqtt.CONF_BROKER: "mock-broker", mqtt.CONF_BIRTH_MESSAGE: {}}],
)
async def test_no_birth_message(hass, mqtt_client_mock, mqtt_mock):
"""Test sending birth message."""
calls = []
mqtt_client_mock.publish.side_effect = lambda *args: calls.append(args)
mqtt_mock._mqtt_on_connect(None, None, 0, 0)
await hass.async_block_till_done()
assert not calls


@pytest.mark.parametrize(
"mqtt_config",
[
{
mqtt.CONF_BROKER: "mock-broker",
mqtt.CONF_WILL_MESSAGE: {
mqtt.ATTR_TOPIC: "death",
mqtt.ATTR_PAYLOAD: "death",
},
}
],
)
async def test_custom_will_message(hass, mqtt_client_mock, mqtt_mock):
"""Test will message."""
mqtt_client_mock.will_set.assert_called_with("death", "death", 0, False, None)


async def test_default_will_message(hass, mqtt_client_mock, mqtt_mock):
"""Test will message."""
mqtt_client_mock.will_set.assert_called_with(
"homeassistant/status", "offline", 0, False, None
)


@pytest.mark.parametrize(
"mqtt_config", [{mqtt.CONF_BROKER: "mock-broker", mqtt.CONF_WILL_MESSAGE: {}}],
)
async def test_no_will_message(hass, mqtt_client_mock, mqtt_mock):
"""Test will message."""
mqtt_client_mock.will_set.assert_not_called()


@pytest.mark.parametrize(
"mqtt_config", [{mqtt.CONF_BROKER: "mock-broker", mqtt.CONF_BIRTH_MESSAGE: {}}],
)
async def test_mqtt_subscribes_topics_on_connect(hass, mqtt_client_mock, mqtt_mock):
"""Test subscription to topic on connect."""
await mqtt.async_subscribe(hass, "topic/test", None)
Expand Down