[nextion] Merge upload tft engine in a single file

[edwardtfn]

What does this implement/fix?

This is another try to merge the algorithms into a single file, as I've previously tried with #6192, but this time in a more simplified way. The new file is the Arduino one with the modifications required by ESP-IDF. Although the #ifdef are not the best readable thing, it was just a couple of those new, so I believe this worth the change, as it makes way easier to maintain this code, reducing the need to repeat the changes on both files every time we want to make a change.

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): N/A

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

Test Environment

• ESP32
• ESP32 IDF
• ESP8266
• RP2040
• BK72xx
• RTL87xx

Example entry for config.yaml:

N/A

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.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.15%. Comparing base (4d8b5ed) to head (2194077).
Report is 514 commits behind head on dev.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #6670      +/-   ##
==========================================
+ Coverage   53.70%   54.15%   +0.44%     
==========================================
  Files          50       50              
  Lines        9408     9575     +167     
  Branches     1654     1687      +33     
==========================================
+ Hits         5053     5185     +132     
- Misses       4056     4066      +10     
- Partials      299      324      +25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[probot-esphome]

Hey there @SenexCrenshaw, mind taking a look at this pull request as it has been labeled with an integration (nextion) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

[jesserockz]

We are actually trying to do the opposite with other components. Trying to split files that are littered with #ifdefs.

[edwardtfn]

Well, I see your point, but #ifdef's will be there anyeays, and in this case, just the http calls are different, while the entire engine is exactly the same. I've been maintaining the component since a while and keep consistent between two identical files is just double work, which increases the chances for errors.
I'm not gonna fight that much for this, but I truly believe it will be more maintenable this way.
Unless you have a hard position on the direction of splitting files, I can try to make it a bit more readable by splitting the http calls into separated functions, so the core engine will be just onem0 and onky the http calls will have #ifdef...
Maybe a nextion_upload_common.cpp with the core engine... What do you think?
Its not about reducing the number of files, but making it easier to maintain.

[jesserockz]

TBH I am just fine with someone wanting to maintain the component =)

If it makes it easier for you, then combine them.

One thing I noticed is that the ifdefs should be USE_ARDUINO, but also, it would be nice to use the idf based code on every esp32 regardless of framework, and then the Arduino code only for the other platforms that this supports like esp8266 etc.

[kbx81]

Its not about reducing the number of files, but making it easier to maintain.

I'm a big advocate of readability -- lots of #ifdefs around blocks of code impairs readability. Code that's easier to read is easier to maintain. That said, personally, I'm totally fine with an #ifdef here and there, as long as it's not terribly crazy. Still, I also agree with Jesse -- quite happy to have someone more actively maintaining this code (especially since I use and depend on it myself). Anyway, just my two cents. 😄

[edwardtfn]

Ok, I will find a better fit here and submit this again.

[edwardtfn]

Change it to esp32 vs esp8266 makes sense and will make it more relevant if we want to extend this to other platforms (rp2040, bk72xx, etc).
But then I have another question... Does it make sense invest time expanding the components to other platforms when I'm not using that myself? Or should I do only if a feature request exists?

[kbx81]

Does it make sense invest time expanding the components to other platforms when I'm not using that myself? Or should I do only if a feature request exists?

It's nice if you care to do so, but as you are (presumably) doing this in your free/spare time, we're not going to hold you to that expectation. 😄 Others may appreciate it at some point, but how far you take it is up to you. Regardless, smaller, incremental changes are easier to review; keep that in mind as you continue your great work! 😄🍻

[jesserockz]

But then I have another question... Does it make sense invest time expanding the components to other platforms when I'm not using that myself? Or should I do only if a feature request exists?

I would prefer you don't write code for platforms you don't use/test =)
It can be marked as supported only on esp32/esp8266 until such a time as someone comes along and extends it.

[edwardtfn]

Testing is not the issue, as I believe I have all those devices here... just need time to connect the display and install ESPHome on those, but as Keith said, I'm doing this on my free time and that may not be the best use of it if no one is gonna use. I would prioritize the existing issues and feature requests around Nextion instead. 😉
And I have a non-supported Dwin display sitting on my desk, maybe I find some time to create a component for that later. 😃

[edwardtfn]

About using the IDF engine for Arduino, it isn't that straightforward as Arduino didn't liked to use the ESP-IDF http client library. 😞

In file included from /data/cache/platformio/packages/framework-arduinoespressif32/tools/sdk/esp32/include/lwip/lwip/src/include/lwip/ip_addr.h:43,
                 from /data/cache/platformio/packages/framework-arduinoespressif32/tools/sdk/esp32/include/lwip/lwip/src/include/lwip/sockets.h:46,
                 from /data/cache/platformio/packages/framework-arduinoespressif32/tools/sdk/esp32/include/lwip/port/esp32/include/sys/socket.h:33,
                 from /data/cache/platformio/packages/framework-arduinoespressif32/tools/sdk/esp32/include/esp_http_client/include/esp_http_client.h:14,
                 from src/esphome/components/nextion/nextion.h:16,
                 from src/esphome/components/nextion/nextion.cpp:1:
/data/cache/platformio/packages/framework-arduinoespressif32/tools/sdk/esp32/include/lwip/lwip/src/include/lwip/ip4_addr.h:63:37: error: expected ')' before numeric constant
 #define IPADDR_NONE         ((u32_t)0xffffffffUL)
                             ~       ^~~~~~~~~~~~
/data/cache/platformio/packages/framework-arduinoespressif32/tools/sdk/esp32/include/lwip/lwip/src/include/lwip/inet.h:71:29: note: in expansion of macro 'IPADDR_NONE'
 #define INADDR_NONE         IPADDR_NONE
                             ^~~~~~~~~~~
/data/cache/platformio/packages/framework-arduinoespressif32/cores/esp32/IPAddress.h:95:18: note: in expansion of macro 'INADDR_NONE'
 extern IPAddress INADDR_NONE;
                  ^~~~~~~~~~~
In file included from /data/cache/platformio/packages/framework-arduinoespressif32/tools/sdk/esp32/include/lwip/lwip/src/include/lwip/ip_addr.h:43,
                 from /data/cache/platformio/packages/framework-arduinoespressif32/tools/sdk/esp32/include/lwip/lwip/src/include/lwip/sockets.h:46,
                 from /data/cache/platformio/packages/framework-arduinoespressif32/tools/sdk/esp32/include/lwip/port/esp32/include/sys/socket.h:33,
                 from /data/cache/platformio/packages/framework-arduinoespressif32/tools/sdk/esp32/include/esp_http_client/include/esp_http_client.h:14,
                 from src/esphome/components/nextion/nextion.h:16,
                 from src/esphome/components/nextion/nextion_upload_esp32.cpp:1:
/data/cache/platformio/packages/framework-arduinoespressif32/tools/sdk/esp32/include/lwip/lwip/src/include/lwip/ip4_addr.h:63:37: error: expected ')' before numeric constant
 #define IPADDR_NONE         ((u32_t)0xffffffffUL)
                             ~       ^~~~~~~~~~~~
/data/cache/platformio/packages/framework-arduinoespressif32/tools/sdk/esp32/include/lwip/lwip/src/include/lwip/inet.h:71:29: note: in expansion of macro 'IPADDR_NONE'
 #define INADDR_NONE         IPADDR_NONE
                             ^~~~~~~~~~~
/data/cache/platformio/packages/framework-arduinoespressif32/cores/esp32/IPAddress.h:95:18: note: in expansion of macro 'INADDR_NONE'
 extern IPAddress INADDR_NONE;
                  ^~~~~~~~~~~
Compiling .pioenvs/office-workstation-panel/src/esphome/components/ota/ota_backend_arduino_esp32.cpp.o
*** [.pioenvs/office-workstation-panel/src/esphome/components/nextion/nextion_upload_esp32.cpp.o] Error 1
*** [.pioenvs/office-workstation-panel/src/esphome/components/nextion/nextion.cpp.o] Error 1
========================== [FAILED] Took 3.46 seconds ==========================

[jesserockz]

About using the IDF engine for Arduino, it isn't that straightforward as Arduino didn't liked to use the ESP-IDF http client library. 😞

Curse you Arduino!

Ok, you can switch it back to arduino vs esp-idf and I may take a look in the future

[edwardtfn]

Ok, you can switch it back to arduino vs esp-idf and I may take a look in the future

Ok. This is how it is in dev right now. 😉

One thing I noticed is that the ifdefs should be USE_ARDUINO,

#6700 fixes that. 😃