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

Support for MQTT Configuration in a single Backlog command #17

Merged
merged 6 commits into from
Jan 26, 2021

Conversation

deveth0
Copy link
Collaborator

@deveth0 deveth0 commented Jan 15, 2021

No description provided.

Copy link
Owner

@tobias-richter tobias-richter left a comment

Choose a reason for hiding this comment

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

Not everyone wants to set client, topic and fulltopic and keep the defaults.
IMHO this is to specific for a special use-case.
From my point of view only host, port are common for everyone. There may be setups out there which do not use user and passwort for their MQTT server (even if it is bad).

Since the Backlog functionality is already supported in the current role and the user can already configure the values in one command I do not see a need in making this part of the role.
Perhaps an example in the example section would make sense.

MqttClient {{ tasmota_mqtt_client }};
MqttTopic {{ tasmota_mqtt_topic }};
MqttFullTopic {{ tasmota_mqtt_fulltopic }};
when: >
Copy link
Owner

Choose a reason for hiding this comment

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

You can use the list syntax here:

when:
  - tasmota_mqtt_user
  - tasmota_mqtt_password
  - tasmota_mqtt_host 
  - tasmota_mqtt_port
  - tasmota_mqtt_client 
  - tasmota_mqtt_topic 
  - tasmota_mqtt_fulltopic

@deveth0
Copy link
Collaborator Author

deveth0 commented Jan 15, 2021

If you don't set values for those fields, they are not set, so I think it's fine to support the full set and only set those fields that are configured.

As mqtt is a basic functionality of tasmota, it justifies a build in block imho.

@tobias-richter
Copy link
Owner

tobias-richter commented Jan 16, 2021

@deveth0 In my setup I have not set these values and i was not required to. I am only setting FriendlyName1 and DeviceName and enabling the home assistant integration and it works like a charm ootb without setting topic and fulltopic. If I now want to adjust the basic mqtt settings based on your implementation I have to lookup the automatically configured values and port them to the configuration management.
From my point of view this would be a little bit awkward.

@deveth0
Copy link
Collaborator Author

deveth0 commented Jan 16, 2021

Nope, cause the default is an empty string which does not cause a change (using commands without value are just read commands for the current value).
So if your want to set the host, you only set the host value and leave the others.

@deveth0 deveth0 closed this Jan 16, 2021
@deveth0 deveth0 reopened this Jan 16, 2021
@tobias-richter
Copy link
Owner

Let's have a chat about this next week. I still see no need for this since it does not offer new functionality. Everything is already possible with the existing implementation and it is simply an new command entry that is passed to the action module.

Beside that it will not report a valid changed/not changed response.

@tobias-richter tobias-richter merged commit bd919cc into master Jan 26, 2021
@deveth0 deveth0 deleted the feature/mqtt_backlog branch February 27, 2021 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants