Disallow variant/family override for known boards

[clydebarrow]

What does this implement/fix?

Types of changes

Currently setting variant: (family: for LibreTiny) in the esp32 component makes ESPHome think it's compiling for a variant that may be different to the default one from board:. This option should not be allowed if the board type is known, since the user may infer it actually changes something.

This is a breaking change since any yamls currently including variant: will now fail to compile.

Now the variant will be allowed without any warning or error message if it matches the board.

Tests have not been added since the ESPHome testing structure currently has no provision for tests that must fail.

• 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 esphome/issues#5664

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

Test Environment

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

Example entry for config.yaml:

# Example config.yaml

rtl87xx:
  board: generic-rtl8710bn-2mb-xxxk
  variant: rtl8720cf

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.

[probot-esphome]

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

[probot-esphome]

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

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.43%. Comparing base (4d8b5ed) to head (aba67ca).
Report is 424 commits behind head on dev.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #6512      +/-   ##
==========================================
- Coverage   53.70%   53.43%   -0.28%     
==========================================
  Files          50       50              
  Lines        9408     9537     +129     
  Branches     1654     1685      +31     
==========================================
+ Hits         5053     5096      +43     
- Misses       4056     4130      +74     
- Partials      299      311      +12     

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

[kuba2k2]

Hi,
I think these changes might actually be unnecessary for the LibreTiny platform - it doesn't use the build.mcu field for compilation. The compilation process only depends on the board JSON file, which contains the information about chip family.

The family: field was added to ESPHome to allow using new boards supported by LibreTiny, which haven't been added to ESPHome yet. It does not change the compilation parameters, it only tells ESPHome to adjust its own USE_* macros to that family. LibreTiny will not work without a board JSON. Changing its family directly is also not possible (different partition layouts, sometimes even encryption keys).

Since the term "variant" is not used internally in LibreTiny (instead "family" is used), I'd propose to keep the original naming (maybe even replace the USE_LIBRETINY_VARIANT_* with USE_LIBRETINY_FAMILY_*.

Also, adding each chip type to ESPHome might make maintaining it more involving - each new chip type supported by LibreTiny will need to be added and users will not be able to just choose a family of a newly supported board.

The following test YAML:

rtl87xx:
  board: generic-rtl8710bn-2mb-788k
  variant: rtl8720cf

will compile the firmware for the RTL8710BN chip, not the RTL8720CF. These belong to different families (AmebaZ vs AmebaZ2).

I hope this will make it easier to understand how LibreTiny handles the compilation for different chips.

[clydebarrow]

Thanks for the response.

I think these changes might actually be unnecessary for the LibreTiny platform - it doesn't use the build.mcu field for compilation. The compilation process only depends on the board JSON file, which contains the information about chip family.

And currently that (the PIO board file) is where PIO gets the mcu type from since ESPHome doesn't pass on the variant selection.

Let me backtrack a bit to explain the reason for going down this rabbit hole:

The original problem (which came up for ESP32) was that if a board is specified but a variant also specified, internally ESPHome uses the variant, but does not pass that to Platformio, which leads to C++ compile-time failures if e.g. pins or libraries dependent on ESP32S3 are compiled for ESP32. E.g:

src/esphome/components/rpi_dpi_rgb/rpi_dpi_rgb.cpp: In member function 'virtual void esphome::rpi_dpi_rgb::RpiDpiRgb::setup()':
src/esphome/components/rpi_dpi_rgb/rpi_dpi_rgb.cpp:10:3: error: 'esp_lcd_rgb_panel_config_t' was not declared in this scope
   esp_lcd_rgb_panel_config_t config{};

That's clearly undesirable - config errors should be caught at ESPHome build time - one of the strong points of ESPHome is that the average user should never see a compiler error, only YAML validation errors.

So the logical solution is to pass the variant on to PIO for compilation - it makes sense that if you have said this chip is an XYZ123 then PIO should compile for that, not the default for the board. So my original (simple) change was just to add the build.mcu PIO flag, and that works just fine for ESP32 (RP2040 and ESP8266 don't have variants, so it doesn't apply to them.) And I thought "Great! Job done!"

But, tests for Libretiny failed because for some of the chips, the family being derived from the board files was not an mcu type recognised by PIO. So I ended up in the rabbit hole changing that.

There is the question of what purpose the variant and family options serve in the first place, and if there's no good justification for being able to override the variant they could simply be removed.

But if they are going to remain, they should work properly.

rtl87xx:
  board: generic-rtl8710bn-2mb-788k
  variant: rtl8720cf

will compile the firmware for the RTL8710BN chip, not the RTL8720CF. These belong to different families (AmebaZ vs AmebaZ2).

No, that will (after this PR) compile for rtl8720cf. The current implementation would look like this:

rtl87xx:
  board: generic-rtl8710bn-2mb-788k
  family: rtl8720cf

And that (currently) fails to pass YAML verification:

rtl87xx: [source rtl87.yaml:7]
  board: generic-rtl8710bn-2mb-788k

  Unknown value 'RTL8720CF', did you mean 'RTL8720C', 'RTL8710B'?.
  family: rtl8720cf

If you use one of the suggested options, it now builds, but as you pointed out, for rtl8710bn. That's an unexpected result since something different was requested.

So basically the family/variant option is currently broken - it does not do what it says on the tin. So it should either be fixed or removed. I went to the trouble of fixing it, since I can see some utility in it being available, but if there's a consensus that it should be removed, I'll have no issue with that.

[clydebarrow]

Also, adding each chip type to ESPHome might make maintaining it more involving - each new chip type supported by LibreTiny will need to be added

There's a script that does that so the effort is small. If there is a board file in Platformio then ESPHome supports it. New variants would also automagically become available via the same process.

Since the term "variant" is not used internally in LibreTiny (instead "family" is used), I'd propose to keep the original naming (maybe even replace the USE_LIBRETINY_VARIANT_* with USE_LIBRETINY_FAMILY_*.

I'd argue that the typical user doesn't need to know what happens internally in LibreTiny, and variant is an established term in ESPHome, so it makes sense to keep consistency. Internal constants like USE_LIBRETINY_VARIANT_ are only seen by developers so that's of lesser importance (but VARIANT was already in use.)

[kuba2k2]

And currently that (the PIO board file) is where PIO gets the mcu type from since ESPHome doesn't pass on the variant selection.

Unfortunately, that's not the case in LibreTiny. It uses build.family to determine the chip family, based on this the appropriate code building script is used (builder/family/*.py). The board.mcu property is used 1) to name the output .UF2 file accordingly, 2) as a #define for C code, which only uses it in lt_cpu_get_model() function.

That's clearly undesirable - config errors should be caught at ESPHome build time - one of the strong points of ESPHome is that the average user should never see a compiler error, only YAML validation errors.

I do 100% agree with that - I was trying to find a solution for LibreTiny too.

But, tests for Libretiny failed because for some of the chips, the family being derived from the board files was not an mcu type recognised by PIO. So I ended up in the rabbit hole changing that.

Because variant is different than family. Family is for... well, a family of chips - MCUs that are similar, but not identical, and code may be directly compatible between them. One thing known for sure, is that code from a certain family will not boot on a different one.

There is the question of what purpose the variant and family options serve in the first place, and if there's no good justification for being able to override the variant they could simply be removed.

You're right, there's no reason to override the variant (MCU type). However, the family override was added to make it possible to use new boards in ESPHome without waiting for a release with updated boards.py.

There's a script that does that so the effort is small. If there is a board file in Platformio then ESPHome supports it.

Continuing: there is a script that generates the boards.py. But if a board gets support in LibreTiny, a PR for ESPHome will have to be submitted, merged and released as a new ESPHome minor version. Only then can the user compile for that board. The family field was added to "ignore what ESPHome knows" and basically say hey ESPHome, use this board, it really exists and belongs to <xyz> family.

---

So the logical solution is to pass the variant on to PIO for compilation - it makes sense that if you have said this chip is an XYZ123 then PIO should compile for that, not the default for the board.

Unfortunately, it is not that simple. Let's say you want to compile for the cbu board (BK7231N) and you change the family to BK7231T. If LibreTiny was to respect that change (by setting build.family, not build.mcu) it would compile for the BK7231T chip using settings appropriate for BK7231N chip - like the partition layout. The resulting firmware would probably not even be able to connect to Wi-Fi (if it boots at all).

will compile the firmware for the RTL8710BN chip, not the RTL8720CF. These belong to different families (AmebaZ vs AmebaZ2).

No, that will (after this PR) compile for rtl8720cf. The current implementation would look like this:

As mentioned in the 1st paragraph of this comment, LibreTiny will ignore that change. It will still compile for RTL8710BN. If you change build.family instead, it will try to compile for RTL8720CF and fail, because the board JSON is missing information that are required for that family (partition layout, bootloader keys).

---

So basically the family/variant option is currently broken - it does not do what it says on the tin. So it should either be fixed or removed. I went to the trouble of fixing it, since I can see some utility in it being available, but if there's a consensus that it should be removed, I'll have no issue with that.

In conclusion: there's no reason to change the chip family in LibreTiny - it is not even possible. The family: field was only introduced to let ESPHome know about the family type of (yet) unknown boards. And you're correct that it is indeed half broken because of that

• If we remove the family: field, ESPHome will not allow using unknown (new) boards.
• If we keep that field, ESPHome will not be able to validate if the family type even exists.

I think the right solution would be to make the family: field only applicable for unknown boards, reserve it for advanced users only. This way, it's their responsibility to make sure the family is correct. Known boards wouldn't allow setting that property, so the validation and compilation would always pass without problems.

Maybe it would help to rename it, e.g. board_family: or custom_family: or unknown_board_family:. Then we can keep the existing USE_LIBRETINY_VARIANT_* macros with family names. Using chip names here is redundant, since the chips within a single family offer the same peripherals.

[clydebarrow]

Ok, so I understand more about what's going on, and the motivation for the option. It does seem to work differently to using PIO with esp-idf.

Maybe it would help to rename it, e.g. board_family: or custom_family: or unknown_board_family:.

I think this is probably the best idea right now. And it should be disallowed if the board: setting is known to ESPHome. Probably not even necessary to rename it, but have something like this:

rtl87xx:
  board: generic-rtl8710bn-2mb-1024k
  allow_unknown_board: true
  family: rtl8710b

It seems only effect of specifying the family on ESPHome is to set macros like:
-DUSE_LIBRETINY_FAMILY_RTL8720CF which (as far as I can tell) has no effect on any ESPHome code right now. But it needs a placeholder since it can't get the family from the board definition. So another question is whether there should be any validation on the argument, since if the board is unknown to ESPHome the family may be also.

How does that sound? And I can't see any reason not to use the same approach with ESP32, since the motivation for the variant option is presumably the same (but for ESP32 the variant does affect ESPHome since it has conditional code for different variants.)

Not important - but what's the difference between RTL8710B (a family) and RTL8710BN (or X) - which are mcu types within that family. The BK series apparently has no such oddities.

[kuba2k2]

I think this is probably the best idea right now. And it should be disallowed if the board: setting is known to ESPHome. Probably not even necessary to rename it, but have something like this:

rtl87xx:
  board: generic-rtl8710bn-2mb-1024k
  allow_unknown_board: true
  family: rtl8710b

Yes, this solution seems to fit really well.

It seems only effect of specifying the family on ESPHome is to set macros like:
-DUSE_LIBRETINY_FAMILY_RTL8720CF which (as far as I can tell) has no effect on any ESPHome code right now. But it needs a placeholder since it can't get the family from the board definition. So another question is whether there should be any validation on the argument, since if the board is unknown to ESPHome the family may be also.

Correct, the macros are only supposed to enable/disable certain features available on a certain family only, however as you have noticed they aren't used so far. Up to this point, the family type validation required it to be known to ESPHome - after all, there's no benefit of specifying the family name unknown to ESPHome, as there will be no macros using that name anyway. Removing the macros would allow to remove the family: field entirely, but I think it's better to keep them for any future needs.

In the current codebase the family name for known boards is in boards.py, so retrieving the name for macros is not a problem. For unknown boards, you can just use whatever family is used in family:.

I can't speak for the ESP32 variant field, because I'm not familiar enough with the differences that it introduces.

Not important - but what's the difference between RTL8710B (a family) and RTL8710BN (or X) - which are mcu types within that family. The BK series apparently has no such oddities.

Good question, it seems to be a bit confusing at first. LibreTiny configures most compilation options based on the family - SDK to use, files to compile, #defines to enable, etc. The MCU type is less important and not generally used. In short, the family divides chips by their similarity and compatibility.
The BK series could all belong to the BK72XX or BK7231 family - however, the T and N variants (as well as BK7252) do have some differences in peripherals (like Wi-Fi, PWM, interrupts) which makes the code pretty much incompatible, so they have been split into several families.
That being said, if I come up with a better solution to reorganize the families, I probably will do that in the next refactor.

[probot-esphome]

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