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

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

Closed
MrGreensWorkshop opened this issue Jul 8, 2022 · 15 comments
Labels
can't reproduce Can not reproduce the bug

Comments

@MrGreensWorkshop
Copy link
Contributor

MrGreensWorkshop commented Jul 8, 2022

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 caused by tinyusb lib. That's why I open this issue also in tinyusb repo.

please watch the gif file.

raspi pico serial dtr

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.

usual usb serial

Cause

This happens when raspberry pi usb cdc is used.
Discussed in here hathach/tinyusb#1548 and hathach/tinyusb#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);
}
@lurch
Copy link
Contributor

lurch commented Jul 8, 2022

Are you sure that this isn't a quirk of your particular serial terminal program? I never had to fiddle with any DTR settings when using PuTTY as detailed in https://datasheets.raspberrypi.com/pico/getting-started-with-pico.pdf

@MrGreensWorkshop
Copy link
Contributor Author

MrGreensWorkshop commented Jul 8, 2022

yes i'm sure. Check this out. This is with CoolTerm on MacOS.
data only received if only DTR is enabled.

CoolTermMac_term

@MrGreensWorkshop
Copy link
Contributor Author

MrGreensWorkshop commented Jul 8, 2022

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. That's the problem. Asking host application for DSR on is reduce the compatibility with other apps if user is explicitly select no flow control. I'm saying this because if you check 2 FDTI USB serial converter example gif, it doesn't uses DTR. If flow control set to none most 3rd party apps only uses RX, TX, GND.

@aallan
Copy link
Contributor

aallan commented Jul 8, 2022

I've never used any flow control when talking to an RP2040-based board. I've never even connected up the CTS/RTS or DSR/DTR pins when using UART adaptors, just RX/TX and GND. I can't replicate this problem. Downloading the UF2 binary in the OP's repo it works as expected on my machine.

IMG_2376

Screenshot 2022-07-08 at 10 37 20

@aallan aallan closed this as completed Jul 8, 2022
@aallan aallan added the can't reproduce Can not reproduce the bug label Jul 8, 2022
@MrGreensWorkshop
Copy link
Contributor Author

MrGreensWorkshop commented Jul 8, 2022

you used screen command right? its also uses VT100.

I'm talking about CDC not UART. I can see that you connect the serial over rx,tx,gnd pins.

@aallan
Copy link
Contributor

aallan commented Jul 8, 2022

you used screen command right? its also uses VT100

I did not use the screen command.

Screenshot 2022-07-08 at 11 00 12

@MrGreensWorkshop
Copy link
Contributor Author

MrGreensWorkshop commented Jul 8, 2022

This is my setup.

Raspberry pi pico -> USB thats all. No usb serial converters attached.

This happens when raspberry pi usb cdc is used.

@aallan
Copy link
Contributor

aallan commented Jul 8, 2022

The UF2 binary you provided doesn't have CDC enabled, only UART?

Oops. No, I'm wrong. I wasn't paying enough attention. Sorry.

However, it turns out I am connected via CDC via USB, not UART serial at all. 🤦‍♂️

IMG_2377

Screenshot 2022-07-08 at 11 36 57

@MrGreensWorkshop
Copy link
Contributor Author

Yes, I checked again just to be 100% sure. I downloaded the file and write to my pico. Yes I only check CDC over USB. I didn't even check uart output because you can clearly see my setting that I disabled UART I'm not talking about uart.

https://github.com/MrGreensWorkshop/RasPiPicoSDK_cdc_test/blob/main/CMakeLists.txt

# Enable USB CDC
pico_enable_stdio_usb(PicoTest 1)

# Disable UART
pico_enable_stdio_uart(PicoTest 0)
 
# Enable extra outputs (bins)
pico_add_extra_outputs(PicoTest)

@MrGreensWorkshop
Copy link
Contributor Author

MrGreensWorkshop commented Jul 8, 2022

As I mentioned earlier here if you use VT100 terminal you can't catch it.

May I kindly ask you, please check with Coolterm as I mentioned here

Please make sure initial line states for DTR, RTS is off.

CoolTermMac_term

Thank you.

@JamesH65
Copy link
Contributor

JamesH65 commented Jul 8, 2022

Might be worth flagging this up on the tinyUSB GitHub issue tracker, since it is their code.

@MrGreensWorkshop
Copy link
Contributor Author

"can't reproduce" feels sad.

@MrGreensWorkshop
Copy link
Contributor Author

MrGreensWorkshop commented Jul 19, 2022

@JamesH65

Might be worth flagging this up on the tinyUSB GitHub issue tracker, since it is their code.

The problem is how tinyUSB implemented in pico-sdk source code.
https://github.com/raspberrypi/pico-sdk/blob/master/src/rp2_common/pico_stdio_usb/stdio_usb.c

tud_cdc_connected is used for checking connection but under the hood its checking DTR. I don't think using tud_cdc_connected is appropriate way to check the connection.

Could you please consider to reopen the issue? Thank you.

src/class/cdc/cdc_device.h

...
static inline bool tud_cdc_connected (void)
{
  return tud_cdc_n_connected(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);
}
...

I made a PR #932 which gives user option to disable DTR check.

@JamesH65
Copy link
Contributor

Not my call to reopen, @kilograham?

@MrGreensWorkshop
Copy link
Contributor Author

MrGreensWorkshop commented Jul 19, 2022

@JamesH65 Things are more clear now. You don't need to reopen the issue. I made a PR #932, but label can't reproduce could be removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
can't reproduce Can not reproduce the bug
Projects
None yet
Development

No branches or pull requests

4 participants