firmata / arduino

Firmata firmware for Arduino
GNU Lesser General Public License v2.1
1.53k stars 515 forks source link

[WiFi] added support for ESP32 architecture and XIAO ESP32C3 board #512

Open jnsbyr opened 6 months ago

jnsbyr commented 6 months ago

features:

notes:

zfields commented 6 months ago

I appreciate the hard work. It would be WAY better if you would break this down into multiple PRs (e.g. one for the addition of the Seed Studio XIAO ESP32C3, another to add Wi-Fi support, and yet another to add servo support for ESP32).

It will be MUCH easier to review, understand, rework and merge.

jnsbyr commented 6 months ago

Breaking down this PR does not seem conclusive for me. It may look like a mulit-feature commit, but it is simply not possible to add the ESP32 architecture and the XIAO board without making all these changes at the same time. The example will simply not build for the new board.

If breaking down is a requirement to continue I suggest 3 parts:

pgrawehr commented 6 months ago

@jnsbyr Are you aware that ConfigurableFirmata already has all of this, including PWM? The only thing that might be missing is the XIAO board definitions, but that's the easiest part here.

jnsbyr commented 6 months ago

@pgrawehr

Are you aware that ConfigurableFirmata already has all of this, including PWM? The only thing that might be missing is the XIAO board definitions, but that's the easiest part here.

Thanks for the info. I did not look, but ConfigurableFirmata was always a little more progressive, so no surprise here.

My history with this PR is quite simple. I have a working project for an ESP8266 board, but I needed an external antenna and I had an unused XIAO ESP32C3 in my box. Initially I thought adding the board definition should be all it needs. Took a few hours more than expected, but once I started there was no reason for me to look for alternatives ;-)

jnsbyr commented 6 months ago

also:

todo:

jnsbyr commented 6 months ago

Did another code review and several tests, see latest 2 commits.

jnsbyr commented 6 months ago

Tried to define a generic board for the chip ESP32-C3 as fallback, as there are so may board variants, but it seems to me that the Arduino build system does not have a chip specific define. ARDUINO=10819 and ARDUINO_ARCH_ESP32 and ESP32 are more or less equivalent, and most of the remaining defines refer to a specific board.

It does not make sense to define a generic board for the ESP32 using absolute pin values. E.g. the ESP32-C3 has 22 GPIOs and the ESP32-S3 has 47 GPIOs.

Best option I see is to use the common constants from variants/../pins_arduino.h (e.g. NUM_DIGITAL_PINS) and to leave some slack.

jnsbyr commented 6 months ago

Notes:

A) Examples

With the synced examples it should be possible to use StandardFirmata and StandardFirmataPlus directly with the ESP32. StandardFirmataBLE needs additional work, but I do not have the setup to provide it. StandardFirmataEthernet will probably work with the Ethernet modules that are already supported, but for the Ethernet chip on the WT32-ETH01 additional changes are needed.

There are 7 other examples not staring with "Standard..." that have not been changed so far.

B) Generic ESP32 board

Defining a generic board for the ESP32 like ConfigurableFirmata does makes sense as there are too many ESP32 board variants around to add and maintain separately.

On the other hand the generic definition in ConfigurableFirmata 3.2.0 does not take into account that there is a difference in pin count and pin function between ESP32(S1), ESP32S2/3 and ESP32C3. This is something that should be revised in ConfigurableFirmata.

C) Digital pins default to output mode

Additionally any board can have manufacturer or user specific mods. This can be problematic, as StandardFirmata examples forces all non-analog digital pins to output mode in systemResetCallback(), while the board may require an input only configuration for the same pin to avoid damage. Defining board specific exceptions in ignorePins() is error prone and hard to maintain. A CPU typically configures input mode on all pins on reset so there is no good reason for Firmata to do differently. This is the first thing I change when starting a new Firmata projects to keep my hardware healthy. This should be considered as a separate change request.