Possible bug in arduino-usbserial.c

[Daniel-Noethen]

Hi, I posted this already to the Arduino Forum but it did not get any attention there. I just found this GitHub page and think it is a much better place for reporting bugs.

My Arduino UNO Rev3 jumped into my UART Rx interrupt when connecting or changing the baudrate/parity settings with a terminal tool (in this case hTerm). Analyzing the Rx connection of the atmega328p showed me that there are short down pulses on the Rx line. The uC interprets these short pulses as UART data when using a baudrate of 19200Bd and 38400Bd but not when using 9600Bd. Digging through the schematics and the source code of the atmeg16u2 finally brought me to the function EVENT_CDC_Device_LineEncodingChanged() in Arduino-usbserial.c. This function is obviously called every time the user connects to the virtual COM device and also when changing the UART settings within the terminal program. Before applying the new UART settings the UART is disabled with the following lines: /* Must turn off USART before reconfiguring it, otherwise incorrect operation may occur */ UCSR1B = 0; UCSR1A = 0; UCSR1C = 0;

As long as the UART is activated the Tx line of the mega16u2 (M8RXD in the schematic) idles high. Disabling the UART reconfigures the Tx line as an input which results in a short down pulse until the UART is reinitialized.

I was able to fix this behavior by adding the following lines before the UART is disabled:

   /* Prevent Tx1 Pin from going low for a short time when temporarily turning off the USART in the next lines*/    DDRD &= ~(1<<PD3);    PORTD |= 1<<PD3;

Maybe there is a better place to fix this. But the down pulses should be prevented.

Edit: Attached is a screenshot of the pulses

Cheers Daniel

[matthijskooijman]

Wow, obscure bug and impressive find :-)

Your fix sounds reasonably, though perhaps we should not be touching the port register whenever resetting the UART registers, but just once at startup? AFAIU, the port registers will simply keep their state while the UART is being used, but the UART takes over the pin making the port registers unused for this particular pin. Whenever the UART is disabled, the pin falls back to using the PORT registers.

I'm not sure what the policy for updating this code is, though. Since this code is used during manufacturing, a bit of extra care is needed (and just committing it to this repository is not enough to actually get it used). Perhaps the Arduino devs can comment?

[Daniel-Noethen]

Yes, you are totally right. The port initialization should be done only once at startup. I just wasn't in the mood to find a proper place for putting the port initialization after figuring out this bug ;-) Those who are already familiar with the code know best where to put the port initialization.

[cmaglie]

If you're not using the USB2serial connection the TX/RX pins on the mega328 should be kept free to use, maybe this is the reason why the 16u2 sets the port as high-impedance inputs when the USB connection is closed. AFAIU the change you're proposing breaks this behaviour.

@matthijskooijman yes, the firmware should be updated also on production, usually I'll take care to notify the factory when an update occurs. Not to say that every change in firmwares need more careful than ever since is very difficult to update a buggy firmware once burned.

[matthijskooijman]

Ah, cmaglie makes a good point. In that case, it might make more sense to set the pin to high output before disabling the UART and then switching back to input after the UART is reconfigured? Then the pins are still floating when the UART is not used, while not dropping low when reconfiguring the UART?

For reference, code is here: https://github.com/arduino/Arduino/blob/ide-1.5.x/hardware/arduino/avr/firmwares/atmegaxxu2/arduino-usbserial/Arduino-usbserial.c#L204-L216