-
Notifications
You must be signed in to change notification settings - Fork 267
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Library does not work properly on ESP32 S2 - fix provided. #219
Comments
@tchilton For the sake of discussing your suggested code changes, please provide them as a PR. |
Why not GPIO0 for ESP32S2? Etc. |
I don't know that |
These are the correct definitions from the manufacturer (Espressif). You can find the definitions as part of the broader chip config in This is based on a fresh build fully patched Windows 10 machine, fresh install of Arduino framework 1.8.15, then an install of the ESP32 2.0.0-alpha-1 direct from Espressif. You can also prove they are defined correctly by dropping in this pre-processor chunk into an empty application, select a chip family and compile, you will get the warning for each type you select. #ifdef CONFIG_IDF_TARGET_ESP32 This is far better than using the Arduino board directives since you would have to constantly keep adding definitions for every new board that gets added to the framework, hence the above is far more robust and easier to maintain, plus it works. |
The sources I used were diagrams direct from Espressif and contain the exact same data as the datasheets, just in an easier and quicker to digest form. see In your initial definitions, you mapped all pins on the ESP32 except for the FLASH SPI bus and the Input only pins. I cloned the same logic. There are three groups to note on the ESP32 S2
The same logic also applies for the ESP32 device. I'd never use BOOT or any of the config pins for IO for the same reason On the ESP32 C3, there are 4 strapping pins defined. In regards to the point about silent failures / dead code. I couldn't determine why the library wasn't working as expected when I used it. As I stated, it looked like dead code as nothing worked and there was no error info coming back to me, so I had to spin up an ESP32 DevKit1 board and I tried again with that and got it going, so I then knew that the code could work. That's when I got into the more detailed analysis . Bear in mind that the Espressif code for ESP32 is not yet out of beta, so it could equally have been some framework related issue that I was encountering. Once I started pulling the library around I found the problem. I had used pins 33 and 34 on the ESP32 S2 and the pin checking logic had silently failed them. Once that was understood, it was trivial to fix the code per the submission above. My suggestion is on improving that experience, either by not having the checks done in code and documenting it in the readme, since we should be able to assume that people read the info on the library and understand the chip well enough for the intended tasks and most other libraries do not do this level of checking. Alternately, if you want to keep it, then given that the begin function has a void type and hence doesn't return any error value and the isValid* functions are all private, there is no easy way to check the config and if it doesn't work, you have to go deeper than many users would probably want to. Other than broad alignment to the standard hardware based serial config, I guess there is no reason why the begin function has to return a void, it could return positive values for error allowing for you to do : ... if (! mySerial.begin(blah...)) { After all, the constructor has different constants and calling arguments already, so this would be a logical extension and results in tidy code. |
@tchilton You forget about |
Thanks. will bool the ::begin. Working on the pull request next. More than happy on the open source contributions side of things. |
That's not what I meant, tho - there is an I forgot about GPIO0, again, it seems to me that the images you refer to are not quite as expressive as the docs I linked, because I cannot see anything that says GPIO0 should not be used on the S2 or C3, whatever. Just stick to the PDFs, please! |
OK, so I just learned something - thanks. If only I'd know that had existed that would have saved a lot of head scratching. Definitely one for the documentation somewhere.
So, using the bool operator, you can see if the object is good or not. GPIO0: I'll leave the older ESP32 device out of any changes and leave it for you to decide what you want to do on that. GPIO0, GPIO2, GPIO5 and GPIO15 are the strapping pins on that family of devices For completeness, the Single page pinout for ESP32 Devkit C can be found at Now, time to get back to that pull request. |
Pull request raised as requested. |
Resolved via #221 which was merged upstream. |
Nice code - thanks, but there are several related problems here that have driven me into a deep dive to fix them.
The readme is not clear on which versions of the ESP32 are supported - it does not state ESP32 S2 or ESP C3, but there are bugfixes in for C3. Please update the readme to more clearly show which CPU versions are supported.
Getting the code to compile - as others have reported, using the stock ESP32 board definitions via Arduino board manager, which is currently listed as in alpha, results in compile errors with `__atomic_exchange_1' type messages.
The solution to this is to update to the latest ESP32/ESP32 S2 code from Expresif's website as detailed in
(https://docs.espressif.com/projects/esp-idf/en/latest/esp32s2/api-guides/tools/idf-tools.html#xtensa-esp32s2-elf) and clarified in the article at (https://www.mischianti.org/2020/12/01/esp32-s2-pinout-specs-and-arduino-ide-configuration-1/). Note the change from the 2020 r3 to 2021 r1 code and the version 3.1 of esptools. Once this is correctly applied, the code will compile properly.
The fixes need to make ESP32 S2 and probably ESP C3 (I've not got one to test with) usable on all the mapped out pins are as follows, The definitions are from the Expresif IDF and not standard to Arduino and the links to the vendors module pinouts are listed below.
bool SoftwareSerial::isValidGPIOpin(int8_t pin) {
#if defined(ESP8266)
return (pin >= 0 && pin <= 16) && !isFlashInterfacePin(pin);
#elif defined(ESP32)
#ifdef CONFIG_IDF_TARGET_ESP32
return (pin >= 0 && pin <= 5) || (pin >= 12 && pin <= 19) ||
(pin >= 21 && pin <= 23) || (pin >= 25 && pin <= 27) || (pin >= 32 && pin <= 39);
#elif CONFIG_IDF_TARGET_ESP32S2
// See [https://docs.espressif.com/projects/esp-idf/en/latest/esp32s2/_images/esp32-s2_saola1-pinout.jpg]
return (pin >= 1 && pin <= 21) || (pin >= 33 && pin <= 44);
#elif CONFIG_IDF_TARGET_ESP32C3
// See [https://docs.espressif.com/projects/esp-idf/en/latest/esp32c3/_images/esp32-c3-devkitm-1-v1-pinout.jpg]
return (pin >= 0 && pin <= 1) || (pin >= 3 && pin <= 7) || (pin >= 18 && pin <= 21);
#else
return true;
#endif
#else
return true;
#endif
}
bool SoftwareSerial::isValidTxGPIOpin(int8_t pin) {
return isValidGPIOpin(pin)
#if defined(ESP32)
#ifdef CONFIG_IDF_TARGET_ESP32
&& (pin < 34)
#elif CONFIG_IDF_TARGET_ESP32S2
&& (pin < 45)
#elif CONFIG_IDF_TARGET_ESP32C3
// Nothing to do
#endif
#endif
;
}
Once the above are all applied, the code compiles and works properly and I've had it running successfully at 115200 baud.
Thinking a little further on this, is it really necessary to have the pin restrictions in the code - I know this is done for good intention to stop people tripping over the pins with special functions or limitations, but could this not be better covered in the documentation, rather than a silent error that is even more difficult to diagnose ?
The text was updated successfully, but these errors were encountered: