Update boards.txt file to fix some of the broken board.build entries that should be upper case #7942

FrightRisk · 2023-03-08T20:10:51Z

Fix some of the board.build entries that are lower case to make them upper case. In particular, the Heltec boards were broken in the Arduino IDE with version 2.0.7 or the ESP32 library when they were changed from HELTEC_WIFI_32 to heltec_wifi_32, etc.

Description of Change

Changes many, but not all of the incorrectly lowecase board.build names to be uppercase. When they were changed to lowercase, they broke conditional compiles based on board type. The standard since these are constants, is to be upper case.

Tests scenarios

Arduino IDE 1.8.09 and 2.0.4. Tested with the Heltec wifi board and wifi board v3

Partially fixes issue

Fixes #7943

Fix some of the board.build entries that are lower case to make the upper case

CLAassistant · 2023-03-08T20:11:01Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

platypii · 2023-03-08T20:54:54Z

If you're going to fix some of them, seems like you might just fix them all? Here are the remaining ones:

grep 'build.board=[a-z]' boards.txt

esp32vn-iot-uno.build.board=esp32vn_iot_uno
heltec_wireless_stick.build.board=heltec_wireless_stick
sensesiot_weizen.build.board=sensesiot_weizen
OpenKB.build.board=openkb
uPesy_wrover.build.board=uPesy_WROVER
uPesy_wroom.build.board=uPesy_WROOM
atmegazero_esp32s2.build.board=atmegazero_esp32s2
connaxio_espoir.build.board=connaxio_espoir
unphone8.build.board=unphone8
unphone9.build.board=unphone9
roboheart_hercules.build.board=roboheart_hercules

mrengineer7777 · 2023-03-08T21:23:56Z

@FrightRisk I couldn't find a commit that specifically made those entries lower case. It looks like they were originally lower case when added. I agree constants should be fully uppercase. Most of the .build.board= entries are uppercase.

boards.txt

platypii · 2023-03-08T23:12:06Z

I couldn't find a commit that specifically made those entries lower case. It looks like they were originally lower case when added. I agree constants should be fully uppercase. Most of the .build.board= entries are uppercase.

#4577 made the heltec ones lowercase

mrengineer7777 · 2023-03-09T00:45:38Z

#4577 made the heltec ones lowercase

Now that's digging deep. Nice catch!

FrightRisk · 2023-03-09T02:05:20Z

If you're going to fix some of them, seems like you might just fix them all? Here are the remaining ones:

grep 'build.board=[a-z]' boards.txt

esp32vn-iot-uno.build.board=esp32vn_iot_uno
heltec_wireless_stick.build.board=heltec_wireless_stick
sensesiot_weizen.build.board=sensesiot_weizen
OpenKB.build.board=openkb
uPesy_wrover.build.board=uPesy_WROVER
uPesy_wroom.build.board=uPesy_WROOM
atmegazero_esp32s2.build.board=atmegazero_esp32s2
connaxio_espoir.build.board=connaxio_espoir
unphone8.build.board=unphone8
unphone9.build.board=unphone9
roboheart_hercules.build.board=roboheart_hercules

Happy to do the rest. I wasn't sure about a couple of them, though I thought they should all be in caps, you guys confirmed it. I can update the PR Thursday sometime. It is Wednesday night for me now.

Here are some I wasn't sure of because I don't know what "revisions" means and if I should change anything that has build.board in it

ttgo-lora32.build.board=TTGO_LoRa32

ttgo-lora32.menu.Revision.TTGO_LoRa32_V1=TTGO LoRa32 V1 (No TFCard)
ttgo-lora32.menu.Revision.TTGO_LoRa32_V1.build.board=TTGO_LoRa32_V1
ttgo-lora32.menu.Revision.TTGO_LoRa32_V1.build.variant=ttgo-lora32-v1

ttgo-lora32.menu.Revision.TTGO_LoRa32_V2=TTGO LoRa32 V2
ttgo-lora32.menu.Revision.TTGO_LoRa32_V2.build.board=TTGO_LoRa32_V2
ttgo-lora32.menu.Revision.TTGO_LoRa32_V2.build.variant=ttgo-lora32-v2

ttgo-lora32.menu.Revision.TTGO_LoRa32_v21new=TTGO LoRa32 V2.1 (1.6.1)
ttgo-lora32.menu.Revision.TTGO_LoRa32_v21new.build.board=TTGO_LoRa32_v21new
ttgo-lora32.menu.Revision.TTGO_LoRa32_v21new.build.variant=ttgo-lora32-v21new

FrightRisk · 2023-03-09T17:11:39Z

Still unclear about several other lower case boards like a LoRa and "Pocket32", the grep doesn't seem to show them, but they are there. Should I change every build.board entry to upper case?

atanisoft · 2023-03-09T17:14:49Z

Should I change every build.board entry to upper case?

I would if for no other reason than to be consistent across the board on what the #define will end up as.

platypii · 2023-03-09T17:30:37Z

the grep doesn't seem to show them

Good point, here's a better regex. It will find boards with any lowercase or - characters. Dash should probably be replaced by underscore too since it's not a legal envvar.

grep 'build.board=\w*[a-z-]' boards.txt

Make all build.board entries consistent with all caps and underscores instead of dashes

FrightRisk · 2023-03-09T18:00:19Z

Yes, I fixed the dashes too when I saw those. I see "variants" and left those alone though some have dashes and some have underscores

atanisoft · 2023-03-09T18:15:48Z

I see "variants" and left those alone though some have dashes and some have underscores

The board.variant entry needs to be an exact match for the board specific directory listed in https://github.com/espressif/arduino-esp32/tree/master/variants.

Fix spelling and remove line spacing added by mistake

Fix last 2 boards including fixing the Lionbit board entry

FrightRisk · 2023-03-09T18:40:31Z

Ok, I think I am good then. All the build boards are uppercase and all use underscores. I also changed the Lion:Bit board. It is referenced elsewhere as "lionbit" and I don't think there should be colons in constants, so made it LIONBIT_DEV_BOARD. I believe it is read for review.

VojtechBartoska · 2023-03-22T12:42:35Z

@P-R-O-C-H-Y Please review this PR.

me-no-dev · 2023-03-31T10:51:04Z

Wouldn't changing ALL boards now cause other conditional statements to fail? I know for a fact that M5 have some conditions that depends on the correct name as-is

FrightRisk · 2023-03-31T12:41:56Z

I guess that decision rests with someone above me, but the standard is that these should be all caps and that is what I would vote for. You really can't have a hodge podge mess. I agree that it might break something else if people were used to a constant listed incorrectly, but is a very simple fix to just change to all caps in any existing code. M5 can issue an update.

P-R-O-C-H-Y · 2023-04-03T11:46:58Z

@Tinyu-Zhao Can you please take a look if this PR will cause issues for your boards? Thanks

Tinyu-Zhao · 2023-04-07T07:54:21Z

@Tinyu-Zhao Can you please take a look if this PR will cause issues for your boards? Thanks

I will check it，Thanks.

tobozo · 2023-04-16T09:27:17Z

hi,

please don't do this to all boards.

as @me-no-dev mentioned, this will break all third party libraries that rely on board names for device detection at compile time, and there are many, I'm only listing here a few ones from memory but I know there are more:

LovyanGFX
M5Unified
M5Stack
M5Core2
ESP32-Chimera-Core
ESP32-targz
ESP32-USB-Soft-Host

And "the fix is to add an if defined" looks as ugly as this:

Please consider the tradeoff of being semantically correct vs the workload and the bugs this will generate downstream.

atanisoft · 2023-04-16T12:52:34Z

@tobozo There will be breakages regardless of what is done or not done unfortunately. The mixed case names should never have been included in the first place.

I agree the workaround is a bit ugly but perhaps it could be mitigated by adding a board pins variant header that defines the old mixed case version of the define?

tobozo · 2023-04-16T16:03:29Z

@atanisoft unless I missed a document saying these are the standards, The standard since these are constants, is to be upper case. sounds more like a convention to me (e.g. linux kernel doesn't respect that).

If it is only a convention, then is it required?

I'm just weighing the tech debt between following that convention and only fixing the heltect board, and I don't understand why an upper or mixed-case macro is important at this stage or even in the future, please enlighten me.

mitigated by adding a board pins variant header that defines the old mixed case version of the define

Although easier to manage that's still a tech debt: trading a global compile flag with a scoped will only affect multidevice libraries/projects who do not explicitely include that header.

atanisoft · 2023-04-16T16:09:22Z

trading a global compile flag with a scoped will only affect multidevice libraries/projects who do not explicitely include that header.

If they include Arduino.h it will be automatically included (I haven't seen any library that doesn't include this header).

If it is only a convention, then is it required?

Definitely more of a convention than standard, but having a mixed set of pre-compiler constants lacks consistency and style. Is it required, that is up for debate.

tobozo · 2023-04-16T17:12:42Z

Can we (library maintainers, board vendors, end users) get a vote before such a tech debt is deferred on us or will it just happen whatever we discuss or debate?

Diako22 · 2023-05-17T01:48:21Z

Всем привет🖖 Я вас очень прошу делайте так чтобы я не ушол на покой😔
Я честно даже не знаю что делать дальше😭

VojtechBartoska · 2023-05-17T11:04:13Z

Hi all,

we have discussed this internally and also on last Community Meeting and I'm sharing here the approach we come up with.

In this PR fix just HELTEC_WIFI_32 board definition which is causing the issues
Leave other boards as they are now to not brake them on other places (libraries and so on, mentioned above by @tobozo)
Set up a rule to use only Upper case naming from now on for new additions

Please let us know if this make sense for you, we're looking forward for your feedback.
cc @atanisoft, @mrengineer7777, @FrightRisk, @brentru

tobozo · 2023-05-18T08:10:35Z

that's good news, thank you for taking the time to discuss this 👍

mrengineer7777 · 2023-05-18T14:07:28Z

1. In this PR fix just HELTEC_WIFI_32 board definition which is causing the issues

2. Leave other boards as they are now to not brake them on other places (libraries and so on, mentioned above by @tobozo)

3. Set up a rule to use only Upper case naming from now on for new additions
Please let us know if this make sense for you, we're looking forward for your feedback. cc @atanisoft, @mrengineer7777, @FrightRisk, @brentru

@VojtechBartoska I agree 100% with this approach. Who is setting up the rule to enforce Upper case naming?

VojtechBartoska · 2023-05-18T15:02:01Z

1. In this PR fix just HELTEC_WIFI_32 board definition which is causing the issues

2. Leave other boards as they are now to not brake them on other places (libraries and so on, mentioned above by @tobozo)

3. Set up a rule to use only Upper case naming from now on for new additions
Please let us know if this make sense for you, we're looking forward for your feedback. cc @atanisoft, @mrengineer7777, @FrightRisk, @brentru
@VojtechBartoska I agree 100% with this approach. Who is setting up the rule to enforce Upper case naming?

We can include it into Boards test script to check if the PR meet all the requirements.
https://github.com/espressif/arduino-esp32/blob/master/.github/workflows/boards.yml

VojtechBartoska · 2023-11-28T11:35:08Z

@P-R-O-C-H-Y up to my last comment, will you be able to add one more check to boards.yml to validate if the new board addition use Capital letters?

After this modification, we can close this PR.

brentru · 2023-11-28T15:03:58Z

Please let us know if this make sense for you, we're looking forward for your feedback.

OK with upper-case naming, enforcement and standards in this file is great to see!

me-no-dev · 2023-12-05T10:25:38Z

We can close this now as the relevant features have already been merged :)

Update boards.txt

eb01618

Fix some of the board.build entries that are lower case to make the upper case

atanisoft reviewed Mar 8, 2023

View reviewed changes

boards.txt Outdated Show resolved Hide resolved

SuGlider added this to the 2.0.8 milestone Mar 9, 2023

Update boards.txt

390e2d9

Make all build.board entries consistent with all caps and underscores instead of dashes

FrightRisk added 2 commits March 9, 2023 13:18

Update boards.txt

cc590b4

Fix spelling and remove line spacing added by mistake

Update boards.txt

cf7bc3a

Fix last 2 boards including fixing the Lionbit board entry

VojtechBartoska added the Type: 3rd party Boards label Mar 13, 2023

VojtechBartoska requested a review from P-R-O-C-H-Y March 13, 2023 12:53

P-R-O-C-H-Y mentioned this pull request Apr 3, 2023

added Bee Data Logger board #7997

Merged

me-no-dev modified the milestones: 2.0.8, 3.0.0 Apr 10, 2023

VojtechBartoska added the Resolution: Awaiting response Waiting for response of author label May 17, 2023

VojtechBartoska assigned P-R-O-C-H-Y Nov 28, 2023

This was referenced Dec 1, 2023

[CI] Add to Boards Test check if build.board is uppercase #8950

Merged

Fix Heltec boards build.board to be uppercase #8958

Merged

me-no-dev closed this Dec 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update boards.txt file to fix some of the broken board.build entries that should be upper case #7942

Update boards.txt file to fix some of the broken board.build entries that should be upper case #7942

FrightRisk commented Mar 8, 2023 •

edited by VojtechBartoska

Loading

CLAassistant commented Mar 8, 2023 •

edited

Loading

platypii commented Mar 8, 2023

mrengineer7777 commented Mar 8, 2023 •

edited

Loading

platypii commented Mar 8, 2023 •

edited

Loading

mrengineer7777 commented Mar 9, 2023

FrightRisk commented Mar 9, 2023 •

edited

Loading

FrightRisk commented Mar 9, 2023

atanisoft commented Mar 9, 2023

platypii commented Mar 9, 2023

FrightRisk commented Mar 9, 2023

atanisoft commented Mar 9, 2023

FrightRisk commented Mar 9, 2023

VojtechBartoska commented Mar 22, 2023

me-no-dev commented Mar 31, 2023

FrightRisk commented Mar 31, 2023

P-R-O-C-H-Y commented Apr 3, 2023

Tinyu-Zhao commented Apr 7, 2023

tobozo commented Apr 16, 2023

atanisoft commented Apr 16, 2023

tobozo commented Apr 16, 2023

atanisoft commented Apr 16, 2023

tobozo commented Apr 16, 2023

Diako22 commented May 17, 2023 •

edited

Loading

VojtechBartoska commented May 17, 2023 •

edited

Loading

tobozo commented May 18, 2023

mrengineer7777 commented May 18, 2023

VojtechBartoska commented May 18, 2023

VojtechBartoska commented Nov 28, 2023

brentru commented Nov 28, 2023

me-no-dev commented Dec 5, 2023

Update boards.txt file to fix some of the broken board.build entries that should be upper case #7942

Update boards.txt file to fix some of the broken board.build entries that should be upper case #7942

Conversation

FrightRisk commented Mar 8, 2023 • edited by VojtechBartoska Loading

Description of Change

Tests scenarios

Partially fixes issue

CLAassistant commented Mar 8, 2023 • edited Loading

platypii commented Mar 8, 2023

mrengineer7777 commented Mar 8, 2023 • edited Loading

platypii commented Mar 8, 2023 • edited Loading

mrengineer7777 commented Mar 9, 2023

FrightRisk commented Mar 9, 2023 • edited Loading

FrightRisk commented Mar 9, 2023

atanisoft commented Mar 9, 2023

platypii commented Mar 9, 2023

FrightRisk commented Mar 9, 2023

atanisoft commented Mar 9, 2023

FrightRisk commented Mar 9, 2023

VojtechBartoska commented Mar 22, 2023

me-no-dev commented Mar 31, 2023

FrightRisk commented Mar 31, 2023

P-R-O-C-H-Y commented Apr 3, 2023

Tinyu-Zhao commented Apr 7, 2023

tobozo commented Apr 16, 2023

atanisoft commented Apr 16, 2023

tobozo commented Apr 16, 2023

atanisoft commented Apr 16, 2023

tobozo commented Apr 16, 2023

Diako22 commented May 17, 2023 • edited Loading

VojtechBartoska commented May 17, 2023 • edited Loading

tobozo commented May 18, 2023

mrengineer7777 commented May 18, 2023

VojtechBartoska commented May 18, 2023

VojtechBartoska commented Nov 28, 2023

brentru commented Nov 28, 2023

me-no-dev commented Dec 5, 2023

FrightRisk commented Mar 8, 2023 •

edited by VojtechBartoska

Loading

CLAassistant commented Mar 8, 2023 •

edited

Loading

mrengineer7777 commented Mar 8, 2023 •

edited

Loading

platypii commented Mar 8, 2023 •

edited

Loading

FrightRisk commented Mar 9, 2023 •

edited

Loading

Diako22 commented May 17, 2023 •

edited

Loading

VojtechBartoska commented May 17, 2023 •

edited

Loading