CDC doesn't respect no flow control setting (DTR control needed to receive data)

[MrGreensWorkshop]

Operating System

Windows 10, Mac OS, I think in all OS es.

Board

Raspberry pi pico

Firmware

just simple app to test cdc behavior.

RasPiPicoSDK_cdc_test-main.zip

What happened ?

When handshake is set to none, to receive data we need to enable DTR. We shouldn't be force to enable DTR to receive data. (other flow control status shouldn't be changed in cdc side if it is the case.)

I believe this is related with #872 maybe with #746 too.

please watch the gif file.

How to reproduce ?

RasPiPicoSDK_cdc_test-main.zip

• Download
• Compile and run. (or just use binary named PicoTest.uf2)
• Connect to Raspberry pi pico to USB thats all. (No usb serial converters attached.)
• Use Terminal.exe or CoolTerm or any other terminal let you set DTR.
• Check DTR behavior.

Expected behavior

This the demonstration that 2 FDTI USB serial converter connected each other with 3 pins.

Rx <- Tx
Tx -> Rx
GND <-> GND

It shouldn't be forced to enable DTR to receive data either sides. Please watch the gif file.

Cause

This happens when raspberry pi usb cdc is used.

This is other issue talks about same thing.
#932

Please check this file.

https://github.com/hathach/tinyusb/blob/master/src/class/cdc/cdc_device.c

it is checking DTR if it's on or not regardless checking flow control.

bool tud_cdc_n_connected(uint8_t itf)
{
  // DTR (bit 0) active  is considered as connected
  return tud_ready() && tu_bit_test(_cdcd_itf[itf].line_state, 0);
}

[TravisRo]

This has been done intentionally. They use the DTR signal to detect when the PC/host has actually opened or closed the com port. I agree, it causes problems it some cases. Most of the arduino CDC implementations do this as well; it is fairly common.

I have a pull request submitted that adds a CH341 emulation class driver which would solve your problem.
https://github.com/hathach/tinyusb/pull/1507

You can get this from my fork here: https://github.com/TravisRo/tinyusb/tree/master

The api is nearly 100% compatible with the cdc driver except you have to change all of your "tud_cdc" prefixes to "tud_ch341"

[MrGreensWorkshop]

Thank you for responding my inquiry. Yes I checked the code. And Thank you for the PR #1507

When people use MCU which support USB, and if serial port is needed using CDC is common to use instead of using USB serial converters. When it happens, you expect using CDC to be the same as using a USB serial converter. Using DTR to detect connection shouldn't be implemented. We can't expect all applications to raise DTR when open the serial port. That's why I raised this issue.

[HiFiPhile]

@MrGreensWorkshop
CDC class has very limited function. I made this issue long time ago #400 and had a long discussion #401

[MrGreensWorkshop]

I saw in #482 too. its closed with another #506 patch, which is still using DTR control.

I think author of this repo only tested in VT100 terminals like PuTTY or Teraterm. PuTTY or Teraterm terminals are made for VT100 etc., not exactly plain text transmission. You can use if you want. But those are uses other pins too. DTR -> DSR and DSR <- DTR. But the main problem is not those terminals. Because we use them just for test.

The real problem is, asking host application for DSR on is reduce the compatibility with other apps if user is explicitly select no flow control.

We should came with better option.

[HiFiPhile]

I think you should ask pico SDK to remove tud_cdc_n_connected() test, just like #506 remove this test from examples.

[MrGreensWorkshop]

I know I know. I'm trying to make a point. I know the solution.
pico SDK -> They don't even understand the problem. Check this #906

[TravisRo]

Removing tud_cdc_n_connected() from your code is not quite enough. the cdc class also changes the tx fifo back and forth between overwriteable (not connected DTR=0) and not overwriteable (connected DTR=1)

[MrGreensWorkshop]

I see your point.

[hathach]

CDC should still send & receive data even if DTTR/RTS aren't set. Could you try the stock example from this repo examples/device/cdc_msc to see if it could echo back. cd to the example directory and run

make BOARD=raspberry_pi_pico

If yes, then the issue is pico-sdk making use of the connect() API which return the state of DTR. In that case you should ask rpi team to remove that check.

[MrGreensWorkshop]

@hathach
I see examples/device/cdc_msc is not using tud_cdc_connected instead tud_cdc_available was used.

src/class/cdc/cdc_device.h

...
static inline bool tud_cdc_connected (void)
{
  return tud_cdc_n_connected(0);
}
...
static inline uint32_t tud_cdc_available (void)
{
  return tud_cdc_n_available(0);
}
...

src/class/cdc/cdc_device.h

...
bool tud_cdc_n_connected(uint8_t itf)
{
  // DTR (bit 0) active  is considered as connected
  return tud_ready() && tu_bit_test(_cdcd_itf[itf].line_state, 0);
}
...
uint32_t tud_cdc_n_available(uint8_t itf)
{
  return tu_fifo_count(&_cdcd_itf[itf].rx_ff);
}
...

the example suggested examples/device/cdc_msc is waiting for chars if they are available, then it sends backs.

void cdc_task(void)
{
  // connected() check for DTR bit
  // Most but not all terminal client set this when making connection
  // if ( tud_cdc_connected() )
  {
    // connected and there are data available
    if ( tud_cdc_available() )
    {
      // read datas
      char buf[64];
      uint32_t count = tud_cdc_read(buf, sizeof(buf));
      (void) count;

      // Echo back
      // Note: Skip echo by commenting out write() and write_flush()
      // for throughput test e.g
      //    $ dd if=/dev/zero of=/dev/ttyACM0 count=10000
      tud_cdc_write(buf, count);
      tud_cdc_write_flush();
    }
  }
}

this works because this example waits for chars first. This comment is misleading. connected and there are data available

OK, lets say if tud_cdc_available is used instead of tud_cdc_connected, isn't this function check for input buffer?
I mean without sending anything towards to device, return value will be 0 right? Result will be not available. so we can't send something out of device.

This is how raspberry pi pico sdk implemented. they are using tud_cdc_connected that's why it's happening.
https://github.com/raspberrypi/pico-sdk/blob/master/src/rp2_common/pico_stdio_usb/stdio_usb.c

Kind of you are right it's their problem. I think there must be better implementation for tud_cdc_connected don't you think so?

I think tud_cdc_connected name is misleading. tud_dtr_check, tud_dtr_is_on would be better choice. But that's out of content.

[hathach]

Kind of you are right it's their problem. I think there must be better implementation for tud_cdc_connected don't you think so?

Let me know if you figure out the better way to determine there is a connected client, I am happy to make the changes.

Note: there is a difference in cdc behavior when dtr is not set, the tx buffer will become overwritable, i.e if tx fifo is full, it will be overwritten. more details of this behavior is discussed in previous issue/pr mentioned above.

[MrGreensWorkshop]

Are you referring to #482 ?

[MrGreensWorkshop]

I made some changes and test it's behavior.
I used this code, with changes in stdio_usb.c

first changed this

bool stdio_usb_connected(void) {
    return true;
}

and renamed tud_cdc_connected() to stdio_usb_connected()

When port is opened all data in tx buffer(255 bytes?) output to terminal.
This is not problem because when handling data on pc we can flush buffer.

Is there any other concerns?

[hathach]

as mentioned above, the only difference in behavior is tx fifo can be overwritten when dtr is not set. When dtr is set, tinyusb will blocking wait for tx fifo to be sent since it is certain there is waiting client. In other words, If you send too many data in short period, there maybe some missing.

connected() is optional, going to support dtr or not is up to stdio integration i.e rpi team. For the changes you propose is within pico-sdk, you should ask them to see if it is acceptable or not.

[MrGreensWorkshop]

@hathach Things are more clear now. Thank you for your explanation.

[hathach]

no problem, I am glad that help. The current state of control signals (dtr,rts, cts etc...) in cdc acm are not standardized among terminal clients, though more and more terminal use dtr as connected nowsaday. I will try to revise/improve this later on when having time. I guess we could now close this issue here ?

[MrGreensWorkshop]

I open the this PR on pico-SDK.

Clearly tud_cdc_connected is used by choice of pico-sdk team.

though more and more terminal use dtr as connected nowadays.

About that. It is also mentioned in #482.

Universal Serial Bus Communications Class Subclass Specification for PSTN Devices for Abstract Control Model (ACM) ..

PSTN Devices are mostly modems. Most of time we need flow control to control data flow. Let's not confuse with modems.

The reason that I raise the issue is not about the terminals default settings. It's about the application side. Most people use terminals for test purpose.

As you know more mcu have usb support nowadays, so serial port replace with CDC. People like me want to use cdc without any change in host application. Host serial app could be run on PC, mobile device or host mcu.

Thank you again for this great project.