rogerclarkmelbourne / Arduino_STM32

Arduino STM32. Hardware files to support STM32 boards, on Arduino IDE 1.8.x including LeafLabs Maple and other generic STM32F103 boards
Other
2.49k stars 1.25k forks source link

USB Serial echos characters into the input (i.e. keyboard) #910

Closed ag88 closed 11 months ago

ag88 commented 1 year ago

STM32F103C8 (an old blue pill) https://www.stm32duino.com/viewtopic.php?t=2039 There seemed to be some bugs which I can't find where is the problem

void setup() {
    Serial.begin();
}

void loop() {
    int c;
    print("hello world");
    println();
    if( Serial.available() > 0)
      c = Serial.read();
      // echo the character typed
      Serial.print((char) c); 
    }
    delay(1000);
}

While this isn't the actual codes, but that the character stream "hello world" gets echoed into the keyboard input. I dug around the codes and don't seem to find the problem, I'm not sure if there could be some variable leaks that causes the tx and rx pointers to be swapped etc. When reviewing the codes, the rx and tx buffers (circular buffer) are separately defined. So it seemed strange that it happened. I tried turning off terminal echo from my console and verified that that isn't due to terminal echo, i.e. the problem persist even if terminal echo is turned off.

A strange thing is this is not persistent, it happens at the start after reset, but after a while the problem seemed to resolve itself and the output to input echo did not happen.

ag88 commented 1 year ago

This problem is puzzling and it seemed to be a hardware related issue.

The problem can be resolved by simply defining setup() as

void setup() {

    while(! Serial);

    Serial.begin();
}

void loop() {
    Serial.print("hello world");
    delay(1000);
}

The trouble with the above is that it would stall waiting for a terminal to connect. If that while(! Serial); is removed, characters from the Serial.print("hello world"); gets echoed into the input when a terminal monitor connects. And interestingly, it doesn't repeat and only the 1st line get echoed.

Another fix is

void loop() {
    if (Serial)
        Serial.print("hello world");
    delay(1000);
}

This works as well but avoids the stall waiting for a terminal monitor to connect. I did not do a debug, but reviewed the codes in usb_cdcacm.c and verified that the 'head' and 'tail' variables handling the ring buffer aren't mixed up, they looked correct as I read that several times. Similarly for USBSerial I tried messing with USBSerial.write(char c) by using a memory buffer e.g. char[5] wrbuf; and writing and reading from this buffer, but that did not fix the issue. Hence, this isn't a case of 'pointer mixup', and it seemed to be related to initialization as while(!Serial) fixes the issue as it waits for a serial monitor to connect. And that the problem apparently only happens without that while(!Serial) when a serial monitor connects (in my case from Linux) and only for the 1st line.

dewhisna commented 1 year ago

I'm curious ... in the failing case, was the USB still in the fully-connected state? i.e. gone through device enumeration with the PC, just with no open terminal connection? Or was the USB subsystem still in limbo?

I ask, because when I read this thread, the first thought that popped in my head was that it was somehow related to USBComposite and whether it was in the "ready" state with enumeration complete with its devices ready to be used or not, since technically, unlike USART Serial, USB Serial can't send or receive anything until the USB connection is complete. USART will trickle the data out on the Rx/Tx pins regardless of whether or not a device is there to hear it, but USB's protocol really wants a connection established first.

I've had similar issues with a USB MIDI connection, not really repeating data issues but where there were hangs and weird buffer behavior, where it was necessary to check the USBComposite.isReady before trying to pump Tx events. And then on the first connect, in its begin handler, do usbReset for that composite part and clear the Rx/Tx buffers and the transmit state-machines.

In other words, you can't transmit anything on USB until the USBComposite is ready. Much of the original Serial logic was written for USART output and might not take USBComposite state into account. Or that's my theory anyway...

ag88 commented 12 months ago

nope, it is the normal usb cdc acm driver.

When this occurred, USB i.e. Serial is not (yet) initialized, or at least not fully initialized.

This happens if you don't block waiting for a terminal to connect by adding a

void setup() {
   Serial.begin();
   while( ! Serial) ;
}

That while( ! Serial ) ; clause cause Serial to block the main thread and wait for the host serial terminal / monitor to connect. Without this while( ! Serial ) ;, the symptom can be observed the moment a host serial terminal / monitor is connected, the Serial.print( "bla bla bla") ; has to be happening e.g. in loop() prior to thoe host serial terminal / monitor being connected.

My suspicion is messages in the Serial.print( "bla bla bla") ; prior connection gets 'echoed' into the input upon 1st connection. This is observed because, shortly after the initial 'echo' ing into input, the problem resolve itself and no more echo into input is observed after a while. But this is just speculation, it would require quite deep in debug or checks to isolate the issue in depth.

if you add thewhile( ! Serial ) ; clause in setup(), or the other way by testing for if (Serial) Serial.print("bla bla bla");, the problem is resolved, the issue won't be observed.

My thoughts are that to treat this as a 'feature' until 'more people' observe the problem and investigate it themselves and perhaps isolate the root cause. A solution has been mentioned and should resolve the issue. It may not be easy to fix (as the problem may not be in the code, it may even be related to the compiler / gcc version used and the flags used. I'm using a custom Makefile and not Arduino IDE to build my sketch. https://www.stm32duino.com/viewtopic.php?t=37

ag88 commented 12 months ago

Note also that for MIDI if you use HID, can be different from this. HID use 'interrupt' transfers, which is quite different from current USB Serial (CDC ACM) implementation which use bulk-in bulk-out transfers. mixing protocols e.g. with additionally Serial in addition to HID could add to the complexity.

if you play with HID, it would be good to learn the USB HID protocols which is complex. HID do not 'automatically work' with hosts (e.g. Windows or Linux) and may require custom divers. HID is only 'driverless' for the most basic HID devices such as keyboards and mice

dewhisna commented 12 months ago

When this occurred, USB i.e. Serial is not (yet) initialized, or at least not fully initialized.

That does sound like the same thing I see with MIDI and the composite driver. And the second "fix" you report:

void loop() {
    if (Serial)
        Serial.print("hello world");
    delay(1000);
}

is essentially the same as my if (USBComposite.isReady()) usb_midi_tx(...).

In other words, the Serial object print function itself should internally just do the equivalent of if (Serial) print. i.e. if the port isn't ready, the print function shouldn't be trying to print and instead should just return.

ag88 commented 12 months ago

In my code, I checked for characters in the buffer with

if (Serial.available() ) {
  int c = Serial.read();
   ...
}

The thing is this check did not prevent the 'echo'ing of prior 'output' into this. I'm guessing that Serial can be reinitialized the moment a serial monitor connects. this would make the if ( Serial ) ... test true.

The problem is simply resolved by not doing Serial print if Serial is testing false, and that alone prevent the 'echo'ing of prior outputs into inputs.

In context, Serial is a variable and you can assume that it is null when nothing is connected, the variable cannot initialize itself , hence, the code bascially checks that Serial is a real variable - not null. In reality, it can be more complicated than that. i.e. that the simplified explanation is that 'Serial' is not there , not created in the prior Serial.print() before a terminal is fully and properly connected. Things can be more complicated than this simplified view.

as for MIDI, it'd need to be evaluated in its own context, it may not be related to this.

for HID more reading materials can be found here: https://www.usb.org/hid https://eleccelerator.com/tutorial-about-usb-hid-report-descriptors/ https://learn.adafruit.com/custom-hid-devices-in-circuitpython/report-descriptors etc

messing with usb can take skills like playing with USB sniffers e.g. https://wiki.wireshark.org/CaptureSetup/USB https://www.google.com/search?q=windows+usb+sniffer and of course an understanding of the underlying protocols and to review the 'driver' or 'library' codes

stevstrong commented 12 months ago

In other words, the Serial object print function itself should internally just do the equivalent of if (Serial) print. i.e. if the port isn't ready, the print function shouldn't be trying to print and instead should just return.

Well, that isn't that obvious. Some users would like to have the opposite behaviour, to see all messages from the beginning. If I remember correctly, there is one flag in the CDC driver which controls this behaviour: buffer all data even before connection complete or throw them away. But actually there is an internal buffer (configurable size, currently I think is 64 bytes but I might be wrong) which holds all characters (up to the buffer size) before complete renumeration is done and send them after that.

So I do not think that this is a bug.

What is really confusing and what I cannot understand how and where the "echoing" could take place. I mean the echo needs some already sent characters. But what to echo if the connection is not yet established?

So @ag88 can you please give more information about how exactly do you proceed, what do you see on which terminal? Which STM32 board?

Furthermore, as I could not reproduce the issue with Arduino, there is still a chance that your custom compile and build process has some strange side effect.

ag88 commented 12 months ago

I managed to reproduce the problem with this sketch

#include <Arduino.h>

void setup() {

    Serial.begin();

    pinMode(LED_BUILTIN, OUTPUT);
}

void loop() {

    Serial.println("hello world");

    if(Serial.available()) {
        int c = Serial.read();
        Serial.println((char) c);
    }

    digitalWrite(LED_BUILTIN, !digitalRead(LED_BUILTIN));

    delay(1000);
}

sample output as follows:

hello world
hello world
hello world
hello world
hello world
hello world
hello world
hello world
hello world
hello world
hello world
hello world
hello world
hello world
hello world
hello hello world
hhello world
ehello world
lhello world
lhello world
ohello world
 hello world
whello world
ohello world
rhello world
lhello world
dhello world
hello world
hello world
hello world
hello world
hello world
hello world
hello world
hello world

for the above board is STM32F103C8 blue pill using the generic_stm32f103c variant. I'm using 'roger's bootloader' maple DFU bootloader https://github.com/rogerclarkmelbourne/STM32duino-bootloader using specifially generic_boot20_pc13.bin

below reproduced on maple mini clone STM32F103CB using the maple_mini variant.

hello world
hello world
hello world
hello world
hello world
hello world
hello world
hello world
hello world
hello world
hello world
hello world
hello world
hello world
h
hello world
e
hello world
l
hello world
l
hello world
o
hello world

hello world
w
hello world
o
hello world
r
hello world
l
hello world
d
hello world
hello world
hello world
hello world
hello world

the serial monitor is Putty https://www.chiark.greenend.org.uk/~sgtatham/putty/latest.html and note that the OS environment is Linux In addition, to reproduce the issue, connect the board e.g. maple mini or blue pill let it run for the 1st few seconds, then run the terminal monitor and connect. That 'few seconds' is normally after connecting, then it takes that little while to search for the app, launch it, selecting the menus and connect (no need to rush with this).

Note however that I'm using a makefile https://www.stm32duino.com/viewtopic.php?t=37 there is a bunch of flags I'm using and it is mostly in the makeffile

and it isn't arduino IDE that builds the bin file. gcc is old: gcc-arm-none-eabi-8-2018-q4-major

the bulid output summary using the makefile looks like this

   text    data     bss     dec     hex filename
  12088    1176    1032   14296    37d8 bin/maple_mini.elf

-rwxr-xr-x 1 andrew users 13264 Jul 12 23:18 bin/maple_mini.bin

bin/maple_mini.elf:     file format elf32-littlearm

Sections:
Idx Name          Size      VMA       LMA       File off  Algn
  0 .text         00002ba4  08002000  08002000  00002000  2**4
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  1 .text.align   00000004  08004ba4  08004ba4  00004ba4  2**0
                  ALLOC, CODE
  2 .ARM.exidx    00000008  08004ba8  08004ba8  00004ba8  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, DATA
  3 .data         00000498  20000000  08004bb0  00010000  2**2
                  CONTENTS, ALLOC, LOAD, DATA
  4 .rodata       00000388  08005048  08005048  00015048  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, DATA
  5 .bss          00000408  20000498  20000498  00020498  2**2
                  ALLOC
  6 .debug_aranges 00001208  00000000  00000000  000153d0  2**3
                  CONTENTS, READONLY, DEBUGGING
  7 .debug_info   00005602  00000000  00000000  000165d8  2**0
                  CONTENTS, READONLY, DEBUGGING
  8 .debug_abbrev 00002267  00000000  00000000  0001bbda  2**0
                  CONTENTS, READONLY, DEBUGGING
  9 .debug_line   00007932  00000000  00000000  0001de41  2**0
                  CONTENTS, READONLY, DEBUGGING
 10 .debug_frame  00002994  00000000  00000000  00025774  2**2
                  CONTENTS, READONLY, DEBUGGING
 11 .debug_str    0000390e  00000000  00000000  00028108  2**0
                  CONTENTS, READONLY, DEBUGGING
 12 .ARM.attributes 0000002b  00000000  00000000  0002ba16  2**0
                  CONTENTS, READONLY
 13 .debug_ranges 00001d80  00000000  00000000  0002ba41  2**0
                  CONTENTS, READONLY, DEBUGGING
 14 .comment      00000075  00000000  00000000  0002d7c1  2**0
                  CONTENTS, READONLY

Source dirs
src
./STM32F1/cores/maple
./STM32F1/variants/maple_mini

src/
src/rtc/
src/spi/
src/spiflash/
./STM32F1/cores/maple/
./STM32F1/cores/maple/avr/
./STM32F1/cores/maple/libmaple/
./STM32F1/cores/maple/libmaple/stm32f1/performance/
./STM32F1/cores/maple/libmaple/usb/stm32f1/
./STM32F1/cores/maple/libmaple/usb/usb_lib/
./STM32F1/cores/maple/stm32f1/
./STM32F1/variants/maple_mini/
./STM32F1/variants/maple_mini/wirish/

Includes
-ISTM32F1/cores/maple/
-ISTM32F1/system/libmaple/
-ISTM32F1/system/libmaple/include/
-ISTM32F1/system/libmaple/stm32f1/include
-ISTM32F1/system/libmaple/stm32f1/include/series/
-ISTM32F1/system/libmaple/usb/stm32f1/
-ISTM32F1/system/libmaple/usb/usb_lib/
-ISTM32F1/variants/maple_mini/
-Isrc/rtc/
-Isrc/spi/
-Isrc/spiflash/

Defines
-DMCU_STM32F103CB
-D__STM32F1__
-DVECT_TAB_ADDR=0x8002000
-DCONFIG_MAPLE_MINI_NO_DISABLE_DEBUG
-DDEBUG_LEVEL=DEBUG_NONE
-DF_CPU=72000000L
-DSERIAL_USB
-DUSB_VID=0x1EAF
-DUSB_PID=0x0004
-DUSB_MANUFACTURER="Unknown"
-DSPI_HAS_TRANSACTION

the codes for STM32F1 folder is from a recent clone from github on 5 July 2023.

stevstrong commented 12 months ago

Can you share your BIN files please?

ag88 commented 12 months ago

bin file and some other outputs for blue pill generic_stm32f103c.zip

bin file and some other outputs for maple mini maple_mini.zip

the makefile used makefile.zip

note that all these use Roger's libmaple USB DFU bootloader https://github.com/rogerclarkmelbourne/STM32duino-bootloader specifically: generic_boot20_pc13.bin

dewhisna commented 12 months ago

@stevstrong --

In other words, the Serial object print function itself should internally just do the equivalent of if (Serial) print. i.e. if the port isn't ready, the print function shouldn't be trying to print and instead should just return.

Well, that isn't that obvious. Some users would like to have the opposite behaviour, to see all messages from the beginning.

But you can't "see all messages from the beginning" on a normal UART serial stream ... it will send it immediately and they will be lost if nothing was connected to the Tx/Rx lines to see it... Why do you want the USB Serial version to behave differently from UART Serial when all the USB variant really is is just bringing the UART->USB adapter, as it were, inside of the STM32F1?

ag88 commented 12 months ago

@dewhisna usb uart dongles/chips do not all necessary work the same way, I think there are some implementations which may buffer some data on its way to the host. But that it is actually quite different from a direct USB implementation. e.g. if you use the built-in USB Serial driver, you can achieve like 1 Mbps or more as USB disregards 'baud rates'. Some uarts may top out at like 115200 bps. With USB it is a fixed 12 mpbs, and time division multiplexed between devices. So if you have many devices (including all the hubs (some of them hidden), mouse, keyboard etc, that bandwidth is shared and the remaining bandwidth for your particular device become much less than 12 mbps.

For the time being, it is unknown if this behavior is after all reproducible. Then we can look at how to 'solve' it. And as mentioned prior, if it is simply a matter of the variable being NULL , in most desktop OS, the program will simply abort with an error if it is run, i.e. calling a method of a NULL object, all that NULL pointer exceptions and segmentation faults. Then to prevent that, a most practical (and sometimes only) way is to check that it is not NULL. A variable can't check itself being NULL..

note that in this embedded land, your sketch can build a stack that overwrite your global variables and there is no warning no nothing. and that many devices has very little ram, e.g. stm32f103c8 has only 20k sram, it is pretty easy for stacks to crash into variables and simply overwrite them. On desktop OS, the program will simply abort, but in embedded land, the firmware can still run even if it corrupts the variables and data.

But that we are not yet at the bottom of this as it would take 'everyone' practically being able to reproduce this 'behavior' prior to deciding what may be an appropriate 'fix'.

dewhisna commented 12 months ago

@ag88 -- yes, I'm aware of the enhanced capabilities of USB over UART serial. I'm just saying that Serial should behave the same regardless of the connection medium. It's about app consistency and portability. If the app wants to explicitly use a serial channel, like USB, with better performance and different capabilities, it should explicitly do so with a USB.something.serial.print() or similar call and know that it's different, not just call Serial and have it randomly behave different just because the underlying implementation is USB Serial and not UART Serial. Serial should behave like Serial and be the same regardless.

rogerclarkmelbourne commented 12 months ago

I've tried to reproduce the 'echoing' problem on 2 different Windows 10 computers using 2 different serial terminals and both work fine

There was no echoing when using the Arduino IDE to compile and using its terminal, and also when using the Herculese terminal exe there was also no echoing

One machine was a clean installation of the Arduion_STM32 (this repo), and clean install of the compiler by selecting an Arduino SAM board, and the other was an existing older installtion.

Most likely what you are seeing is a product of either your operating environment or build enviroment, e.g. OS, drivers, compiler version, and not using the Arduino

It is not a bug, becuase the problem does not occur if you use the repo for the purpose it was intended.

I see you have forked the repo, so you can of course make your own version which works with your own individial enviroment.

But I see no reason to continue any further investigation into the operation of this core for its normal / intended use, as it works fine.

ag88 commented 11 months ago

hi, no worries, lets close the issue as this is likely related to my custom compile, as I'm using a Makefile to build the sketch, so the flags etc which is likely different could be a possible cause.