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

jnsbyr · 2023-12-18T22:45:35Z

features:

support for ESP32 architecture with WiFi interface (WiFiStream, Firmata.h, StandardFirmataWiFi)
support for Seed Studio XIAO ESP32C3 board (Boards.h)
support for Firmata applications without Servo.h (Boards.h, StandardFirmataWiFi)

notes:

new library version should be fully backward compatible with existing Firmata applications and previous architectures and boards (not tested)
new library requires code changes in existing Firmata applications when used with ESP32 architecture, see modifications of example StandardFirmataWiFi:
- The occurrences of the constants INPUT and OUTPUT have to be replaced with PIN_MODE_INPUT/PIN_MODE_OUTPUT when used in Firmata context, as the ESP32 SDK does not use the same constant values as e.g. the Atmel SDK. This fixes a long standing inconsistency that can be found in (all?) Firmata examples, probable due to historic reasons.
- Servo.h will not be included, because it is not provided by ESP32 SDK. A skeleton definitions had to be added to Boards.h to allow building StandardFirmataWiFi.
- Method detachServo() had to be modified to support MAX_SERVOS=0.
- Introduced new define DEFAULT_ANALOG_RESOLUTION, as the ESP32 SDK defaults to 12 bits.
- Only build diagnostic code in method printWifiStatus() if SERIAL_DEBUG is defined to avoid compiler warning.
PWM support not yet implemented, as the ESP32 SDK does not support analogWrite(). A possible solution could be a set of preprocessor defines for pinMode() and analogWrite(). Give a thumbs up for me to continue or suggest alternative approach.

zfields · 2023-12-18T23:30:21Z

Boards.h

@@ -29,7 +30,16 @@
 // compile, but without support for any Servos.  Hopefully that's what the
 // user intended by not including Servo.h
 #ifndef MAX_SERVOS
-#define MAX_SERVOS 0
+  #define MAX_SERVOS 0
+  class Servo


This is not the appropriate place to define a class.

Maybe. I dont' want this class at all. The source comments give the impression that the Firmata application can do without servos, but than the examples do not build. Adding a new Servo.h to the library itself is also not a good idea because it will create conflicts.
So where put it? Into each example?

In the example, is it not possible to include ESP32Servo.h?

In theory, you could use the same switch as you are using elsewhere (i.e. ARDUINO_ARCH_ESP32).

I already considered using ESP32Servo.h. On first glance it should be compatible, but I did not try so far and I do not have the hardware to verify.

Problem I see here: it's an extra library that needs to be installed separately. So most users will choose the Firmata example, build and get a fail. Only some will read the docs and add the library first. It would be preferable to have the example work out of the box without this extra step and make it still an option. To do this the Servo.h include should be disabled and that can be done with the ARDUINO_ARCH_ESP32 define in the Firmata examples. The result could look like this:

_
#ifdef ARDUINO_ARCH_ESP32
// comment in if ESP32Servo library is installed and servos are required
//#include <ESP32Servo.h>
#else
#include <Servo.h>
#endif
_

What remains to do is to find a solution for the example code that depends on Servo.h. I know this is not ConfigurableFirmata, but previously I had to remove the servo code manually from the INO to make it work without (especially to save codespace for other purposes on small MCUs like the Atmega 328P). Here I see 2 options:

use define ARDUINO_ARCH_ESP32
advantage: compact binary
disadvantage: ugly because several code blocks must be taken out

use skeleton for Servo class, could be a file name FirmataServoSkeleton.h
disadvantage: less compact binary

Problem I see here: it's an extra library that needs to be installed separately.

There is already a precedent for this behavior, because the regular sample requires the standard Servo library to be installed.

Please remove the class definition and proceed with the following:

#ifdef ARDUINO_ARCH_ESP32 #include <ESP32Servo.h> #else #include <Servo.h> #endif

Done, see latest commit. I considered using a preprocessor check "#if __has_include("ESP32Servo.h)" to create a more descriptive error message if the library was not added, but this check is not a C standard and may cause problems.

@jnsbyr You can add a library dependency by adding a depends line to library.properties, as done here: https://github.com/firmata/ConfigurableFirmata/blob/master/library.properties Then the library manager should automatically grab the dependency.

zfields · 2023-12-18T23:32:45Z

utility/WiFiStream.h

+  /**
+   * check if TCP connection is established
+   */
+  inline uint8_t connected()


Why are you creating an ESP32 specific API?

The ESP32 SDK does not provide the same functionality as the ESP8266 SDK. For some use cases it is still relevant to know if the stream is connected, as this is not the same as checking the WiFi status. One alternative would be to mimic the ESP8266 implementation and create a status value from the bool provided by the ESP32 SDK.

I think that's the way to go, especially since there is precedent.

I think that's the way to go.

You refer to the alternative I mentioned (to use the previous interface)?

Done, see latest commit. Also added an enum for the possible return values of method status() as the ESP32 SDK does not define constants like the ESP8266 SDK does.

zfields · 2023-12-18T23:39:22Z

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.

- support for ESP32 architecture with WiFi interface (WiFiStream, Firmata.h, StandardFirmataWiFi) - support for Seed Studio XIAO ESP32C3 board (Board.h) - support for Firmata applications without Servo.h (Boards.h, StandardFirmataWiFi)

jnsbyr · 2023-12-19T00:20:41Z

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:

doing without Servo.h (need recommendation how to implement, see comment above)
fixing usage of INPUT and OUTPUT
adding ESP32 / XIAO ESP32C3

pgrawehr · 2023-12-19T07:11:58Z

@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.

zfields · 2023-12-19T13:34:57Z

Firmata.h

+#ifndef ARDUINO_ARCH_ESP32
+  #define ANALOG                0x02 // same as PIN_MODE_ANALOG
+#endif


Can you explain why this is necessary again?

Maybe we can figure out a creative solution to work around your need. My fear is that you are going to break backward compatibility with this change.

I had to do a similar change in ConfigurableFirmata, as the problem is that there's a conflicting definition of ANALOG (as well as INPUT and OUTPUT) in the ESP32 headers. IIRC, the values don't even match, so setting the pin mode could strangely fail, depending on which of the definitions is in scope at the calling place. But I think the right solution is to call this PIN_MODE_ANALOG, to avoid further ambiguities.

Does PIN_MODE_ANALOG come from Arduino.h?

Oh, I see it. It's in FirmataDefines.h. It seems like the right move is to move away from ANALOG and deprecate it altogether, as we did for INPUT and OUTPUT.

jnsbyr · 2023-12-19T23:35:47Z

@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 ;-)

…ted()

jnsbyr · 2023-12-21T19:55:38Z

also:

renamed DEFAULT_ANALOG_RESOLUTION to DEFAULT_ADC_RESOLUTION to make it compatible to the solution for ConfigurableFirmata from @pgrawehr
added another ESP32 board: W32-ETH01 (to be able to continue developing and testing)

todo:

Find a solution for the ESP32 PWM implementation (currently disabled):
The solution in ConfigurableFirmata is using the modular design to provide an implementation that is specific for the ESP32 SDK. This approach cannot be applied directly to StandardFirmata.
My suggestion: define macros in Boards.h h for the relevant functions (init, write), either with the necessary code directly in Boards.h or in separate header file like utility/PwmImplementation.h.

zfields · 2023-12-22T17:15:03Z

examples/StandardFirmataWiFi/StandardFirmataWiFi.ino

@@ -871,6 +881,7 @@ void printWifiStatus() {
    DEBUG_PRINT( "WiFi connection failed. Status value: " );
    DEBUG_PRINTLN( WiFi.status() );
  }
+#ifdef SERIAL_DEBUG


What is this doing for us? How does SERIAL_DEBUG get defined?

SERIAL_DEBUG can be found in StandardFirmataWifi.ino:90. If not enabled with ESP32 SDK (default) the compiler will raise a warning or an error (depending on the compiler settings) because of the unused local variables like "rssi" in line 898, caused by defining DEBUG_PRINT to nothing (see utility/firmataDebug.h).

zfields · 2023-12-22T17:34:07Z

utility/WiFiStream.h

+#if ESP8266 || ESP32
+  enum ConnectionStatus { STATUS_CLOSED = 0,
+                          STATUS_ESTABLISHED = 4
+                        };
+


Forgive me, I'm not familiar with the ESP8266 and ESP32 Wi-Fi libraries. Can you please explain the enum a little bit better?

Using this enum and the logic below, it looks like you are translating a boolean into values from the ESP8266 enum.

If that's correct, then I don't understand why you would want to redefine these values for ESP8266.

Shouldn't the enum only be defined under #if ESP32?

Suggested change

#if ESP8266 || ESP32

enum ConnectionStatus { STATUS_CLOSED = 0,

STATUS_ESTABLISHED = 4

};

#if ESP8266 || ESP32

#if ESP32

enum ConnectionStatus {

STATUS_CLOSED = 0,

STATUS_ESTABLISHED = 4

};

#endif // ESP32

The ESP8266 SDK defines all the constants listed in the method description, but they do not exists with the same definition in the ESP32 SDK, if at all (they are related to the low level TCP socket implementation).

So similar how the Firmata library handles the PIN_MODE_XXX definitions I defined the enum to have the same definitions available, regardless which SDK is used. It would be possible to add all the constants known in the ESSP8266 SDK, but most of the other are typically of no practical value, so I added only the one that are available in both SDKs.

I hope that I got your initial idea right by implementing the status() method instead of the new connected() method to avoid having a platform specific solution. If not, just let me know.

Yes, I generally like your latest implementation. 👍

However, I'm concerned that you are "redefining" the symbols STATUS_CLOSED and STATUS_ESTABLISHED for the ESP8266 platform. They are already defined in the ESP8266 Wi-Fi library, no?

STATUS_CLOSED and STATUS_ESTABLISHED are enum values and are part of the WiFiStream class namespace of Firmata.

On the other hand CLOSED, LISTEN, etc. are defined as enum values in lwip/tcp.h of the ESP8266 SDK in the global namespace. So the two sets of constants have unique names and in the case oft ESP8266 SDK the same values. To make the code more robust to changes in the ESP8266 SKD one could use a switch:

inline uint8_t status() { #ifdef ESP8266 uint8 cs = _client.status(); switch (cs) { case CLOSED: cs = STATUS_CLOSED; break; case ESTABLISHED: cs = STATUS_ESTABLISHED; break; default: break; } return cs; #elif ESP32 return _client.connected()? STATUS_ESTABLISHED : STATUS_CLOSED; #endif }

jnsbyr · 2023-12-23T22:15:34Z

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

jnsbyr · 2023-12-23T23:04:16Z

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.

…T32_ETH01, fixed TOTAL_ANALOG_PINS, IS_PIN_DIGITAL IS_PIN_ANALOG for ARDUINO_WT32_ETH01

…a explicit for ESP8266

jnsbyr · 2024-01-03T19:32:49Z

made WiFiStream::status const mapping explicit
synced example code
protected SPI flash pins in generic ESP32 board definition

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.

jnsbyr changed the title ~~[WiFi] added support for ESP32 architecture and XIAO ESP32C2 board~~ [WiFi] added support for ESP32 architecture and XIAO ESP32C3 board Dec 18, 2023

zfields requested changes Dec 18, 2023

View reviewed changes

jnsbyr force-pushed the feature/xiao-esp32c2 branch from a3fe862 to 7d6ada1 Compare December 19, 2023 00:06

zfields requested changes Dec 19, 2023

View reviewed changes

jnsbyr added 4 commits December 21, 2023 20:30

WiFiStream: implement method status() instead of adding method connec…

7a6dd82

…ted()

StandardFirmataWiFi: include library ESP32Servo

9651a7c

Boards.h: renamed DEFAULT_ANALOG_RESOLUTION to DEFAULT_ADC_RESOLUTION

dde9fd9

added board WT32-ETH01 (ESP32-S1)

ad79f15

zfields requested changes Dec 22, 2023

View reviewed changes

jnsbyr added 2 commits December 23, 2023 22:49

Boards.h: fixed PIN_TO_DIGITAL for board ARDUINO_XIAO_ESP32C3

4567db4

StandardFirmataWiFi: added error check and debug output for ESP32_WIFI

08325d1

jnsbyr added 5 commits December 24, 2023 00:27

Boards.h: enabled PIN_TO_SERVO for ARDUINO_XIAO_ESP32C3 and ARDUINO_W…

af2e9aa

…T32_ETH01, fixed TOTAL_ANALOG_PINS, IS_PIN_DIGITAL IS_PIN_ANALOG for ARDUINO_WT32_ETH01

Boards.h: added generic board defintion for ESP32

90ac082

Boards.h: exclude SPI flash pins for generic ESP32 board

44a8788

synced changes in StandardFirmataWiFi to all examples

b3a78d5

WiFiStream: make connection status const remapping from SDK to Firmat…

d072c0a

…a explicit for ESP8266

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jnsbyr commented Dec 18, 2023 •

edited

Loading

zfields Dec 18, 2023

jnsbyr Dec 19, 2023

zfields Dec 19, 2023

jnsbyr Dec 19, 2023

zfields Dec 20, 2023

jnsbyr Dec 21, 2023

pgrawehr Dec 22, 2023

zfields Dec 18, 2023

jnsbyr Dec 19, 2023

zfields Dec 19, 2023

jnsbyr Dec 19, 2023

jnsbyr Dec 21, 2023

zfields commented Dec 18, 2023 •

edited

Loading

jnsbyr commented Dec 19, 2023 •

edited

Loading

pgrawehr commented Dec 19, 2023

zfields Dec 19, 2023

pgrawehr Dec 19, 2023

zfields Dec 19, 2023

zfields Dec 19, 2023

jnsbyr commented Dec 19, 2023

jnsbyr commented Dec 21, 2023

zfields Dec 22, 2023

jnsbyr Dec 22, 2023

zfields Dec 22, 2023

jnsbyr Dec 22, 2023

zfields Dec 22, 2023

jnsbyr Dec 23, 2023

jnsbyr commented Dec 23, 2023

jnsbyr commented Dec 23, 2023

jnsbyr commented Jan 3, 2024

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

Are you sure you want to change the base?

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

Conversation

jnsbyr commented Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zfields commented Dec 18, 2023 • edited Loading

jnsbyr commented Dec 19, 2023 • edited Loading

pgrawehr commented Dec 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnsbyr commented Dec 19, 2023

jnsbyr commented Dec 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnsbyr commented Dec 23, 2023

jnsbyr commented Dec 23, 2023

jnsbyr commented Jan 3, 2024

jnsbyr commented Dec 18, 2023 •

edited

Loading

zfields commented Dec 18, 2023 •

edited

Loading

jnsbyr commented Dec 19, 2023 •

edited

Loading