espressif / arduino-esp32

Arduino core for the ESP32

GNU Lesser General Public License v2.1

13.31k stars 7.35k forks source link

'TX' / 'RX' Defns in 'pins_arduino.h' Variant Files Appear Unused #1607

Closed mrwgx3 closed 5 years ago

mrwgx3 commented 6 years ago

Description

While researching issue (#1577), I noticed that the RX and TX definitions within ['pins_arduino.h' variant file for the Adafruit ESP32 Feather][txrx_feather] were different from all the others defined in the 'variants' folder:

static const uint8_t TX = 17;    // Esp32 Feather
static const uint8_t RX = 16;

versus

static const uint8_t TX = 1;     // All the rest
static const uint8_t RX = 3;

This made me curious on how RX and TX were used, hence I search the ESP32 repository for all their occurances. To my surprise, I found no usage of either TX or RX in any active code, only their definitions in the 'variant' files.

If this is confirmed, should the RX/TX pin definitions be depreciated or even removed from the 'pins_arduio.h' variant files?

[txrx_feather]: https://github.com/espressif/arduino-esp32/blob/master/variants/feather_esp32/pins_arduino.h#L17#L18

stickbreaker commented 6 years ago

@mrwgx3 you have found a bug. HardwareSerial.begin() needs to be fixed.

Chuck.

mrwgx3 commented 6 years ago

@stickbreaker, I propose that the following changes be made to resolve the disconnect between the variant files and HardwareSerial.begin():

For every 'pins_arduino.h' variant file, we replace the RX/TX definition with the following default extended version.

Replace

static const uint8_t TX = 1;
static const uint8_t RX = 3;

with

// Default physical UART / TX pin / RX pin assignments
// UARTA = 0   TXA =  1   RXA =  3
// UARTB = 1   TXB = 10   RXB =  9
// UARTC = 2   TXC = 17   RXC = 16

static const uint8_t UARTA =  0;     // Default 'Serial'
static const uint8_t   TXA =  1;
static const uint8_t   RXA =  3;

static const uint8_t UARTB =  1;     // Default 'Serial1'
static const uint8_t   TXB = 10;
static const uint8_t   RXB =  9;

static const uint8_t UARTC =  2;     // Default 'Serial2'
static const uint8_t   TXC = 17;
static const uint8_t   RXC = 16;

Note that the TX/RX definitions paired with each physical UART are the pins required for enhanced digital mode, the same ones used in the original HardwareSerial.begin().

The following changes are made to ../esp32/cores/esp32/HardwareSerial.cpp

1) Replace

#if !defined(NO_GLOBAL_INSTANCES) && !defined(NO_GLOBAL_SERIAL)
HardwareSerial Serial(0);
HardwareSerial Serial1(1);
HardwareSerial Serial2(2);
#endif

with

#if !defined(NO_GLOBAL_INSTANCES) && !defined(NO_GLOBAL_SERIAL)
HardwareSerial Serial ( UARTA );
HardwareSerial Serial1( UARTB );
HardwareSerial Serial2( UARTC );
#endif

to allow pairing of a specific physical UART with a given 'Serial' instance.

2) Replace the original HardwareSerial.begin()

void HardwareSerial::begin(unsigned long baud, uint32_t config, int8_t rxPin, int8_t txPin, bool invert)
{
    if(0 > _uart_nr || _uart_nr > 2) {
        log_e("Serial number is invalid, please use 0, 1 or 2");
        return;
    }
    if(_uart) {
        end();
    }
    if(_uart_nr == 0 && rxPin < 0 && txPin < 0) {
        rxPin = 3;
        txPin = 1;
    }
    if(_uart_nr == 1 && rxPin < 0 && txPin < 0) {
        rxPin = 9;
        txPin = 10;
    }
    if(_uart_nr == 2 && rxPin < 0 && txPin < 0) {
        rxPin = 16;
        txPin = 17;
    }
    _uart = uartBegin(_uart_nr, baud, config, rxPin, txPin, 256, invert);
}

with

void HardwareSerial::begin(unsigned long baud, uint32_t config, int8_t rxPin, int8_t txPin, bool invert)
{
    if(0 > _uart_nr || _uart_nr > 2) {
        log_e("Serial number is invalid, please use 0, 1 or 2");
        return;
    }
    if(_uart) {
        end();
    }
    if(_uart_nr == UARTA && rxPin < 0 && txPin < 0) {
        rxPin = RXA;
        txPin = TXA;
    }
    if(_uart_nr == UARTB && rxPin < 0 && txPin < 0) {
        rxPin = RXB;
        txPin = TXB;
    }
    if(_uart_nr == UARTC && rxPin < 0 && txPin < 0) {
        rxPin = RXC;
        txPin = TXC;
    }
    _uart = uartBegin(_uart_nr, baud, config, rxPin, txPin, 256, invert);
}

to re-connect the symbolic constants in the variant 'pins_arduino.h' files.

@ladyada, we can now pair the global 'Serial1' instance with physical UART2 within the 'feather_esp32' variant 'pins_arduino.h' file, and no longer need to modify any of the 'Adafruit_GPS' files.

Replace

static const uint8_t TX = 17;
static const uint8_t RX = 16;

with

// Default physical UART / TX pin / RX pin assignments
// UARTA = 0   TXA =  1   RXA =  3
// UARTB = 1   TXB = 10   RXB =  9
// UARTC = 2   TXC = 17   RXC = 16

static const uint8_t UARTA =  0;   // Default 'Serial'
static const uint8_t   TXA =  1;
static const uint8_t   RXA =  3;

static const uint8_t UARTB =  2;   // Non-default 'Serial1'
static const uint8_t   TXB = 17;
static const uint8_t   RXB = 16;

static const uint8_t UARTC =  1;   // Non-default 'Serial2'
static const uint8_t   TXC = 10;
static const uint8_t   RXC =  9;

@me-no-dev, please refer to [Fix TelnetToSerial sketch][fix_tele]

If the following changes are accepted, you'll have to revert and modify the original 'Serial1' instance to some unique name then initialize with the long form of HardwareSerial.begin() in order to insure the original Uart/Pin pairing.

[fix_tele]: https://github.com/espressif/arduino-esp32/commit/659c8ad528f806c3eba564a671d465c263de4944

ladyada commented 6 years ago

what the arduino SAMD variant structure uses is the best we've seen so far, you can define the # of interfaces and their pins

https://github.com/adafruit/ArduinoCore-samd/blob/master/variants/circuitplay/variant.h#L111

see the whole thing including how Serial1 gets definied!

mrwgx3 commented 6 years ago

The essence of the problem here is getting sketches which use 'Serial1' as an auxilary serial-port to work with processors that have either fixed or mutable hardware interfaces. For the ESP32 case, we have (3) physical UARTs to pick from, which requires an additional degree-of-freedom (DOF) to be put somewhere.

@ladyada, I do agree with you that the 'SAMD variant structure' is very well organized and could provide the necessary DOF's required. Implementing and testing it for the ESP32, however, may take considerable effort not lightly expended.

The proposed solution, although much less comprehensive, would get the job done, as would conditional compilations added to the affected sketches.

ladyada commented 6 years ago

im totally into Getting The Job Done :)

me-no-dev commented 6 years ago

I do not want to put 9 same extra lines in all pins_arduino.h files. Let's find a "better" solution on how to do this. And TX and RX should stay (maybe as defines to RXA/TXA or something)

mrwgx3 commented 6 years ago

@me-no-dev and @ladyada

Here is the requested "better" solution...

For 'pins_arduino.h', additional serial port definitions are specified by giving a UART number with a '#define', which enables a conditional compilation in 'HardwareSerial.cpp'. Serial port defines are now added as needed:

static const uint8_t TX = 1;     // Always present, UARTA = UART0, assigned to 'Serial'
static const uint8_t RX = 3;

#define UARTB  2                 // Conditional compilation
static const uint8_t TXB = 17;   // UARTB = UART2 assigned to 'Serial1'
static const uint8_t RXB = 16;   // using these pin defaults

#define UARTC  1                 // Conditional compilation
static const uint8_t TXC =  9;   // UARTC = UART1 assigned to 'Serial2'
static const uint8_t RXC = 10;   // using these pin defaults

Here are the conditional compiles which setup the 'Serial', 'Serial1', and 'Serial2' instances:

#include "HardwareSerial.h"
#include "pins_arduino.h"

#if !defined(NO_GLOBAL_INSTANCES) && !defined(NO_GLOBAL_SERIAL)

  HardwareSerial Serial(0);   // Always present, always UART0

  // Check for auxilary uart definitions
  #if  defined(UARTB) && ((UARTB == 1) || (UARTB == 2))
     #define UARTB_OK
  #endif

  #if  defined(UARTC) && ((UARTC == 1) || (UARTC == 2))
     #define UARTC_OK
  #endif

  // Default 'Serial1' and 'Serial2'
  #if  !defined(UARTB_OK) && !defined(UARTC_OK)
  HardwareSerial  Serial1(1);
  HardwareSerial  Serial2(2);

  // UARTB assigned to 'Serial1'
  #elif   defined(UARTB_OK) && !defined(UARTC_OK)
  HardwareSerial  Serial1( UARTB );
  HardwareSerial  Serial2( UARTB == 1 ? 2 : 1 );

  // UARTC assigned to 'Serial2'
  #elif   defined(UARTC_OK) && !defined(UARTB_OK)
  HardwareSerial  Serial1( UARTC == 1 ? 2 : 1 );
  HardwareSerial  Serial2( UARTC );

  // UARTB assigned to 'Serial1', UARTC to 'Serial2'
  #elif   defined(UARTB_OK) &&  defined(UARTC_OK)
  HardwareSerial  Serial1( UARTB );
  HardwareSerial  Serial2( UARTC );
  #endif

#endif  //endif, 'no globals'

Here is the new 'HardwareSerial::begin()':

// Default pin assignments, UARTS 1 and 2
void HardwareSerial::default_uart_pins ( int8_t& rxPin, int8_t& txPin )
{
    if(_uart_nr == 1) {
        rxPin = 9;
        txPin = 10;
    }
    if(_uart_nr == 2) {
        rxPin = 16;
        txPin = 17;
    }
} //default_uart_pins

void HardwareSerial::begin(unsigned long baud, uint32_t config, int8_t rxPin, int8_t txPin, bool invert)
{
    if(0 > _uart_nr || _uart_nr > 2) {
        log_e("Serial number is invalid, please use 0, 1 or 2");
        return;
    }
    if(_uart) {
        end();
    }

    // If no pins assignments for UART, use defaults
    if (rxPin < 0 && txPin < 0) {

        // UART0 is always defined
        if(_uart_nr == 0 ) {
           rxPin = RX;
           txPin = TX;
        }

        // No additional serial ports
        #if  !defined(UARTB_OK) && !defined(UARTC_OK)
        default_uart_pins( rxPin, txPin );
        #endif

        // (1) additional serial port, UARTB
        #if   defined(UARTB_OK) && !defined(UARTC_OK)
        if(_uart_nr == UARTB) {
            rxPin = RXB;
            txPin = TXB;
        } else {
           default_uart_pins( rxPin, txPin );
        }
        #endif

        // (1) additional serial port, UARTC
        #if   defined(UARTC_OK) && !defined(UARTB_OK)
        if(_uart_nr == UARTC) {
            rxPin = RXC;
            txPin = TXC;
        } else {
           default_uart_pins( rxPin, txPin );
        }
        #endif

        // (2) additional serial ports defined
        #if   defined(UARTB_OK) &&  defined(UARTC_OK)
        if(_uart_nr == UARTB) {
            rxPin = RXB;
            txPin = TXB;
        }
        if(_uart_nr == UARTC) {
            rxPin = RXC;
            txPin = TXC;
        }
        #endif
    } //end-if, pins check

    _uart = uartBegin(_uart_nr, baud, config, rxPin, txPin, 256, invert);
}

Let me know if this is satisfactory, and I'll generate a pull request.

ladyada commented 6 years ago

I'm ok with anything @me-no-dev will accept :)

me-no-dev commented 6 years ago

This is a lot of code given that uart numbers do not matter ;) you can start Serial2 on pins 1 and 3 and go just fine. What we can do is a simple ifdef to check if pins_arduino has redefined the default pins and change them accordingly. This way we do not need to bother changing the other boards either.

pins_arduino.h

#define TX1 17
#define RX1 16

#define TX2 2
#define RX2 4

HardwareSerial.cpp

#include "pins_arduino.h"
#include "HardwareSerial.h"

#ifndef RX1
#define RX1 9
#endif

#ifndef TX1
#define TX1 10
#endif

#ifndef RX2
#define RX2 16
#endif

#ifndef TX2
#define TX2 17
#endif

#if !defined(NO_GLOBAL_INSTANCES) && !defined(NO_GLOBAL_SERIAL)
HardwareSerial Serial(0);
HardwareSerial Serial1(1);
HardwareSerial Serial2(2);
#endif

HardwareSerial::HardwareSerial(int uart_nr) : _uart_nr(uart_nr), _uart(NULL) {}

void HardwareSerial::begin(unsigned long baud, uint32_t config, int8_t rxPin, int8_t txPin, bool invert)
{
    if(0 > _uart_nr || _uart_nr > 2) {
        log_e("Serial number is invalid, please use 0, 1 or 2");
        return;
    }
    if(_uart) {
        end();
    }
    if(_uart_nr == 0 && rxPin < 0 && txPin < 0) {
        rxPin = 3;
        txPin = 1;
    }
    if(_uart_nr == 1 && rxPin < 0 && txPin < 0) {
        rxPin = RX1;
        txPin = TX1;
    }
    if(_uart_nr == 2 && rxPin < 0 && txPin < 0) {
        rxPin = RX2;
        txPin = TX2;
    }
    _uart = uartBegin(_uart_nr, baud, config, rxPin, txPin, 256, invert);
}

stickbreaker commented 6 years ago

@me-no-dev What about multiple Serial.begin() statements that only change baud?

Serial.begin(9600,15,16);
//
Serial.begin(115200);
// Which pins are now Tx and Rx?  16 and 15 or 1 and 3?

Chuck.

me-no-dev commented 6 years ago

@stickbreaker that is a different thing. we are talking about the default pins currently 😄

me-no-dev commented 6 years ago

@mrwgx3 are you OK with what I propose above? @ladyada any comments from your side?

mrwgx3 commented 6 years ago

@mrwgx3 are you OK with what I propose above? @ladyada any comments from your side?

This is a lot of code given that uart numbers do not matter ;)
 you can start Serial2 on pins 1 and 3 and go just fine.

@me-no-dev, I disagree with the above statement, both from a hardware and design philosophy perspective. I'll elaborate more when I post the response I'm currently finishing up.

To insure we start on the same page, I'm referencing the [ESP32 Technical Reference Manual, Version 3.4, obtained via a download link on the Adafruit website][esp32_tekref]. If your reference is newer, please review Section 4 on the 'IO_MUX and GPIO Matrix' and verify that it is current.

[esp32_tekref]: https://www.espressif.com/sites/default/files/documentation/esp32_technical_reference_manual_en.pdf

ladyada commented 6 years ago

i'd just like it to be as compatible as possible with ever other Arduino compatible: Serial is the default hardware UART for debugging/uploading code Serial1 is the first non-debug-output UART device and the default pins shouldn't be defined in a sketch, they should be defined in the variant. thankfully ESP8266 settled on default pins for I2C, because for a while everyone was trying to submit PR's to me with Wire.begin(4,5) which would of course break every other build :)

mrwgx3 commented 6 years ago

Some background before jumping into the heart of things...

[hws_class_dir]: https://github.com/espressif/arduino-esp32/tree/master/cores/esp32

[hws_cons]: https://github.com/espressif/arduino-esp32/blob/master/cores/esp32/HardwareSerial.cpp#L14

[hws_begin_decl]: https://github.com/espressif/arduino-esp32/blob/master/cores/esp32/HardwareSerial.h#L40

[hws_begin_body]: https://github.com/espressif/arduino-esp32/blob/master/cores/esp32/HardwareSerial.cpp#L16#L38

[var_files_dir]: https://github.com/espressif/arduino-esp32/tree/master/variants

[uart_begin_body]: https://github.com/espressif/arduino-esp32/blob/12ca9e8b521127c5455b8e45b463371cc0cc6d3d/cores/esp32/esp32-hal-uart.c#L160#L219

Debug Philosophy

First off, I would like to say that I appreciate the quality work done by the ESP8266 and ESP32 Arduino communities. This is the main reason I write bug-reports, and occasionally even try to fix them. Being keenly aware that even trival fixes can entail considerable code retesting, I try to approach bug repair with these (2) maxims foremost in mind:

All code presented is likely there for a good reason. Alway try to understand the 'design intent' behind a piece of code before changing it.
All changes will likely have 'un-intended consequences', hence make them carefully and lightly.

'Design Intent' on Original 'HardwareSerial'

Before discussing the proposed changes to the ['HardwareSerial' class][hws_class_dir], let's explores the 'design intent' behind its default initialization:

The minimal [constructor][hws_cons] accepts and saves a physical uart number, '_uart_nr' for later use.
[HardwareSerial.begin()][hws_begin_body] completes the serial-port initialization; its [default call][hws_begin_decl] only requires a baudrate.
The default RX and TX pin arguments are 'undefined', which results in uart specific pin assignments for RX and TX:
```
UART0   TX =  1   RX =  3
UART1   TX = 10   RX =  9
UART2   TX = 17   RX = 16
```
Stopping here, one could reasonably conclude that these are the UART layout requirements for Arduino/ESP32 boards. Reviewing 'Chapter 4: IO_MUX and GPIO Matrix' of the ESP32 technical reference, however, shows the hardware interface to be much more mutable.

With minor restrictions, the input/ouput of any peripheral can be connected to any GPIO pad (pin) by appropriate setup of both the GPIO matrix and IO_MUX. Some high speed digital functions (Ethernet, SDIO, SPI, JTAG, UART) can connect to the IO_MUX directly, bypassing the GPIO matrix for better high-frequency performance, but with limited GPIO pad choices.

Studying the Peripherial Signal List (Section 4.9, Table 16) and the IO_Mux Pad List (Section 4.10, Table 16) together reveals the high-speed GPIO pad list for physical UARTS 0 through 2 are identical the rxPin/txPin defaults used in 'HardwareSerial.begin()':

U0TXD => GPIO_1    U0RXD => GPIO_3
U1TXD => GPIO_10   U1RXD => GPIO_9
U2TXD => GPIO_17   U2RXD => GPIO_16

After the pin defaults are set, calling ['uartBegin()'][uart_begin_body] initializes the UART port. Only the slower GPIO matrix seems selected for usage, and not the expected high-frequency digital mode. Perhaps the designers of the 'HardwareSerial' class deemed the ESP32 hardware interface too mutable for the Arduino environment, and:

Restricted UART speeds to only those allowed by the GPIO matrix, but
Kept the high-speed digital mode RX/TX pin assignments so advanced users could use their own "follow-on" code to access that mode.

Seemed like a reasonable 'intent' to me, with the depreciation or removal of the RX/TX 'variants' files constants simply a forgotten task. @stickbreaker, however, considered this a 'real' bug that needs addressing.

Given that all other interface pins do have definitions in the variant files, I would agree that RX and TX should have definitions too, if nothing else but for self-consistency. Being defined, RX and TX should connect to something, and it's a 'bug' if they do not.

mrwgx3 commented 6 years ago

Notes and Observations

A. Related issues and documents to review:

https://github.com/espressif/arduino-esp32/issues/1577
https://github.com/adafruit/Adafruit_GPS/issues/74
[ESP32 Technical Reference Manual, Version 3.4 (click to download PDF)][esp32_tekref] "See Chapter 4"

[esp32_tekref]: https://www.espressif.com/sites/default/files/documentation/esp32_technical_reference_manual_en.pdf

[m5stack]: https://github.com/espressif/arduino-esp32/blob/master/variants/m5stack_core_esp32/pins_arduino.h#L17#L18

[feather_esp32]: https://github.com/espressif/arduino-esp32/blob/master/variants/feather_esp32/pins_arduino.h#L17#L18

B. Notes on the 'pins_arduino.h' variant files

Statically defined constant variables are the preferred method of declaring constants. Unfortunately, one cannot do conditional compilations with them, as with '#define' constants.

Some IDE's (Borland C++ Builder, for example) can show static constant values during 'mouse-over' debug; are there similar IDE's that do the same for the Arduino ESP8266 / ESP32?. Is this the reason for the preference?

The defacto pin connections for UART0 (debug Serial) are:

static const uint8_t TX = 1;  // How did the board designers know this?
static const uint8_t RX = 3;  // Where is this originally documented?

Aside from the ['feather_esp32'][feather_esp32] board, I've only found [(1) other board][m5stack] which attempts to define an auxilary serial port:

//                               // 'feather_esp32' board
static const uint8_t TX = 17;    // Pin defn's are incorrect; board functions
static const uint8_t RX = 16;    // because of RX/TX disconnect & defaults used
...
//                               // 'm5stack_core_esp32' board
static const uint8_t TXD2 = 17;  // These unique definitions would not work
static const uint8_t RXD2 = 16;  // until renamed 'TX2' and 'RX2' with new code

Besides unique definitions, I've also seen additions, deletions, and renaming of constants; some standardization of the 'pins_arduino.h' files is needed.

I've noted the influence of Arduino/SAMD implimentation on the ESP8266 / ESP32 variant files in 'pins_arduino.h'

The one thing liked the SAMD implimentation was that it placed all hardware relevant stuff, including global instantiations and their external declarations, in a folder unique to the hardware.
IMHO, just doing this would help alot in clarifying the code. But as I stated earlier, "Implementing and testing it for the ESP32, however, may take considerable effort not lightly expended."

mrwgx3 commented 6 years ago

[1st_hws_sol]: https://github.com/espressif/arduino-esp32/issues/1607#issuecomment-403287300

[1st_hws_prop]: https://github.com/adafruit/Adafruit_GPS/issues/74#issuecomment-403257142

[2nd_hws_sol]: https://github.com/espressif/arduino-esp32/issues/1607#issuecomment-404716389

[hws_begin_body]: https://github.com/espressif/arduino-esp32/blob/master/cores/esp32/HardwareSerial.cpp#L16#L38

[uart_begin_body]: https://github.com/espressif/arduino-esp32/blob/12ca9e8b521127c5455b8e45b463371cc0cc6d3d/cores/esp32/esp32-hal-uart.c#L160#L219

[uart_undef]: https://github.com/espressif/arduino-esp32/issues/1604#issuecomment-403100956

Now we touch upon some design philosophy...

As seen from our earlier exploration of [HardwareSerial.begin()][hws_begin_body], the default RX and TX pin assignments are UART specific:

UART0   TX =  1   RX =  3    These pin assignments are required when
UART1   TX = 10   RX =  9    UARTS 0 - 2 are operated in high-frequency
UART2   TX = 17   RX = 16    digital mode

@me-no-dev commented:

This is a lot of code given that uart numbers do not matter ;) you can start Serial2 on pins 1 and 3 and go just fine.

As coded, ['uartBegin()'][uart_begin_body] does not enable the high-frequency (speed) digital mode; hence the statement about successfully starting Serial2 (UART2) using UART0's designated high-speed RX/TX pin assignments is correct. One could not, however, enter high-speed mode 'after the fact'.

Ideally, one should code a given hardware interface to its maximum versatility. As 'uartBegin()' is supplied with enough information to setup high-speed digital mode, it should be coded to do so if:

There are not mitigating circumstances (such as EMI issues) which preclude doing so.
There are sufficient resources available to test the code (I lack the hardware resourses).

Automatic entry into high-speed digital mode should be done by 'uartBegin()' when:

The appropriate UART/pin-combo is detected, and
Permission to do so was granted prior to or at calling 'uartBegin()'

Manual enable/disable of high-speed digital mode should also be allowed after calling 'uartBegin()'.

'uartBegin()' also currently supports 'half' UARTs, which either transmit or receive, but not both. If this feature can support high-speed digital mode, it should do so as well.

IMHO, changing 'HardwareSerial' to support high-speed digital mode would augment its current 'design intent'.

mrwgx3 commented 6 years ago

[1st_hws_sol]: https://github.com/espressif/arduino-esp32/issues/1607#issuecomment-403287300

[1st_hws_prop]: https://github.com/adafruit/Adafruit_GPS/issues/74#issuecomment-403257142

[2nd_hws_sol]: https://github.com/espressif/arduino-esp32/issues/1607#issuecomment-404716389

[hws_begin_body]: https://github.com/espressif/arduino-esp32/blob/master/cores/esp32/HardwareSerial.cpp#L16#L38

[uart_begin_body]: https://github.com/espressif/arduino-esp32/blob/12ca9e8b521127c5455b8e45b463371cc0cc6d3d/cores/esp32/esp32-hal-uart.c#L160#L219

Now we address @ladyada's concerns...

@ladyada commented:

i'd just like it to be as compatible as possible with ever other Arduino compatible: Serial is the default hardware UART for debugging/uploading code Serial1 is the first non-debug-output UART device and the default pins shouldn't be defined in a sketch, they should be defined in the variant.

Allowing the pairing a given uart number with a specific 'Serial' instance addresses ladyada's concerns. The [first proposed solution][1st_hws_sol] was designed to do this while also reconecting RX/TX pairs, as [outlined here][1st_hws_prop].

Note that the first solution was coded specifically to:

Use only 'statically declared' constants, as not to change the variant file format.
Require that all global 'Serial' instances (Serial, Serial1, & Serial2) exist.

@me-no-dev commented:

I do not want to put 9 same extra lines in all pins_arduino.h files. Let's find a "better" solution on how to do this. And TX and RX should stay (maybe as defines to RXA/TXA or something)

This comment prompted the creation of a [second solution][2nd_hws_sol], one which allowed uart definitions to be added conditionally. This was done by converting the physical uart numbers to '#define' type constants, which bends the implicit rule of not changing the variant file format (but not to greatly).

@me-no-dev commented:

This is a lot of code given that uart numbers do not matter ;)

The amount of extra code is due to the additional constraint of adding UART/PIN definition to the variant file as needed, rather than defining them all at once. Not knowing beforehand on what definitions will be present, one has to test the 'UARTx conditional define' combinations exhaustively, both when:

Defining the global 'Serial' instances; remember all MUST exist and have proper UART numbers.
Assigning 'Rx/Tx' pin defaults, in order to know what RX/TX pairs exist.

By noting that same RX/TX pin defaults are paired with UART0/'Serial' across all variant files, one can reduce the combinations checked to a managable (but still messy) level. If (4+) UARTS were available, the second solution would be untenable.

My preference is NOT to use conditional includes unless absolutely necessary.

@me-no-dev commented:

This is a lot of code given that uart numbers do not matter ;) ...What we can do is a simple ifdef to check if pins_arduino has redefined the default pins and change them accordingly. This way we do not need to bother changing the other boards either.

Given this code snippet, you'll still have the RX/TX disconnect, which is considered a 'bug'.

if(_uart_nr == 0 && rxPin < 0 && txPin < 0) {
    rxPin = 3;   // Should equal RX
    txPin = 1;   // Should equal TX

If you do define pins for other UARTS, you've mixed your constant types.


static const uint8_t TX = 1;    // These should be changed to '#define' constants 
static const uint8_t RX = 3;    // in order to be consistent with new additions

define TX1 17

define RX1 16

define TX2 2

define RX2 4


@me-no-dev commented:
> ...```This way we do not need to bother changing the other boards either```.

To preserve consistency across all variant files, you'd then need to change all static definitions for RX and TX to '#define' type constants. Having arrived at this point, you should just go with the preferred statically defined constants, *and update all the variant files* (this is where the information belongs).

mrwgx3 commented 6 years ago

[uart_undef]: https://github.com/espressif/arduino-esp32/issues/1604#issuecomment-403100956

[gps_echo]: https://github.com/adafruit/Adafruit_GPS/blob/master/examples/GPS_HardwareSerial_EchoTest/GPS_HardwareSerial_EchoTest.ino

Here is the proposed '3rd' solution. It's pretty much like the 1st one, but with a couple of minor additions:

As before, any physical UART number can be assigned to any 'Serialx' instance.
Undefined UARTs are now allowed in order to [address pin conflicts with UART defaults][uart_undef], see issue #1604.
Un-used UARTs should be deemed 'un-defined' by default.
'Redefining' UART numbers should be allowed to accomodate customized hardware.
'Half' UARTs can now be defined in a variant file, for consistency with existing code.

pins_arduino.h

// The required (9) constants are now defined on (3) lines
// Default UART/RX/TX constants for most variants are:

static const uint8_t  UARTB =  0,  RX  =  3,  TX  =  1;
static const uint8_t  UARTB = -1,  RXB =  9,  TXB = 10; 
static const uint8_t  UARTC = -1,  RXC = 16,  TXC = 17;

// UART/RX/TX constants for the (2) variants currently 
// defining an auxilary serial port:
//
//   a) 'feather_esp32' 
//   b) 'm5stack_core_esp32'

static const uint8_t  UARTA =  0,  RX  =  3,  TX  =  1;  
static const uint8_t  UARTB =  2,  RXB = 16,  TXB = 17;
static const uint8_t  UARTC = -1,  RXC =  9,  TXC = 10;

// 'Half' UARTs can now be defined in a variant file:
...
static const uint8_t  UARTB = 2,  RXB = -9,  TXB =  10; // Xmit
static const uint8_t  UARTC = 1,  RXC = 16,  TXC = -17; // Rcve

// (2) 'undefined' pins generates an error message:
...
static const uint8_t  UARTC = 1,  RXC = -16,  TXC = -17; // Error

@ladyada and @reaper7, please review and verify this is what you want.

HardwareSerial.h

A routine was added to change the physical UART number of a 'Serialx' instance.

class HardwareSerial: public Stream
{
public:
    HardwareSerial(int uart_nr);

    void changeUartNumber( int uart_nr );   // Add here
    void begin(unsigned long baud, uint32_t config=SERIAL_8N1, int8_t rxPin=-1, int8_t txPin=-1, bool invert=false);
...

HardwareSerial.cpp

Here is the new 'HardwareSerial' code.

#include "pins_arduino.h"
#include "HardwareSerial.h"

#if !defined(NO_GLOBAL_INSTANCES) && !defined(NO_GLOBAL_SERIAL)

  HardwareSerial  Serial ( UARTA );   // UARTx = 0, 1, 2, or undefined (-1). Values
  HardwareSerial  Serial1( UARTB );   // set in variant 'pins_arduino.h' file
  HardwareSerial  Serial2( UARTC );

#endif  // 'no globals'

HardwareSerial::HardwareSerial(int uart_nr) : _uart_nr(uart_nr), _uart(NULL) {}

void HardwareSerial::changeUartNumber( int uart_nr )
{
    if(_uart) {
        end();
    }
    _uart_nr = uart_nr;
}

void HardwareSerial::begin(unsigned long baud, uint32_t config, int8_t rxPin, int8_t txPin, bool invert)
{
    if ( (-1 > _uart_nr) || (_uart_nr > 2) ) {
        log_e("Serial number is invalid, please use 0, 1, 2, or undefined (-1)");
        return;
    }
    if(_uart) {
        end();
    }

    // Use 'variant' file RX/TX pin values if:
    //   a) The physical UART number is defined (0,1, or 2), and
    //   b) Both the given RX/TX pins are undefined (< 0)

    if ( (_uart_nr >= 0) && (rxPin < 0) && (txPin < 0) )
    {
       if(_uart_nr == UARTA ) {
           rxPin = RX;
           txPin = TX;
       }
       if(_uart_nr == UARTB ) {
           rxPin = RXB;
           txPin = TXB;
       }
       if(_uart_nr == UARTC ) {
           rxPin = RXC;
           txPin = TXC;
       }

       if ( (rxPin < 0) && (txPin < 0) ) {
          log_e( "Variant file RX/TX cannot both be undefined (< 0)" );
       }
    } //end-if, set defaults

    // 'uartBegin()' handles an undefined:
    //   a) Physical UART number (returns NULL)
    //   b) RX/TX pin pair       (returns NULL)
    //   c) RX or TX pin         ('half'  UART config)

    _uart = uartBegin(_uart_nr, baud, config, rxPin, txPin, 256, invert);
}

Example: Changing Uart Number for 'Serial1' Instance

  // Change Uart Number from 'undefined' = -1 to '2'
  Serial1.changeUartNumber( 2 );

  // Setup UART parameters using full-form of 'begin()'
  Serial1.begin(9600, SERIAL_8N1, 16, 17, false );

This solution has been fully tested in my local repository, including:

[GPS_HardwareSerial_EchoTest.ino][gps_echo] using the 'feather_esp32' variant, and
'Half' uart configuration (GPIO pins 16 and 17 are jumpered)
```
static const uint8_t  UARTA =  0,  RX  =  3,  TX  =  1;
static const uint8_t  UARTB =  2,  RXB =-16,  TXB = 17;
static const uint8_t  UARTC =  1,  RXC = 16,  TXC =-17;
```
I've also made the required constants changes for all variant files (a trival effort) defined as of 06/05/2018. I can generate a pull-request with minimal effort.

In closing, @me-no-dev, I think I've covered all the bases that I can for the moment; let's put this issue to bed :)

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 5 years ago

This stale issue has been automatically closed. Thank you for your contributions.