Skip to content
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

Serial as a #define #8798

Merged
merged 9 commits into from
Oct 25, 2023
Merged

Serial as a #define #8798

merged 9 commits into from
Oct 25, 2023

Conversation

SuGlider
Copy link
Collaborator

@SuGlider SuGlider commented Oct 23, 2023

Description of Change

The PR changes Serial to be a pre-processor symbol instead of a HardwareSerial/HWCDC/USBCDC object.
Therefore, UART0 is always the Serial0 object.

If the sketch doesn't use CDC on Boot, Serial will defined to be Serial0 by default.
Whenever the sketch is set to use CDC on Boot, Serial will defined to be USBSerial object, which in turn will point to a HWCDC or USBCDC object depending on how it is configured / which SoC is used.

The PR will make Serial0, Serial1 and Serial2 (if it exists) always available as UART0/1/2.
USBSerial will also always be available whenever the SoC has a CDC interface.

If necessary the sketch can change/force the redefinition of Serial symbol to whatever necessary.

The PR also adds a HWCDCSerialEvent() callback that is executed after loop() whenever there are bytes available in the HW Serial Interface.
The same for USBSerialEvent() in Native USB CDC. Those two functions can be defined in the sketch by the user and work in the same way as SerialEvent() works.

Tests scenarios

Used "OnReceive_Demo.ino" to test it using ESP32/ESP32S2/ESP32C3/ESP32S3 with different configurations for USB, whenever possible.

Related links

None

@SuGlider SuGlider added this to the 3.0.0 milestone Oct 23, 2023
@SuGlider SuGlider self-assigned this Oct 23, 2023
#if ARDUINO_USB_CDC_ON_BOOT //Serial used for USB CDC
if(Serial0.available()) serialEvent();
#else
// Serial is always defined as something (UART0 or USBSerial)
if(Serial.available()) serialEvent();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be Serial0.available()

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about it yesterday. If I use Serial.available(), it works with the CDC as well...
Serial0 will limit it to UART only.
Any opinion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be left as Serial0 and then have other lines that handle USB. In case of CDC, Serial0 still should be handled here :0

// When using CDC on Boot, Arduino Serial is the USB device
#define Serial USBSerial
#endif
// USBSerial is always available to be used
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the whole #if ARDUINO_USB_CDC_ON_BOOT block should be in HardwareSerial.h. Same goes for HWCDC.h.

#if !ARDUINO_USB_MODE        // Native USB CDC selected
extern USBCDC USBSerial;
#endif

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

@@ -9,13 +9,7 @@ void loop(){}
#if !ARDUINO_USB_MSC_ON_BOOT
FirmwareMSC MSC_Update;
#endif
#if ARDUINO_USB_CDC_ON_BOOT
#define HWSerial Serial0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you no longer need this define. Change the rest of the example code to call Serial0 for UART and USBSerial for CDC. Same for the other applicable examples.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's something I was thinking about yesterday as well...
I left it there because the user could change it to any other Print communication channel...
What do you say? We can change it to "PrintObj" instead of "HWSerial"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code, I think that with those changes, it needs to just call Serial and not even check this define. If it's set to be UART, it will go there, else whatever CDC it's set to.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace all HWSerial with Serial and delete references to USBSerial

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

#else
extern HWCDC USBSerial;
#if ARDUINO_USB_MODE // Hardware JTAG CDC selected
#if ARDUINO_USB_CDC_ON_BOOT //Serial used for USB CDC
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment about USBCDC.h

#if !ARDUINO_USB_MODE
#include "USB.h"
#include "USBCDC.h"
#if !ARDUINO_USB_CDC_ON_BOOT //Serial used for USB CDC
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here you should have the checks from both CDCs as well. One place for all :)

@SuGlider SuGlider requested a review from me-no-dev October 24, 2023 19:36
@SuGlider
Copy link
Collaborator Author

@me-no-dev - the changes shall be addressed now. I also added a way to run SerialEvent() like callbacks for HW and USB CDC.

@me-no-dev me-no-dev merged commit 9dd9bde into espressif:master Oct 25, 2023
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

2 participants