Add build flags for global i2c & SPI pins

[ezcGman]

Hey there,

it's me again! We also don't have enough build flags, do we? :) So please let me add some more!

This PR is to make the new global i2c and SPI pins configurable via build flags. Why would I want that? Because for the QuinLED boards, we compile a build that already has everything set (like LED outputs, relay pin, IR pin, etc.) using build flags. And now in combination with my new SHT usermod (#2942), we can also provide a build with that mod included, but we currently can't set the i2c pins; users would need to do that themselves still.

With this PR, we could pre-configure everything on the boards so boards are even more Plug-n-Play:

[env:custom_i2c_pin_test]
board = esp32dev
platform = ${esp32.platform}
platform_packages = ${esp32.platform_packages}
build_unflags = ${common.build_unflags}
build_flags = ${common.build_flags_esp32}
  -D I2CSDAPIN=32
  -D I2CSCLPIN=13
  -D SPIMOSIPIN=0
  -D SPIMISOPIN=1
  -D SPISCLKPIN=2
board_build.partitions = ${esp32.default_partitions}

We only need the i2c pins, but I thought it would be stupid to not do the same for the SPI pins :)

Thanks for considering this PR and greetings,

Andy!

[softhack007]

Could it be that this already exists?

https://github.com/Aircoookie/WLED/blob/8619e8fc0bfbf97b0834384aa683e66bb1d7337a/wled00/const.h#L401-L406

https://github.com/Aircoookie/WLED/blob/8619e8fc0bfbf97b0834384aa683e66bb1d7337a/wled00/const.h#L420-L432

https://github.com/Aircoookie/WLED/blob/8619e8fc0bfbf97b0834384aa683e66bb1d7337a/wled00/wled.h#L679-L683

[ezcGman]

No, these are just helpers that always have the HW default pin per board. Each board can have different HW pins and also different const defined in the Arduino lib. Hence WLED uses these HW_* to access the default HW pin for SPI/i2c, regardless of the board it's compiled for.

Those are not even considered for these new global / usermod i2c/SPI pins, not sure why though... Maybe a better change would be instead of new pins (what I did) initialize these usermod i2c/SPI pins with these HW_* const, instead of -1 🤔

Let's see what maintainers say!

[ezcGman]

Actually no... I don't want to set the default HW pin; I want to set the usermod i2c pin specifically... Still, maybe a change would be;

• Add new build flags / consts as I did for specifically setting the usermod i2c/SPI pins
• Still, when these new consts are absent, initialize these with the HW_* consts, instead of -1

[blazoncek]

@softhack007 this actually aligns with what @ewoudwijma wanted to do with #2931.
I do agree that this approach is acceptable but will not be added to built in platformio.ini (unless @Aircoookie approves and creates such environment). This is due to the fact that people share binaries and if they lay hands on such build which will prevent them to use I2C or SPI pins we will get many support requests or unnecessary issues opened.

[softhack007]

This is due to the fact that people share binaries and if they lay hands on such build which will prevent them to use I2C or SPI pins we will get many support requests or unnecessary issues opened.

I fully agree. Still i'm not sure if a second -D I2CSCLPIN=42 is necessary, when the same information can passed to WLED already by -D HW_PIN_SCL=42.

There is only two I2C hardware units available, and only one can be used when doing a Wire.Begin(..,..); because the second one is initialized with a Wire1.Begin(..,..);.

So my suggestion would be to avoid new preprocessor defines like I2CSCLPIN but utilize the existing ones. The solution proposed in this PR - with two defines to achieve the same thing - will lead to confusion, bcause people need to use both defines (-D HW_PIN_SCL=42 -D I2CSCLPIN=42) in oder to initialize I2C properly, all tell WLED that they do want to use the I2C-hardware units.

[softhack007]

Those are not even considered for these new global / usermod i2c/SPI pins, not sure why though... Maybe a better change would be instead of new pins (what I did) initialize these usermod i2c/SPI pins with these HW_* const, instead of -1 🤔

I agree - this is problematic when you want to make a build that runs "out of the box" and utilizes all features of a specific board instantly. Another proposal could be to have one new define like -D ENABLE_I2C_AT_STARTUP. If this is set, initialize i2c_sda (and other) with whatever is in HW_PIN_SCL.

[softhack007]

Actually no... I don't want to set the default HW pin; I want to set the usermod i2c pin specifically.

This is very much the same thing. On ESP32, you can choose any GPIO as I2C pins, at the same time you only have one Wire and one Wire1 globally. So when one usermod chooses 40/41 for SCL/SDA, then all other usermods have to follow (if you want to run more than one usermod). Only exception is 4LD, which also offers "software i2C". But "software i2C" is a different beast and it causes lots of other trouble...

[blazoncek]

When you put it that way it makes sense. 👍

[ezcGman]

I already had a bigger response typed, but realised I may not have all the understandings. So I actually first want to ask a few questions, maybe I have a misunderstanding here (for less confusion, let's only focus on i2c):

1. What was (or currently is) the purpose of the HW_PIN_SCL & HW_PIN_SDA macros? Isn't it (as I guessed above) that these hold the device specific HW i2c pins?
2. What are they even used for? I don't see WLED using them somewhere? I see a few mods using them, but if we now have i2c_sda & i2c_scl, what is their meaning now?
3. Am I right, that the newly added WLED_GLOBALS i2c_sda & i2c_scl (the ones I'm setting in this PR) are specifically meant to be used only by usermods? Hence they're also set on the usermods page?
4. Why are those new WLED_GLOBALS not initialized with the HW_PIN_SCL & HW_PIN_SDA macros?

Because what I wanted to say (which I deleted because I'm not sure if I have the correct understanding), is that I think the purpose of the HW_PIN_SCL & HW_PIN_SDA macros and those new i2c_sda & i2c_scl is different, hence I think it makes sense to set them individually: Maybe I specifically want to define the i2c pins usermods should use (by setting i2c_sda & i2c_scl), but I don't want to mess with whatever the HW_PIN_SCL & HW_PIN_SDA macros do.

One comment though on this:

So when one usermod chooses 40/41 for SCL/SDA, then all other usermods have to follow

That's not true, I believe. I have been using Wire1 in a mod right now, while everything else uses Wire, because at the time I wrote it, I wasn't too experienced with i2c and didn't want to interfere with the other bus, hence I used Wire1. What do you mean with "all other have to follow"?

Thanks already!

[blazoncek]

You are correct, this area is still very much WIP.

1. yes, these are used to override default values of GPIOs assigned to HW accelerated interface (as some implenetations of controller boards use differnet GPIO layout than what is usually recommended/default by Espressif). they do not allocate (reserve, use) those GPIO in any way. their intent is informational.
2. see 1.
3. not necessarily, since I2C or SPI can be shared by many devices WLED may use them in the future
4. see 1., setting globals to -1 was intentional to prevent unnecessary GPIO allocation

You are correct with your thinking.

[ezcGman]

Thanks for the answers, Blaz!

Sooo, elaborating on your reply earlier:

I do agree that this approach is acceptable

Did you mean this / my current approach here? Or my suggestion to also initialize them with the HW_* macros? Or were you referencing the other issue you linked? 🙈 I got confused, sorry :D

but will not be added to built in platformio.ini (unless @Aircoookie approves and creates such environment).

With "added to built in platformio.ini" you mean you won't provide a build config in platform.io using them? That's totally fine; We provide our own builds / pre-flash controllers with these. It's just that we can build such a build (haha) and have factory pre-flash it to the controllers.

This is due to the fact that people share binaries and if they lay hands on such build which will prevent them to use I2C or SPI pins we will get many support requests or unnecessary issues opened.

Totally makes sense, yeah.

Thanks again!

[blazoncek]

Did you mean this / my current approach here?

Yes, introducing new set of #defines for setting i2c_sda, i2c_scl, ... is ok IMO. They may work in tandem with HW_PIN_... even though they may introduce some ambiguity. We will need to add description of what HW_PIN_... constants are used for.

With "added to built in platformio.ini" you mean you won't provide a build config in platform.io using them?

Yes, exactly that. I would encourage you (or @intermittech) and @ewoudwijma (or @srg74) to provide platformio_override.ini with approriate build environments for users that want to compile source themselves for those particular boards you support/sell. Or just binaries for those that do not compile.

[ewoudwijma]

@softhack007 and I worked together on the same topic, analysis can be found in #2931

What we did at least partly is overlapping with your ideas

Very beta coding can be found here:
https://github.com/MoonModules/WLED/tree/I2CSPI-refactor

@ezcGman , if you want you can give it a try with this bin:
WLEDMM_0.14.0.10_esp32_4MB_max.bin.zip

To see what we did best is to look at the commit diffs
MoonModules/WLED-MM@mdev...MoonModules:WLED:I2CSPI-refactor

[ezcGman]

@ewoudwijma I think what you guys do there is on a whole other level / does more than I wanted to achieve with this tiny PR. So whatever you do there might (if my PR gets merged) overwrite what I did here ;)

I was actually just looking for a way to set these pins via build flags, similar to how WLED does it for all the other pins (Relay, IR, buttons, LED outs...) :)

So I'm still hoping to get this change merged, if it's acceptable / doesn't cause issues and I bet what you are doing there might overwrite what I did here, as you spent way more thought into this than I did ;)

[softhack007]

One point is still not clear for me.
I don't see the point of having to two configurable options for the same thing, like I2CSCLPIN and HW_PIN_SCL.

The problem is, it will not work properly when I2CSCLPIN and HW_PIN_SCL have different values. I have explained before, so I'm not gonna repeat myself about that.

All you want can be achieved with setting HW_PIN_SCL, and then adding one new flag like INIT_I2C_AT_STARTUP that can be used to request initialization of global i2c_sda with HW_PIN_SCL, instead of initializing with -1 (and other i2c and spi pins alike).

[blazoncek]

One point is still not clear for me. I don't see the point of having to two configurable options for the same thing, like I2CSCLPIN and HW_PIN_SCL.

These are not the same. One assigns global i2c_sda & i2c_scl the other just defines which GPIO should be used as HW accelerated pin. This due to possible pre-allocation in current implementation.

The problem is, it will not work properly when I2CSCLPIN and HW_PIN_SCL have different values. I have explained before, so I'm not gonna repeat myself about that.

This very much depends on the implementation of software I2C driver. I am not saying we want to use it as such, but someone might.

All you want can be achieved with setting HW_PIN_SCL, and then adding one new flag like INIT_I2C_AT_STARTUP that can be used to request initialization of global i2c_sda with HW_PIN_SCL, instead of initializing with -1 (and other i2c and spi pins alike).

I do not like this idea as it is counter intuitive compared to initialisation of other GPIOs like relay, button, etc.

I would rather dump HW_PIN_SDA & HW_PIN_SCL as these are informational (and always were as such). More like helpers for usermod to decide if it uses SW or HW driver.

[ezcGman]

I don't see the point of having to two configurable options for the same thing, like I2CSCLPIN and HW_PIN_SCL.

Because they're not the same thing, see my detail questions and Blaz' answers above:

they do not allocate (reserve, use) those GPIO in any way. their intent is informational.

The problem is, it will not work properly when I2CSCLPIN and HW_PIN_SCL have different values. I have explained before, so I'm not gonna repeat myself about that.

Thanks for not providing clarity on what you mean, if it seems to be unclear to people. I don't see an issue why this wouldn't work. HW_PIN_SCL is not used anywhere at the moment and even if it would: At least on ESP32 you can have two i2c busses working simultaneously. One mod (or WLED core) could use the bus that is behind HW_PIN_SCL, a mod can use the ones behind the i2c_scl WLED GLOBAL.

[ewoudwijma]

I do not like this idea as it is counter intuitive compared to initialisation of other GPIOs like relay, button, etc.

I would rather dump HW_PIN_SDA & HW_PIN_SCL as these are informational (and always were as such). More like helpers for usermod to decide if it uses SW or HW driver.

This is what is confusing for me, build flags are normally initializing values, also for relay, button etc. Why shouldn't hw_pin values work similar. I don't see a point in making them informational only.
Besides that, we are coders so we want values to DO something. Informational only is more for politicians or lawyers 😜

[blazoncek]

I suggest that we go back to the start and re-define everything and build from there.
Let's determine what we have:

1. usermods may want to share I2C bus
2. there are 2 global instances of HW I2C bus (Wire and Wire1) both share-able.
3. WLED itself does not use I2C and would want as many GPIO pins available for LEDs
4. some controller board vendors may want to provide binaries pre-compiled for their boards (with as many pre-defined pins as possible)
5. those controller boards may have different GPIO layout than the default recommended by Espressif
6. users may accidentally delete pre-compiled configuration (stored in cfg.json) and would not want to erase flash to recover default settings (they want visual cue)
7. we want usermods to be "compatible" with each other regarding sharing of I2C
8. users may want to utilise 2 I2C buses for different devices
9. there may be usermods that can utilise I2C or SPI bus depending on the selected device type (e.g. 4 Line Display)
10. there may be composite devices that use I2C or SPI pins alongside other pins (e.g. digital microphone)

From this we may get:

1. WLED introduces global I2C pin variables (for one I2C bus only) defaulting to -1 (accommodates 1., 2., 3.)
2. we define pin override macros (#defines) to allow setting of non-default I2C pin values (accommodates 4., 5.)
3. we define constants of controller board HW wired I2C pins for UI information (accommodates 5., 6.)
4. we need to re-write/update all usermods to use new global I2C pins instead of the ones provided in usermod's settings (accommodates 7., 9.)
5. we leave second I2C bus (Wire1) for custom implementation by users (acommodates 8., 10.)

All points, except 2. & (partially) 4., have been implemented in current version and this PR adds support for 2. as I see it.

I propose to merge this PR and after that we only need to work on 4. (audio_source.h may need update to use Wire1 for the special microphone).

[ewoudwijma]

I propose to merge this PR and after that we only need to work on 4. (audio_source.h may need update to use Wire1 for the special microphone).

I am afraid this ignores some of the things I and @softhack007 are currently working on.

This introduces new build flags while we have already HW_PIN * flags (and FLD_PIN flags for FLD). I think we need to use the HW_PIN flags to implement what Is done in this PR (or rename them to I2CSDAPIN etc) but not have both.

This as been worked on (and is WIP) as described in #2931.

So I think we wait on advice of @softhack007 on this matter

[softhack007]

Hi @blazoncek, @ezcGman and @ewoudwijma,

First I'd like to apologize, as my words seemed to cause frustration and I had the feeling of not being listened to at all.
In addition to what was already said, allow me to summarize what is important from my point of view.

Current there is ongoing work to improve handling of I2C busses (and possibly SPI, too) which will be relevant for a future release of WLED, like 0.14.1. From this perspective, it is important to make good decisions already now, because everything that will be put into users hands today will become "the legacy" in 14.1 and we'll have to live with that once users made use of it. personally I am mainly working on the "low level" part of improved I2C support, while leaving it to @ewoudwijma and @blazoncek to find a good UI integration that is consistent with the overall concepts in WLED.

There are some aspects where I fully agree with what was said before:

• There should be a way to pre-define I2C pins at compile time, similar to IRPin of BtnPin. So I absolutely agree with the Intention of this PR, to have such a possibility.

• The global variables i2c_sda and i2c_scl should reflect what is defined in usermod global preferences. Values "-1" should mean "don't use I2C".

• It is true that when building boards / devices / shields, you want to have the possibility to say "these are the I2C pins you should use".

• The whole discussion about "userdefined hardware I2C PINS" is relevant for ESP32 only, because on ESP8266 there is nothing you can change, pins are fixed, this is already considered in WLED 0.14.

• personally, I would also prefer to dump HW_PIN_SDA & HW_PIN_SCL, as these have no real meaning and can only help for coloring things in the UI (see below).

The ESP32 hardware is a bit tricky, because in reality there is no "Hardware (accelerated) I2C PIN" as you would have on other arduino environment. It works completely the other way. There are two I2C units on the chip, and you have "Wire" and "Wire1" respectively. Wire.begin(pin_sda, pin_scl) assigns GPIO pins to the first HW I2C unit, and likewise does Wire1.begin(...) for the second unit. Different to other arduino environments, Wire.begin() without parameter is not a good idea, and WLED 0.14 already has a way to prevent this. And it works basically, when you only have one I2C device you want to use. For more than one device, we need architecture changes for 0.14.1.

When a usermod calls Wire.begin() without parameters, this means that - silently - the values from pins_arduino.h will be used as GPIO, no matter what is in HW_PIN_SDA etc - not really good from my POV. The other important point is, once that i2C pins were set during the very first Wire.begin(pin_sda, pin_scl), these pins become "the hardware pins". A reboot is needed to set other gpio.

This brings me to the problem i see currently, when allowing users to independently define I2CSCLPIN and HW_PIN_SCL:

• It is inconsistent to IRPin or BtnPin - we would not want IRPin + BtnPin, and at the same time have HW_IR_PIN + HW_BUTTON_PIN. Because it creates confusion and it does not make much sense.

• As the first Wire.begin(...) determines what are the "hardware PINs", effectively when I2CSDAPIN is defined, then HW_PIN_SDA becomes meaningless - as a consequence of how the ESP32 works.

• Currently I don't see that we will need these HW_I2C_PINS even if we allow software based I2C in all usermods. Most users will not treat that correctly, and we end up with broken usermods and a very inconsistent situation in pinManager - like 6 I2C_HW allocations, but the ones really used by the i2C units are still different from that.

I hope this helps to understand my concerns. To say it again - I fully agree on the needs to have a way for users to define "preferred I2C pins" of their special board/shield. I am just saying that having both I2CSCLPIN and HW_PIN_SCL (that both can be set within build_flags) is not making much sense technically - it could lead to confusion, and might makes our life harder once we impement improvements for 0.14.1.

What do you think?

[blazoncek]

Both of you are still missing the point of this PR. From my perspective it is clear and approved. Please re-read my post above.
There is no need to talk of intricacies of I2C initialisation for this PR, it does not deal with that.

Still if you want: This PR, combined with existing code in WLED (cfg.cpp, lines 294-322; set.cpp, lines 507-560) satisfies all needs for initialising I2C and SPI buses/interfaces. Once the variables i2c_sda and i2c_scl are set the interface is initialised and hardware set up. There is no need to do that anywhere else! It is consistent and global, applied at boot.

What is needed is to modify existing usermods to heed these settings and notify them if pins change from set.cpp (may need a reboot, I understand this). Nothing more!

If you and @ewoudwijma still cannot wrap your heads around HW_PIN_SDA or HW_PIN_SCL take another look at const.h, lines 395-433, where their definition is set. They are nothing more than constants with references to platform dependant or user specified values. Their purpose is to let any code identify if i2c_sda and i2c_scl is set to "user's hardware" or not. Nothing else. A visual cue for them is also available in usermods settings page if you delete characters in I2C & SPI input fields.

For clearer picture consider this:

if (i2c_sda == HW_PIN_SDA) {
  // we have SDA pin that corresponds to what is used in hardware/controller board
  ...
} else {
  // warn user that setup is not according to his/her hardware
  ...
}

@softhack007 you are comparing apples to oranges when you talk about IRPIN, etc. and HW_PIN_SDA/HW_PIN_SCL. IRPIN corresponds to I2CSCLPIN/I2CSDAPIN from this PR. HW_PIN_SDA/HW_PIN_SCL have to do with controller board design (like Dig-Octa or Wemos Shield) or hardware wiring so that any code may be aware of that. HW_PIN_... have nothing to do either with I2C interface or variable initialisation. They never did.

If you still think this is confusing I may remove all references to HW_PIN_... and replace visual cue in xml.cpp, lines 700-704, with I2CSDAPIN, etc. The problem with this approach is that you will not be able to have "help for the user" when he/she later adds I2C sensor but has a binary that does not define i2c_... variables using I2CSDAPIN (which will be stock with WLED.) So build environments like [env:wemos_shield_esp32] or (currently inexistent) [env:dig_octa] etc. will not be able to help users setting appropriate pins for their hardware.

[softhack007]

Hi @blazoncek If you still think I'm talking gibberish and I don't know what I'm saying, then maybe we have a bigger problem
and let's discuss privately on discord.

It seems my opinion is to "brainy" or something, so I'll shut up for now and will stop making life miserable by comparing apples to oranges.

Proceed as you prefer. I'll concentrate on MoonModules, as I don't want to get anybody into a situation where they fear of losing their faces, just because I dared to challenge their design choices.

If you want, contact me on discord. Finito.

[softhack007]

Just in case anybody here was concerned about yesterday's disussion - all good. @blazoncek and myself agreed on a compromise that allows to merge this PR without possible problems later, and comments will be added to explain the purpose of new build flags. So approved from my side 👍

[ezcGman]

Thanks guys! And I feel a little bad for kicking off this heated discussion here <3

[softhack007]

And I feel a little bad for kicking off this heated discussion here <3

Don't worry. From my perspective this discussion was due any way, so you just gave a spark, but any other "spark" could have caused the same. What's important is that we were able to find a good way forward.

[MoonModules]

I looked at latest dev:

Is it correct HW_PIN_SCL should now only providing info ?

If so, I see in code it is used to provide values and allocate pins with it. Is this correct?

and should I2CSCLPIN be added to platform io entries where now HW_PIN_SCL is used?

Same question for HW_PIN_CLOCKSPI and a bunch of other HW_PIN values

[blazoncek]

Yes, this is all correct.

EDIT: As noted above I2CSDAPIN & co. will not be added to platformio.ini because their purpose is different, as stated above.

[ewoudwijma]

If all my statements are correct then it contradicts with the note you added. There is a lot written above.
I am confused still. Can you maybe summarize the relevant texts above to get a better understanding?

[blazoncek]

Defining I2CSDAPIN will assign i2c_sda (and consequentially allocate GPIO using PinManager). It will also define HW_PIN_SDA if not defined explicitly.
Defining HW_PIN_SDA will only inform code what is hardwired SDA GPIO. If left undefined platform specific constants are used instead of user specified value. i2c_sda will remain unassigned (-1) and no allocation is done.

All code should use i2c_sda whenever dealing with GPIO pins, and use HW_PIN_SDA in comparisons to detect if user entered HW accelerated/assisted GPIO. No user code should ever use I2CSDAPIN anywhere (the only exception is wled.h).

Some usermods will require updates.