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 Serial send blocks when host closes connection #106

Open
NicoHood opened this Issue Dec 4, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@NicoHood
Copy link
Contributor

NicoHood commented Dec 4, 2017

Sample code:

    while(true)
    {
        LED_ON();
        _delay_ms(500);
        LED_OFF();
        _delay_ms(500);
        for(uint8_t i = 0; i<100;i++){
            if(VirtualSerial_CDC_Interface.State.ControlLineStates.HostToDevice){
                CDC_Device_SendByte(&VirtualSerial_CDC_Interface, 'a');
            }            
        }
  }

The problem is also described here:
http://www.avrfreaks.net/forum/lufa-usb-cdc-detect-when-host-application-connects

The DTR line determines if the host opened the serial port or not. If DTR is set, you should be able to send. Problem1: The CDC class driver does not check the DTR line for sending, only if baud != 0. If you manually add the check there is a remaining Problem2: The host does not clear anything if he closes the port (not even a single setup request happens).

I've tried the Arduino USB core, which does seem to handle the problem like this:

  • Do a DTR check
  • On the first error of a writing stream, abort

In this case it really helps to stop sending if you know the USB device will timeout. This might work for sending a stream (println), but in my case I am using stdio/printf so every byte will be sent on its own.

Arduino sketch comparison:

void loop() {
  digitalWrite(LED_BUILTIN, HIGH);   // turn the LED on (HIGH is the voltage level)
  delay(1000);                       // wait for a second
  digitalWrite(LED_BUILTIN, LOW);    // turn the LED off by making the voltage LOW
  delay(1000);                       // wait for a second
  //Serial.print("testegorogkrhtrhkhprtkpj\n"); <-- This works fine
  for(uint8_t i = 0; i<100;i++){
      Serial.write('a');  // <- This also crashes
  }
}

I am using ArchLinux:

Linux hostname 4.9.65-1-lts #1 SMP Fri Nov 24 13:02:16 CET 2017 x86_64 GNU/Linux

I am not sure if this a linux problem and since when this happens. Maybe someone can check on older kernels or windows. If they behave the same, we should try to fix it.

The only solution I can imagine is to set an error flag and do not accept sending requests until a timeout is reached.

@NicoHood

This comment has been minimized.

Copy link
Contributor

NicoHood commented Dec 4, 2017

This is my current solution. It waits for the next SOF event (~1ms) and then enables the serial again.

Note: This does not include any DTR check, the code still works fine. Seems to be the best solution to me. The user has to disable the flag his self though. It requires updating all examples. Some #ifdef might also be required for the case no SOF are used.

bool CDC_Device_Error = false;

uint8_t CDC_Device_SendByte(USB_ClassInfo_CDC_Device_t* const CDCInterfaceInfo,
                            const uint8_t Data)
{
	if (CDC_Device_Error || (USB_DeviceState != DEVICE_STATE_Configured) || !(CDCInterfaceInfo->State.LineEncoding.BaudRateBPS))
	  return ENDPOINT_RWSTREAM_DeviceDisconnected;

	Endpoint_SelectEndpoint(CDCInterfaceInfo->Config.DataINEndpoint.Address);

	if (!(Endpoint_IsReadWriteAllowed()))
	{
		Endpoint_ClearIN();

		uint8_t ErrorCode;

		if ((ErrorCode = Endpoint_WaitUntilReady()) != ENDPOINT_READYWAIT_NoError)
        {
            if(ErrorCode == ENDPOINT_READYWAIT_Timeout){
                CDC_Device_Error = true;
            }
            return ErrorCode;
        }
	}

	Endpoint_Write_8(Data);
	return ENDPOINT_READYWAIT_NoError;
}
extern bool CDC_Device_Error;

/** Event handler for the USB device Start Of Frame event. */
void EVENT_USB_Device_StartOfFrame(void)
{
    uint8_t PrevSelectedEndpoint = Endpoint_GetCurrentEndpoint();

    // Flush CDC serial
    CDC_Device_Error = false;
    CDC_Device_USBTask(&VirtualSerial_CDC_Interface);

    Endpoint_SelectEndpoint(PrevSelectedEndpoint);
}

As a more generic solution we could also use an uint8_t which can indicate up to 8 endpoint failures and store the endpoint number inside. This would also make more sense if you are using two serial interfaces at the same time.

@NicoHood

This comment has been minimized.

Copy link
Contributor

NicoHood commented Dec 4, 2017

This is a more generic solution which works for ALL IN endpoints. The reset of the timeout-flag-byte can be placed inside the lufa core itself, which is way more handy.

I am not sure if the addition is useful for every endpoint/device. However we do have some options here:

  • Disable the feature via configuration file
  • Add an endpoint mask which tells the driver which endpoints should have this feature. This requires knowing the endpoints from Descriptors.h at compile time.
uint8_t EndpointInTimeout = 0;

#if !defined(CONTROL_ONLY_DEVICE)
uint8_t Endpoint_WaitUntilReady(void)
{
	#if (USB_STREAM_TIMEOUT_MS < 0xFF)
	uint8_t  TimeoutMSRem = USB_STREAM_TIMEOUT_MS;
	#else
	uint16_t TimeoutMSRem = USB_STREAM_TIMEOUT_MS;
	#endif

    uint8_t ep = Endpoint_GetCurrentEndpoint();
    uint8_t ep_mask = (1 << (ep & ENDPOINT_EPNUM_MASK));

	uint16_t PreviousFrameNumber = USB_Device_GetFrameNumber();

	for (;;)
	{
		if (ep & ENDPOINT_DIR_IN)
		{
			if (Endpoint_IsINReady())
			  return ENDPOINT_READYWAIT_NoError;

            if (EndpointInTimeout & ep_mask) {
              return ENDPOINT_READYWAIT_Timeout;
            }
		}
		else
		{
			if (Endpoint_IsOUTReceived())
			  return ENDPOINT_READYWAIT_NoError;
		}

		uint8_t USB_DeviceState_LCL = USB_DeviceState;

		if (USB_DeviceState_LCL == DEVICE_STATE_Unattached)
		  return ENDPOINT_READYWAIT_DeviceDisconnected;
		else if (USB_DeviceState_LCL == DEVICE_STATE_Suspended)
		  return ENDPOINT_READYWAIT_BusSuspended;
		else if (Endpoint_IsStalled())
		  return ENDPOINT_READYWAIT_EndpointStalled;

		uint16_t CurrentFrameNumber = USB_Device_GetFrameNumber();

		if (CurrentFrameNumber != PreviousFrameNumber)
		{
			PreviousFrameNumber = CurrentFrameNumber;

			if (!(TimeoutMSRem--)){
                if (ep & ENDPOINT_DIR_IN) {
                    EndpointInTimeout |= ep_mask;
                }
                return ENDPOINT_READYWAIT_Timeout;
            }

		}
	}
}
#endif

@NicoHood NicoHood changed the title CDC Serial send blocks when device disconnects CDC Serial send blocks when host closes connection Dec 4, 2017

NicoHood added a commit to NicoHood/lufa that referenced this issue Dec 29, 2017

@abcminiuser abcminiuser added the bug label Dec 31, 2017

@NicoHood

This comment has been minimized.

Copy link
Contributor

NicoHood commented Jan 16, 2018

Update: It turns out my fix is working properly but not enough for normal usecases. Example:

I am reading Infrared input and debug print those to the serial. On a special keypress I translate those to keyboard commands, for example turning up the volume.

Without the fix (PR #110) the keypress reaches the host after a loooong timeout (as I am printing much debug info and every stdio call to putchar requires a timeout.

With the PR it works better but if I hold down the key it sometimes aborts. This is possibly because my IR library relies on interrupts and those are disabled while sending the serial bytes? Or it just takes too long that the IR lib just times out. As a sideffect we also do not know which other devices the PR influences, so its better to not merge it.

Conclusion: PR #110 is (sadly) not enough. The best way to solve this issue is to check the DTR state. The problem however is, that if the host opens the port once, it leaves DTR enabled. I guess that is the limitation we have. But the program with still work if you never open the (in most cases) debug serial. Update: With kernel 4.14.13 (Arch Linux) it is working properly again. So checking the DTR state is really a reasonable solution!

I've implemented the new fix here: #120

It would really help and also it would behave similar to the Arduino implementation. They also had no better solution so far.

NicoHood added a commit to NicoHood/lufa that referenced this issue Jan 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment