Preparing color.h for IDF >= 5

[HeMan]

What does this implement/fix?

Compiler started to give warnings about operator= in color.h when preparing other components for IDF >= 5. Adding copy constructor and changing is_on to const gave a suggestion from compiler to change argument to const reference on display-classes. It's a breaking change for external modules based on the display class.

Types of changes

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Other

Related issue or feature (if applicable): fixes

Pull request in esphome-docs with documentation (if applicable): esphome/esphome-docs#

Test Environment

• ESP32
• ESP32 IDF
• ESP8266
• RP2040

Example entry for config.yaml:

# Example config.yaml

Checklist:

• The code change is tested and works locally.
• Tests have been added to verify that the new code works (under tests/ folder).

If user exposed functionality or configuration variables are added/changed:

• Documentation added/updated in esphome-docs.

[guillempages]

Are you sure this is the right way to go? When the "=" operator is used now, won't this convert the color first to a 32 bit int, do some bit-magic and create a new Color? Won't we lose performance here? When drawing images, the "=" operator is being used on the hot loop IIRC.

[HeMan]

Are you sure this is the right way to go? When the "=" operator is used now, won't this convert the color first to a 32 bit int, do some bit-magic and create a new Color? Won't we lose performance here? When drawing images, the "=" operator is being used on the hot loop IIRC.

No, I'm not sure. The message I got was:

src/esphome/core/color.h: In member function 'esphome::Color esphome::Color::operator+(const esphome::Color&) const':
src/esphome/core/color.h:116:12: warning: implicitly-declared 'constexpr esphome::Color::Color(const esphome::Color&)' is deprecated [-Wdeprecated-copy]
  116 |     return ret;
      |            ^~~
src/esphome/core/color.h:61:17: note: because 'esphome::Color' has user-provided 'esphome::Color& esphome::Color::operator=(const esphome::Color&)'
   61 |   inline Color &operator=(const Color &rhs) ALWAYS_INLINE {  // NOLINT
      |                 ^~~~~~~~
src/esphome/core/color.h: In member function 'esphome::Color esphome::Color::operator-(const esphome::Color&) const':
src/esphome/core/color.h:139:12: warning: implicitly-declared 'constexpr esphome::Color::Color(const esphome::Color&)' is deprecated [-Wdeprecated-copy]
  139 |     return ret;
      |            ^~~
src/esphome/core/color.h:61:17: note: because 'esphome::Color' has user-provided 'esphome::Color& esphome::Color::operator=(const esphome::Color&)'
   61 |   inline Color &operator=(const Color &rhs) ALWAYS_INLINE {  // NOLINT
      |                 ^~~~~~~~
src/esphome/core/color.h: In member function 'esphome::Color esphome::Color::gradient(const esphome::Color&, uint8_t)':
src/esphome/core/color.h:164:12: warning: implicitly-declared 'constexpr esphome::Color::Color(const esphome::Color&)' is deprecated [-Wdeprecated-copy]
  164 |     return new_color;
      |            ^~~~~~~~~
src/esphome/core/color.h:61:17: note: because 'esphome::Color' has user-provided 'esphome::Color& esphome::Color::operator=(const esphome::Color&)'
   61 |   inline Color &operator=(const Color &rhs) ALWAYS_INLINE {  // NOLINT
      |                 ^~~~~~~~

I'll try adding an explicit copy constructor instead, could that work?

[guillempages]

looks good to me

[HeMan]

looks good to me

To bad it failed CI... Reworking it now

[guillempages]

Wow, what a rabbit hole you got yourself into :-D

The changes look good to me; just a small suggestion regarding name, but feel free to ignore it.

This might have conflicts with #5001 though; IMHO it would be better to wait until that one is merged (hopefully soon), and rebase on top of that. Now that it has been merged, you should probably rebase.

[guillempages]

AFAIK the C++ convention for copy constructors is to call the original "other", but I might be mistaken here.

Suggested change

	inline Color(const Color &other_color) : raw_32(other_color.raw_32) {}
	inline Color(const Color &other) : raw_32(other.raw_32) {}

[guillempages]

Also this should probably be marked as a breaking change; if you had to change the const'ness of all the displays, external components will also break and need to change that.

(Don't get me wrong; I still think these changes you are making are meaningful and good)

[guillempages]

Looks good to me.

As mentioned above this might be a breaking change for external display components. IMHO it should be put in the same release as #5002 (which is also a breaking change for external display components), so that those components do not need to react to breaking changes twice.

[ayufan]

@HeMan I wonder, can we just remove the offended operator = instead? It seems to serve no function, and is rather slower due to copying each byte individually.

Adding const Color & everywhere is not the best idea (from performance point of view) since it makes to pass a pointer, and getting the value requires doing dereference of the object, where as the current Color does fit the 32bit register.

Also the description says "Compiler started to give warnings about operator= in color.h when preparing other components for IDF >= 5.". It would help to have those warning listed in the description.

IMHO. Just remove operator = (const Color &.

cc @guillempages

[HeMan]

Reference

Hmm, @guillempages had the suspicion that that it would cast to uint32_t to let the other operator= do it's magic, but I tried to remove both operators and everything seem to work fine. I think that is the best solution, but I'll create another PR to let you decide which to use.

[ayufan]

@HeMan Thanks. I think we should merge the #5070, not this PR.