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

MQTT JSON light options brightness and color_mode should be deprecated #110682

Closed
fredck opened this issue Feb 15, 2024 · 5 comments · Fixed by #111676
Closed

MQTT JSON light options brightness and color_mode should be deprecated #110682

fredck opened this issue Feb 15, 2024 · 5 comments · Fixed by #111676
Assignees

Comments

@fredck
Copy link

fredck commented Feb 15, 2024

The problem

The brightness and color_mode configuration options, available in the json configuration schema, should be deprecated in the light platform of the MQTT integration, in favor of the supported_color_modes configuration.

The rational is simple:

  • They're redundant: supported_color_modes conveys the same information that these "flags" provide.
  • They're confusing: one may think that it is enough to set brightness: true to have brightness only supported, but this is not true. We faced issues because of that, in fact (which lead to this issue)
  • Light wants that: The Light Entity is clearly stating that supported_color_modes is the way to go and already deprecated the support for "flags" (supported_features)

I'm not saying that these attributes (which for sure are widely used already) should be removed. It would be enough to mark them as deprecated in the documentation and to not require them when supported_color_modes is defined.

Actually, the documentation should actively recommend implementing supported_color_modes.

What version of Home Assistant Core has the issue?

core-2024.2.1

What was the last working version of Home Assistant Core?

No response

What type of installation are you running?

Home Assistant OS

Integration causing the issue

MQTT

Link to integration documentation on our website

https://www.home-assistant.io/integrations/mqtt/

Diagnostics information

No response

Example YAML snippet

# Example configuration.yaml entry (current)
mqtt:
  - light:
      schema: json
      name: mqtt_json_light_1
      state_topic: "home/rgb1"
      command_topic: "home/rgb1/set"
      brightness: true
      color_mode: true
      supported_color_modes: ["brightness"]

# Example configuration.yaml entry (after deprecation)
mqtt:
  - light:
      schema: json
      name: mqtt_json_light_1
      state_topic: "home/rgb1"
      command_topic: "home/rgb1/set"
      supported_color_modes: ["brightness"]

Anything in the logs that might be useful for us?

No response

Additional information

No response

@home-assistant
Copy link

Hey there @emontnemery, @jbouwh, mind taking a look at this issue as it has been labeled with an integration (mqtt) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of mqtt can trigger bot actions by commenting:

  • @home-assistant close Closes the issue.
  • @home-assistant rename Awesome new title Renames the issue.
  • @home-assistant reopen Reopen the issue.
  • @home-assistant unassign mqtt Removes the current integration label and assignees on the issue, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the issue.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

(message by CodeOwnersMention)


mqtt documentation
mqtt source
(message by IssueLinks)

@emontnemery emontnemery changed the title Configurations brightness and color_mode should be deprecated MQTT JSON light options brightness and color_mode should be deprecated Feb 26, 2024
@emontnemery
Copy link
Contributor

Yes, this makes sense.

The following flags are already deprecated, and removed from the documentation:

  • color_temp
  • hs
  • rgb
  • xy

We can indeed also deprecate the brightness flag and remove it from the documentation.

The color_mode flag is a bit tricky because we need to change its default value if we deprecate it, I think we need to first warn that the default value is about to change if it's not set.

@fredck
Copy link
Author

fredck commented Feb 26, 2024

The color_mode flag is a bit tricky because we need to change its default value if we deprecate it, I think we need to first warn that the default value is about to change if it's not set.

You're right. A deprecation warning is welcome.

Hopefully the impact will be small, since this could be an issue only if you have supported_color_modes defined but you want it to be ignored, which is counterintuitive (and weird).

@jbouwh
Copy link
Contributor

jbouwh commented Mar 1, 2024

@fredck We cannot deprecated the brightness flag as we still support rgbx lights without x brightness topics. But except for on/off lights brightness support should be assumed. This is what the linked PR suggests now. May be you can have a look at it?
The PR deprecates color_mode and assumes supported_color_modes to supply the basic info instead of rgb, color_temp, xy, hs flags. Some of these flags were already removed the the documentation but were not marked a deprecated the the code yet.

@fredck
Copy link
Author

fredck commented Mar 4, 2024

We cannot deprecated the brightness flag as we still support rgbx lights without x brightness topics.

@jbouwh, As long as the documentation clearly states the use case, and that specifying supported_color_modes is a must, and that brightness is by default assumed on on supported color modes, this should be fine.

I see that @emontnemery is already on your PR. Thanks a lot for the effort! 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants