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

Fixes the hardware cdc jtag plugged/unplugged status and related timeout/delay #9275

Merged
merged 30 commits into from
Feb 28, 2024

Conversation

SuGlider
Copy link
Collaborator

@SuGlider SuGlider commented Feb 22, 2024

Description of Change

This will use a new IDF 5.1 feature to detect if the USB calbe is plugged or not.
Testing HWCDCSerial (as bool operator) will return true only when CDC is connected (to a terminal application).
It also fixes issues related to timeout, hanging or delays while writing to the HW Serial when USB is unplugged.

Main changes:

  • checking if(HWCDCSerial) returns true only when CDC is connected
  • plugging/unplugging the USB cable will reflect in checking if CDC is connected
  • if USB cable is not plugged or CDC is not connected, flush and write operation will not block
  • unplugging the USB cable while the sketch is running (writing to CDC) won't affect its execution and it can be checked at any time testing HWCDCSerial == true|false

Tests scenarios

Tested with ESP32-C3, ESP32-S3, ESP32-C6 and ESP32-H2 using the new example from this same PR.
It was also tested using this example:

#include "driver/usb_serial_jtag.h"

bool isPlugged() {
  return usb_serial_jtag_is_connected();
}

const char* _hwcdc_status[] = {
  " USB Plugged but CDC is not connected\r\n",
  " USB Plugged and CDC is connected\r\n",
  " USB Unplugged and CDC not connected\r\n",
  " USB Unplugged BUT CDC is connected :: PROBLEM\r\n",
};

const char* HWCDC_Status() {
  int i = isPlugged() ? 0 : 2;
  if(HWCDCSerial) i += 1;
  return _hwcdc_status[i];
}

void setup() {
  HWCDCSerial.begin();
  Serial0.begin(115200);

#if 1  // undefine this part to test the hanging issue with flush and/or write when USB is unplugged
  while (!HWCDCSerial) {  // only returns true when USB is plugged and CDC is connected to a serial monitor application
    Serial0.print(millis());
    Serial0.print(HWCDC_Status());
    delay(500);
  }
#endif

  Serial0.print("\n=================\nPrinting on UART\n=================\n");
  Serial0.flush();

  HWCDCSerial.print("\n=================\nPrinting on USB CDC\n=================\n");
  HWCDCSerial.flush(); // <-- HERE PROGRAM STALLS IF USB IS UNPLUGGED - FIXED :: is doesn't block.


}

void loop() {
  static uint32_t now = millis();

  if (millis() > now + 1000) {
    Serial0.printf("[%ld] Loop running UART...\n", millis());
    Serial0.print(HWCDC_Status());
    Serial0.flush();

    // when unplugged or not connected, it will discart the TX buffer when it gets full. 
    // Only the lastest written messages will be kept in the TX buffer.
    HWCDCSerial.printf("[%ld] Loop running USB CDC...\r\n", millis()); // it doesn't block even when TX Ringbuffer is full
    HWCDCSerial.flush(); // it doesn't block anymore, even with USB unplugged or CDC not connected

    now += 1000;
  }
}

Related links

Fixes #9043
Fixes #9004
Fixes #8284
Fixes #7779
Fixes #7554
Fixes #6089

This will use a new IDF 5.1 feature to detect if the USB HW CDC is plugged or not. This can be checked testing HWCDCSerial.
It also fixes issues related to timeout or delays while writing to the HW Serial when USB is unplugged.
@SuGlider SuGlider added this to the 3.0.0-RC1 milestone Feb 22, 2024
@SuGlider SuGlider self-assigned this Feb 22, 2024
Copy link
Contributor

github-actions bot commented Feb 22, 2024

Warnings
⚠️

Some issues found for the commit messages in this PR:

  • the commit message "Apply suggestions from code review":
    • summary looks empty
    • type/action looks empty
  • the commit message "Update cores/esp32/HWCDC.cpp":
    • summary looks empty
    • type/action looks empty
  • the commit message "Update cores/esp32/HWCDC.cpp":
    • summary looks empty
    • type/action looks empty
  • the commit message "Update cores/esp32/HWCDC.cpp":
    • summary looks empty
    • type/action looks empty
  • the commit message "Update cores/esp32/HWCDC.cpp":
    • summary looks empty
    • type/action looks empty
  • the commit message "Update cores/esp32/HWCDC.cpp":
    • summary looks empty
    • type/action looks empty
  • the commit message "Update cores/esp32/HWCDC.cpp":
    • summary looks empty
    • type/action looks empty
  • the commit message "Update cores/esp32/HWCDC.cpp":
    • summary looks empty
    • type/action looks empty
  • the commit message "Update cores/esp32/HWCDC.cpp":
    • summary looks empty
    • type/action looks empty
  • the commit message "Update cores/esp32/HWCDC.cpp":
    • summary looks empty
    • type/action looks empty
  • the commit message "Update cores/esp32/HWCDC.cpp":
    • summary looks empty
    • type/action looks empty
  • the commit message "Update cores/esp32/HWCDC.cpp":
    • summary looks empty
    • type/action looks empty
  • the commit message "Update cores/esp32/HWCDC.cpp":
    • summary looks empty
    • type/action looks empty
  • the commit message "Update cores/esp32/HWCDC.cpp":
    • summary looks empty
    • type/action looks empty
  • the commit message "Update cores/esp32/HWCDC.cpp":
    • summary looks empty
    • type/action looks empty
  • the commit message "Update cores/esp32/HWCDC.cpp":
    • summary looks empty
    • type/action looks empty
  • the commit message "Update cores/esp32/HWCDC.cpp":
    • summary looks empty
    • type/action looks empty
  • the commit message "feat(hw_cdc):fixes the hardware cdc jtag plugged/unplugged status":
    • body's lines must not be longer than 100 characters
    • summary looks empty
    • type/action looks empty
  • the commit message "feat(serialcdc): non block functions":
    • body's lines must not be longer than 100 characters
    • summary looks too short
  • the commit message "feat: adds .skip.esp32":
    • summary looks too short
  • the commit message "feat: adds .skip.esp32s2":
    • summary looks too short
  • the commit message "fix(HWCDC): changes made demands testing for CDC ON BOOT":
    • scope/component should be lowercase without whitespace, allowed special characters are _ / . , * - .

Please fix these commit messages - here are some basic tips:

  • follow Conventional Commits style
  • correct format of commit message should be: <type/action>(<scope/component>): <summary>, for example fix(esp32): Fixed startup timeout issue
  • allowed types are: change,ci,docs,feat,fix,refactor,remove,revert,test
  • sufficiently descriptive message summary should be between 20 to 72 characters and start with upper case letter
  • avoid Jira references in commit messages (unavailable/irrelevant for our customers)

TIP: Install pre-commit hooks and run this check when committing (uses the Conventional Precommit Linter).

⚠️ Please consider squashing your 30 commits (simplifying branch history).
⚠️

The source branch "SuGlider-patch-2" incorrect format:

  • contains uppercase letters. This can cause troubles on case-insensitive file systems (macOS).
    Please rename your branch.

👋 Hello SuGlider, we appreciate your contribution to this project!


Click to see more instructions ...


This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Resolve all warnings (⚠️ ) before requesting a review from human reviewers - they will appreciate it.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...


We do welcome contributions in the form of bug reports, feature requests and pull requests.

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
4. If the change is approved and passes the tests it is merged into the default branch.

Generated by 🚫 dangerJS against 7b42a8f

Skips the ESP32 SoC test given that it has no USB
Skips the ESP32S2 because it has no HW CDC JTAG interface
Only compiles the example in case it is using Hardware CD and JTAG mode.
@SuGlider SuGlider marked this pull request as draft February 22, 2024 03:51
@VojtechBartoska VojtechBartoska added the Status: In Progress Issue is in progress label Feb 22, 2024
lucasssvaz and others added 3 commits February 23, 2024 08:57
modifies write and flush to do not clock in case CDC host is not connected to the CDC client from the C3/S3/C6/H2
cores/esp32/HWCDC.cpp Outdated Show resolved Hide resolved
Improves the example by adding more information about USB being plugged and CDC being connected.
Detects correctly when CDC is or not connected. 
Deals with USB unplugged while the sketch is printing to CDD.
cores/esp32/HWCDC.cpp Outdated Show resolved Hide resolved
cores/esp32/HWCDC.cpp Outdated Show resolved Hide resolved
cores/esp32/HWCDC.cpp Outdated Show resolved Hide resolved
cores/esp32/HWCDC.cpp Outdated Show resolved Hide resolved
cores/esp32/HWCDC.cpp Outdated Show resolved Hide resolved
cores/esp32/HWCDC.cpp Outdated Show resolved Hide resolved
cores/esp32/HWCDC.cpp Outdated Show resolved Hide resolved
cores/esp32/HWCDC.cpp Outdated Show resolved Hide resolved
cores/esp32/HWCDC.cpp Outdated Show resolved Hide resolved
cores/esp32/HWCDC.cpp Outdated Show resolved Hide resolved
cores/esp32/HWCDC.cpp Outdated Show resolved Hide resolved
cores/esp32/HWCDC.cpp Outdated Show resolved Hide resolved
cores/esp32/HWCDC.cpp Outdated Show resolved Hide resolved
cores/esp32/HWCDC.cpp Outdated Show resolved Hide resolved
cores/esp32/HWCDC.cpp Outdated Show resolved Hide resolved
cores/esp32/HWCDC.cpp Outdated Show resolved Hide resolved
cores/esp32/HWCDC.cpp Outdated Show resolved Hide resolved
cores/esp32/HWCDC.cpp Outdated Show resolved Hide resolved
cores/esp32/HWCDC.cpp Outdated Show resolved Hide resolved
cores/esp32/HWCDC.cpp Outdated Show resolved Hide resolved
@SuGlider SuGlider marked this pull request as ready for review February 28, 2024 06:02
@SuGlider SuGlider removed the Status: In Progress Issue is in progress label Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment