Skip to content

Update device registry of MQTT Switch#19540

Merged
balloob merged 4 commits intohome-assistant:devfrom
emontnemery:mqtt_switch_update_device_info
Jan 25, 2019
Merged

Update device registry of MQTT Switch#19540
balloob merged 4 commits intohome-assistant:devfrom
emontnemery:mqtt_switch_update_device_info

Conversation

@emontnemery
Copy link
Copy Markdown
Contributor

@emontnemery emontnemery commented Dec 23, 2018

Description:

Support update of device registry so Hass doesn't need to be restarted to, for example, show updated firmware version of the device.

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This line can be removed after this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Why is it ok to remove the device_info function?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not all of it. But everything except for identifiers/connections can be removed as now that's handled above.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK, I think it's fixed now.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, but the code is now no longer duplicated in MqttEntityDeviceInfo.device_info_discovery_update(), so shouldn't be removed from MqttEntityDeviceInfo.device_info().

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Check if self._config_entry is None before. If the user tries to configure the device registry through YAML, this line will raise an AttributeError

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought device registry only works through discovery as documented: https://www.home-assistant.io/components/switch.mqtt/#device
Can you give an example of when this errors so I can test and verify your suggestion?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Example is in previous comment.

Previously, this would not do anything in the case of yaml config. But now this will raise an attributeerror. We can do better than that. Let's print a proper warning/error

Copy link
Copy Markdown
Contributor Author

@emontnemery emontnemery Jan 2, 2019

Choose a reason for hiding this comment

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

But function device_info_discovery_update is only called if mqtt discovery message is received again after the switch had been discovered, how can this happen for a device configured trough yaml? An example of exactly what yaml triggers an error would be helpful.

Copy link
Copy Markdown
Contributor Author

@emontnemery emontnemery Jan 3, 2019

Choose a reason for hiding this comment

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

I think this is fixed/clearer now that the duplicated code has been removed.

@emontnemery
Copy link
Copy Markdown
Contributor Author

@OttoWinter Do you think this is OK to merge now after the duplicated code was removed?

@emontnemery emontnemery force-pushed the mqtt_switch_update_device_info branch from 50e220e to 519d3b0 Compare January 7, 2019 16:22
@emontnemery emontnemery force-pushed the mqtt_switch_update_device_info branch 2 times, most recently from 50e220e to 5c65c56 Compare January 7, 2019 17:00
@thomasloven
Copy link
Copy Markdown
Contributor

I can't see anything indicating it, but I'm not really comfortable with the entity registry, so just to make sure:
This does not remove the ability to add mqtt devices via legacy yaml config, right?

@emontnemery
Copy link
Copy Markdown
Contributor Author

@thomasloven That's absolutely not the intention. The purpose of this PR is to allow the device registry to be updated without a restart of Hass. A typical use case would firmware version to be correctly shown in Hass UI.

@thomasloven
Copy link
Copy Markdown
Contributor

Thank you for the clarification.

@emontnemery emontnemery force-pushed the mqtt_switch_update_device_info branch from e445db9 to b5233b7 Compare January 15, 2019 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants