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

fix: HW CDC write() #9401

Merged
merged 5 commits into from
Apr 8, 2024
Merged

fix: HW CDC write() #9401

merged 5 commits into from
Apr 8, 2024

Conversation

SuGlider
Copy link
Collaborator

@SuGlider SuGlider commented Mar 21, 2024

Description of Change

When writing a stream of data to the HW CDC of S3/C3/C6/H2, the SoC looses some bytes and don't send them all.

This fix makes it work fine.

Tests scenarios

Using an Arduino Sketch and a Python script to test the data flow.
Run the sketch in the S3/C3/C6/H2 with HWCDC on Boot enabled.
Run the script in the USB Host computer. It will open the CDC port and run the test.

Arduino side:

// ESP32-C3 using HW Serial CDC on Boot enabled

bool testCase1(const String &data) {
  size_t receivedLen = data.length();
  size_t sendLen = 0;

  while (sendLen < receivedLen) {
    if (!Serial.write(data[sendLen++])) break;   // <<< if it fails sending a single byte, it will fail the whole test case
  }
  Serial.println("===");
  if (sendLen != receivedLen) {
    Serial0.printf("\r\n ===> T1::Error -- didn't send all data [%d of %d]\r\n", sendLen, receivedLen);
    return false;
  } else {
    return true;
  }
}

bool testCase2(const String &data) {
  size_t receivedLen = data.length();

  size_t sent = Serial.write(data.c_str(), receivedLen);
  Serial.println("===");
  if (sent != receivedLen) {
    Serial0.printf("\r\n ===> T2::Error -- didn't send all data [%d of %d]\r\n\r\n", sent, receivedLen);
    return false;
  }  else {
    return true;
  }
}

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

  Serial.begin();
  Serial0.println("\r\nStarting... open Serial Monitor with CDC port.");
  while(!Serial) delay(100);
  Serial.println("\r\nCDC Started.");

  bool result = true;
  String testString = "0";
  for (int x = 0; x < 100; x++) {  // runs the test cases 100 times
    String testString = "0";
    for (int i = 0; i < 302; i++) {  // sends from 1 to 301 bytes per test
      result &= testCase1(testString);
      result &= testCase2(testString);
      if (!result) {
        Serial0.printf("Failed with %d bytes\r\n", i);
        break;
      }
      char c = testString.charAt(testString.length() - 1);
      c = c == '9' ? '0' : c + 1;
//      if(i > 22) c = '7'; inserts an error to test the python script
      testString += c;
    }
    if (!result) break;
    Serial0.print(x+1);
    Serial0.print(" ");
    if (((x+1) % 10) == 0) Serial0.println();
  }
  if (result) {
    Serial.println("OK");
    Serial0.println("\r\nSetup is done : SUCCESS!\r\n");
  }  else {
    Serial.println("ERROR!!!");
    Serial0.println("\r\n\t\t====> Setup is done : FAILED!\r\n");
  }
}

void loop() {}

Python side:

#! /c/Users/rocor/AppData/Local/Microsoft/windowsApps/python.exe


"""
	Python Script for testing ESP32 S3 + C3 Arduino issue #9378 
	ISSUE - HW CDC Write() with fast stream of data
"""

import serial
import re
import sys
import random
import time

print ("CDC App test for issue 9378")

def is_valid_sequence(s):
    # Initialize variables
    prev_digit = None
    valid = True

    # Iterate through each character in the string
    for char in s:
        if char.isdigit():
            digit = int(char)
            if prev_digit is None:
                # First digit
                if digit != 0:
                    valid = False
                    break
            else:
                # Check if the next digit is valid
                if digit != (prev_digit + 1) % 10:
                    valid = False
                    break
            prev_digit = digit
        elif char == "=":
            # Check if the sequence ends with "==="
            if s.endswith("==="):
                break
            else:
                valid = False
                break
        else:
            # Invalid character
            valid = False
            break

    return valid


try:
	# CDC same as SERIAL_8N1 - Arduino equivalent
	# Change 'com1' to what ever is your USB CDC serial device name (win/linux)
	# timeout=None means that it will work as a blocking read()
	# write() is blocking by default

	CDC = serial.Serial(port='com22', baudrate=115200)   # timeout=0.01 means non blocking! 
#	CDC.set_buffer_size(rx_size = 128, tx_size = 128)  ## comment it out to speed up processing
except:
	print("COM22: port is busy or unavailable")
	exit()

print(f"Serial port com22 opened successfully.")
#read first 2 lines
line = CDC.readline().decode("utf-8").strip()
print(line)   ### for debugging
line = CDC.readline().decode("utf-8").strip()
print(line)  ### for debugging
pattern = re.compile(r"(\d+)===")

Error = False
counter = 0

while True:
	# Adds some random delay... for testing
#	randomTime_ms = random.randint(0, 5)   ## comment it out to speed up processing
#	time.sleep(randomTime_ms / 1000)
	
	# Read a line from the serial port
	line = CDC.readline().decode("utf-8").strip()
#	print(line)  ### for debugging

	# Check if the line ends with "==="
	if line.endswith("==="):
		# Extract the counter and sequence
		match = pattern.match(line)
		if match:
			sequence = match.groups()
			# Count the number of digits (0 to 9) in the sequence
			num_digits = len(sequence[0])
			if is_valid_sequence(sequence[0]):
				if num_digits >  301: ### Arduino sketch sends up to 302 digits
					if counter % 2 == 1: ### Arduino sketch send the line twice (TestCase 1 and 2)
						### CDC output will match ESP32 UART0 output for user control
						### Python will process the OS buffer and stay behind the ESP32 counting
						print(f"{int(counter / 2) + 1} ", end='',sep='',flush=True)				
					counter += 1
					if counter % 20 == 0:
						print("")
#				print(f"Sequence: '{sequence[0]}', Number of Digits: {num_digits}")  ### for debugging
			else:
				print(f"==> BAD sequence '{sequence[0]}', length: {num_digits}")
				Error = True
				break
		else:
			print(f"Invalid format: '{line}'")
			Error = True
			break
	else:
		print(line)  ### for debugging
		if line != "OK":
			print(f"Incomplete line: '{line}'")
			Error = True
		break

if Error:
	print("    ERROR ....")
else:
	print("\n All Fine!")
sys.exit()  ## this will close the CDC connection and reset the ESP32-xx

Related links

Fix #9378

When writing a stream of data to the HW CDC of S3/C3/C6/H2, the SoC looses some bytes and don't send them all.

This fix makes it work fine.
@SuGlider SuGlider added the Status: In Progress Issue is in progress label Mar 21, 2024
@SuGlider SuGlider self-assigned this Mar 21, 2024
Copy link
Contributor

github-actions bot commented Mar 21, 2024

Warnings
⚠️

Some issues found for the commit messages in this PR:

  • the commit message "fix: HW CDC write()":
    • body's lines must not be longer than 100 characters
    • summary looks too short

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

⚠️

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

  • contains uppercase letters. This can cause troubles on case-insensitive file systems (macOS).
    Please rename your branch.
Messages
📖 You might consider squashing your 5 commits (simplifying branch history).

👋 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.
- Addressing info messages (📖) is strongly recommended; they're less critical but valuable.
- 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 c5749cb

@SuGlider SuGlider added this to the 3.0.0-RC1 milestone Mar 21, 2024
@SuGlider
Copy link
Collaborator Author

Let's wait for @TD-er to confirm that this PR fixes the issue.

@P-R-O-C-H-Y
Copy link
Member

@SuGlider Is this ready to be reviewed? :)

@TD-er
Copy link
Contributor

TD-er commented Apr 8, 2024

I have been running this code for a while now and it does seem to work fine.
Only issue might be when you try to set the TX or RX buffer twice while HWCDC is already active.
One shouldn't do this, but it may be hard to diagnose when the device is crashing immediately.
Maybe just documenting it is enough here?

@me-no-dev me-no-dev merged commit 34f5456 into master Apr 8, 2024
39 checks passed
@me-no-dev me-no-dev deleted the SuGlider-patch-1 branch April 8, 2024 12:17
P-R-O-C-H-Y pushed a commit to P-R-O-C-H-Y/arduino-esp32 that referenced this pull request Apr 16, 2024
When writing a stream of data to the HW CDC of S3/C3/C6/H2, the SoC looses some bytes and don't send them all.

This fix makes it work fine.
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.

[HWCDC] Missing data via USB Serial on C3/C6/S3
5 participants