USB enhancement

[fpistm]

List:

• Timers removal, rebased to master. Also fixed PMA memory distribution #410 merged (include some typos and warnings fixes)
• HID composite clean up
• USB_DISC_PIN and re-enumeration --> Covered by [USBD] Force re-enumeration #426
• Serial and Serial Event management --> Covered by Generic Serial #424
• readBytes and readBytesUntil thanks @makarenya (Timers removal, rebased to master. Also fixed PMA memory distribution #410)

Current Status:
FS well functional.
HS ok except ReadByte test.

[ktand]

@fpistm @makarenya

I've tested this PR on STM32F407 and the performance is just about the same as with the timer based CDC. RAM usage is much better:

USB enhancement:

Sketch uses 22540 bytes (2%) of program storage space. Maximum is 1048576 bytes.
Global variables use 3460 bytes (1%) of dynamic memory, leaving 193148 bytes for local variables. Maximum is 196608 bytes.

Average bytes per second = 635948

Timer based USB:

Sketch uses 24260 bytes (2%) of program storage space. Maximum is 1048576 bytes.
Global variables use 9564 bytes (4%) of dynamic memory, leaving 187044 bytes for local variables. Maximum is 196608 bytes.

Average bytes per second = 633275

Unfortunately, using a simple Arduino test program that just echoes back what it receives there seems to be character loss:

#include <Arduino.h>
#include "USBSerial.h"

void setup()
{
    SerialUSB.begin(250000);

    pinMode(LED_BUILTIN, OUTPUT);
}

void loop()
{
    if (SerialUSB.available() > 0)
    {
       String s=SerialUSB.readStringUntil('\r');
       SerialUSB.print(s+'\r');
       
       digitalToggle(LED_BUILTIN);
    }
}

The PC programs just sends an increasing number of bytes then reads and compares the number of bytes sent/received:

...
Wrote: 156 Read: 156
Wrote: 157 Read: 157
Wrote: 158 Read: 158
Wrote: 159 Read: 159
Wrote: 160 Read: 160
Wrote: 161 Read: 64 Expected = 161
Wrote: 162 Read: 162
Wrote: 163 Read: 64 Expected = 163
Wrote: 164 Read: 164
Wrote: 165 Read: 165
Wrote: 166 Read: 166
Wrote: 167 Read: 167
Wrote: 168 Read: 168
Wrote: 169 Read: 169
Wrote: 170 Read: 170
Wrote: 171 Read: 171
Wrote: 172 Read: 172
Wrote: 173 Read: 173
Wrote: 174 Read: 174
Wrote: 175 Read: 175
Wrote: 176 Read: 176
Wrote: 177 Read: 177
Wrote: 178 Read: 178
Wrote: 179 Read: 179
Wrote: 180 Read: 180
Wrote: 181 Read: 181
Wrote: 182 Read: 182
Wrote: 183 Read: 183
Wrote: 184 Read: 184
Wrote: 185 Read: 185
Wrote: 186 Read: 186
Wrote: 187 Read: 187
Wrote: 188 Read: 188
Wrote: 189 Read: 189
Wrote: 190 Read: 190
Wrote: 191 Read: 191
Wrote: 192 Read: 105 Expected = 192
Wrote: 193 Read: 88 Expected = 193
Wrote: 194 Read: 108 Expected = 194

I assume this is caused by the smaller buffers but not being able to transfer 160 bytes is a limitation.

[makarenya]

Can you attach a client application to make it easier to test for similar errors?

[makarenya]

I'm trying to reproduce this bug, but...

python code on host:

import serial
ser = serial.Serial('/dev/ttyACM0', 9600, timeout=1)
for i in range(1000):
    ser.write('{}\n'.format(i).encode('utf-8'))
    s = ser.readline().strip().decode('utf-8')
    if s == str(i):
        print('Wrote: {} Read: {}'.format(i, s))
    else:
        print('Wrote: {} Read: {} Expected = {}'.format(i, s, i))

code on STM32F103C8T6

#include <Arduino.h>
#include "USBSerial.h"

void setup()
{
  SerialUSB.begin(250000);
  pinMode(LED_BUILTIN, OUTPUT);
}

void loop()
{
  if (SerialUSB.available() > 0)
  {
    String s=SerialUSB.readStringUntil('\n');
    SerialUSB.print(s+'\n');
    digitalToggle(LED_BUILTIN);
  }
}

same as yours, except it uses '\n' instead of '\r'

And it produces no errors at all. On the STM32F103...
Today, my STM32F407 board makes a fascinating journey through the vast expanses of China and Russia, on the broken wings of the postal services of these countries, and I can test it only in a month or two...

[fpistm]

This is What I thought, some bytes are Lost. This mainly depends of the cpu speed. This is why HS on F7 is not ok.

[makarenya]

I think I know the answer. The fact is that a prerequisite for flawless operation is that the USBD_CDC_TransmitPacket method (& hUSBD_Device_CDC); on cores/arduino/stm32/usb/cdc/usbd_cdc_if.c:259 will write all the data transferred to it to the allocated memory at once. But in fact, he is not obliged to do this! It will only do this if the size of the transferred data is less than the size of the endpoint buffer, and this is at least 100% true for FS!

[makarenya]

I will read the code for STM32F407 today and see what and where bugs could have been generated.

[ktand]

python code on host:

import serial
ser = serial.Serial('/dev/ttyACM0', 9600, timeout=1)
for i in range(1000):
    ser.write('{}\n'.format(i).encode('utf-8'))
    s = ser.readline().strip().decode('utf-8')
    if s == str(i):
        print('Wrote: {} Read: {}'.format(i, s))
    else:
        print('Wrote: {} Read: {} Expected = {}'.format(i, s, i))

@makarenya

Your test program is not doing the same as mine. I send a buffer with an increasing size, you send a buffer containing a string represenation of an increasing value.

Try this instead:

import serial
ser = serial.Serial('/dev/ttyACM0', 9600, timeout=1)
for i in range(1000):
    ser.write('x' * i + '\n') 
    r = len(ser.readline().strip())
    if r == i:
        print('Wrote: {} Read: {}'.format(i, r))
    else:
        print('Wrote: {} Read: {} Expected = {}'.format(i, r, i))

[RickKimball]

Has anyone tried the teensy test code? https://www.pjrc.com/teensy/benchmark_usb_serial_receive.html . It might be useful to see how this code compares to other boards

[BennehBoy]

@RickKimball I, and others, tried it on the parent USB PR #388: speeds were similar to above.

[fpistm]

@RickKimball yes, this is how we bench the speed. Anyway, I will try to find/implement a test for send and also check data integrity.
I've the same result than @BennehBoy. Speed are very closed with current implementation and this PR. The main improvements is the code size and free timer.
Here the result of #388:
https://github.com/stm32duino/Arduino_Core_STM32/files/2746352/usb_serial_receive_results.xlsx
Note that now H7 is functional and I've tested also F0. So all supported STM32 series are functional with USB.

[makarenya]

All errors are fixed, PR are updated!

[makarenya]

Can someone share a speed test? It's interesting to try double buffering and generally look at USB optimization options.

[fpistm]

Thanks @makarenya. I will test it tomorrow. Currently I do not have a full speed test. We only use the one from PJRC. https://www.pjrc.com/teensy/benchmark_usb_serial_receive.html

[fpistm]

I've added @makarenya fixes. Thanks ;)
HS speed test seems now functional even if speed seems impacted (Previous: 3915181, now 1897899).
FS has the same speed.
I've only made speed test no data integrity.

[ktand]

@makarenya

Tried the speed test on STM32F407 and speed is the same. Also tried the echo test program and now there is no lost characters! Great work!

[makarenya]

I can try to speed up it on f103 board (it has endpoint double buffering feature, that must speed it up). But i today i have no other stm32 boards... however i bough f407 and am waiting for the postal service

[BennehBoy]

Happy to test any f4 speed improvements here...

[ktand]

I'm also happy to test any F4 speed improvments.

Maybe one optimization would be to implement the readBytes() function as done in Teensyduino? (ref USB Virtual Serial Receive Speed)

[fpistm]

Yes, of course. This is in the list. Forgot to add it.

[makarenya]

#410 updated, added bulk readBytes method and some other small speed improvements. Can you test it?

[fpistm]

Thanks @makarenya
I will test it this afternoon

[fpistm]

I've updated the PR with @makarenya speed improvements.
My first tests show that speed is a bit better.
I'm currently not succeeded to use readBytes() with the PJRC tests. :(
But I need to go deeper to investigate and tests more

[fpistm]

Thanks @makarenya for the last fixes.
readBytes() works nows for FS. For HS it seems it still an issue but I guess this could be handle later as FS is currently the most used.
Here my tests result using PJRC tests and this PR.
I've made some charts to compare.
usb_serial_receive_results.xlsx

[makarenya]

What exactly is wrong with HS?

[fpistm]

Ok. Thanks @BennehBoy ,
I think I will merge those PR then release the 1.5.0.
It seems enough stable.

[BennehBoy]

HID bootloader works fine.

DFU with Maple Original bootloader works fine - DISC PIN handling working.

Seems good to me.

Will you consider merging HID/DFU & the magic word handling also?

[ktand]

@fpistm @makarenya

I have tried this pull request and the branch created by Frederic (https://github.com/fpistm/Arduino_Core_STM32/tree/USB_PR_419_424_426) with the Marlin 3D printer firmware (2.0.x). Unfortunately there is a serious issue which causes the firmware to lock up until the USB CDC port is opened from the host computer.

Marlin writes to the serial port during startup and if the port is not open the USBSerial.write() function never returns.

I think the linestate must be montored by the CDC implementation.

This was not a problem with the timer-based CDC.

Regards,
Karl

[makarenya]

Got it, i make it tomorrow

[ktand]

Hi @makarenya

Did you find a solution to the problem with writes to the CDC when the port is not opened on the host side? If you need any help testing just let me know.

[fpistm]

@BennehBoy, @ktand could you test this latest version, please?
Thanks in advance.
@BennehBoy this should solve issue you met with bootloader.
@ktand I don't think this solved your issue anyway USB is init earlier.

I will do a release this week, so hope to solve your issue with Marlin before.

[BennehBoy]

@fpistm - works 👍

[ktand]

@fpistm I will test tonight. Den tis 12 feb. 2019 22:27Ben Anderson <[email protected]> skrev:

…

@fpistm <https://github.com/fpistm> - works 👍 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#419 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQ8NJI7p-x6bPaI0C7cruMqdtqoqtBDjks5vMzGvgaJpZM4aTPhy> .

[fpistm]

I think I will merge this PR.
New PR could be opened to solve issue related to Marlin before the release or this will be available in the next one.