arpruss / USBComposite_stm32f1

USB Composite library for STM32F1 (HID, Serial, MIDI and XBox360 controller)
Other
381 stars 76 forks source link

Issue with RAWHID and reports doubling #93

Closed Doridian closed 3 years ago

Doridian commented 3 years ago

It seems that incoming reports to my STM32 (STM32F1RET6) are doubling up in a weird way.

Lines with // are comments and not actually sent over USBSerial

// Program sends report ID 0, first byte 0, STM32 prints:
REPORT 0 0 0 0 0 0 0 0 
REPORT 0 0 0 0 0 0 0 0 

// Program sends report ID 0, first byte 1, STM32 prints:
REPORT 0 0 0 0 0 0 0 0 
REPORT 1 0 0 0 0 0 0 0 

// Program sends report ID 0, first byte 2, STM32 prints:
REPORT 1 0 0 0 0 0 0 0 
REPORT 2 0 0 0 0 0 0 0 

As you can see, whenever a new report comes in, it seems it reads the previous report...again somehow?

Output of program ([>] = sent this, [<] = received this)

[>] 00-00-00-00-00-00-00-00-00
[<] 00-00-00-00-00-00-00-00-00
[<] 00-00-00-00-00-00-00-00-00

[>] 00-01-00-00-00-00-00-00-00
[<] 00-00-00-00-00-00-00-00-00
[<] 00-01-00-00-00-00-00-00-00

[>] 00-02-00-00-00-00-00-00-00
[<] 00-01-00-00-00-00-00-00-00
[<] 00-02-00-00-00-00-00-00-00

As expected, the report ID is not part of the data, but why does it seem to repeat the previous report whenever a new one comes in? I have tested my program sending RAW HID reports with an Atmega32U4 and it only gets these once (it is also a very simple program only calling write with an incrementing first byte whenever I press enter)

The sketch I used is a basic echo-back program. I have also tried the HIDRaw example from this repol to see if that is the cause and it still got the reports doubled reading them ( https://github.com/arpruss/USBComposite_stm32f1/blob/master/examples/rawhid/rawhid.ino ).

Below is a gist of my sketch. Am I (and the example I found) using the library wrong maybe? https://gist.github.com/Doridian/d0cef69ecff14b3f3401a30297636e32

In case this is relevant, I am using Windows 10 and tried both node-hid in NodeJS and C#'s HID support, both with the exact same results. I have also tried both flashing the raw FW using an ST-Link (clone) as well as using the stm32duino bootloader. Same results. My board is one of these: https://stm32-base.org/boards/STM32F103RET6-Generic-Board

arpruss commented 3 years ago

Do the same issues come up with any other HID profiles?

Doridian commented 3 years ago

Do the same issues come up with any other HID profiles?

Well, the mouse profile seems to work. However I am not sure how reliant that is on receiving reports once (and only once). I can really only test incoming reports doubling up with the HIDRaw profile. Since sending data from MCU to the computer does not double up, I don't know which profile would be best to test receiving reports (rather than sending, which works perfectly)

All the profiles do work, even HIDRaw, just HIDRaw doubles received reports on the MCU side in a weird way.

//EDIT: Also worthy of note: I am testing on Windows, so if it is recognized as a Mouse or Keyboard, I can't send raw reports to it myself, only the OS does.

arpruss commented 3 years ago

Two issues. First, you call getOutput() twice. Get rid of the second call. That won't fix your problem, but it's a good idea.

Then, second, pass true to the optional poll parameter of getOutput(). Otherwise, each time you call getOutput(), it will return the last data in the buffer, whether or not new data has been sent. Only with the poll parameter will it check if the data is fresh.

The default poll=false behavior makes some sense for feature buffers, but perhaps it was a poor design decision on my part.

Doridian commented 3 years ago

Two issues. First, you call getOutput() twice. Get rid of the second call. That won't fix your problem, but it's a good idea.

Then, second, pass true to the optional poll parameter of getOutput(). Otherwise, each time you call getOutput(), it will return the last data in the buffer, whether or not new data has been sent. Only with the poll parameter will it check if the data is fresh.

The default poll=false behavior makes some sense for feature buffers, but perhaps it was a poor design decision on my part.

Passing in true did not change anything, sadly. (And yes, I removed the second call) However, looking at your code: https://github.com/arpruss/USBComposite_stm32f1/blob/master/USBHID.h#L432 it seems poll=1 is the default already.

arpruss commented 3 years ago

Hmm. Try this. Send only one report ever from the PC. Then see how many times it is received.

Doridian commented 3 years ago

Hmm. Try this. Send only one report ever from the PC. Then see how many times it is received.

I did this as simply as possible. I changed my loop to the following:

void loop() {
  if (usbHIDRaw.getOutput(buf)) {
    digitalWrite(LED_PIN, LOW);
    delay(100);
    digitalWrite(LED_PIN, HIGH);
    delay(100);
  }
}

The LED blinks twice even when I just send a singular report from my program.

arpruss commented 3 years ago

That definitely looks like a bug in my code.

Can you try another experiment? In your original code (with the removal of the duplicate getOutput()) replace:

    usbSerial.print(buf[i]);
    usbSerial.print(' ');
  }

with

    usbSerial.print(buf[i]);
    buf[i]=0xFE;
    usbSerial.print(' ');
  }

Then check for me if the duplicate data printed is a string of 0xFE's or if it's the "correct" data but duplicated.

Doridian commented 3 years ago

That definitely looks like a bug in my code.

Can you try another experiment? In your original code (with the removal of the duplicate getOutput()) replace:

    usbSerial.print(buf[i]);
    usbSerial.print(' ');
  }

with

    usbSerial.print(buf[i]);
    buf[i]=0xFE;
    usbSerial.print(' ');
  }

Then check for me if the duplicate data printed is a string of 0xFE's or if it's the "correct" data but duplicated.

REPORT 0 0 0 0 0 0 0 0 
REPORT 0 0 0 0 0 0 0 0 

REPORT 0 0 0 0 0 0 0 0 
REPORT 1 0 0 0 0 0 0 0 

REPORT 1 0 0 0 0 0 0 0 
REPORT 2 0 0 0 0 0 0 0 

It does indeed re-fill the buffer with the "old" report.

//EDIT: Just to ensure this is nothing wrong with that chip, I have also tried a STM32F103C8T6 board and it has the exact same behaviour.

arpruss commented 3 years ago

I just caught a bug that may have been the one. See if it works now.

BTW, I didn't know the library worked with anything other than the C8/C6/CB. Good to know if I need some extra memory.

Doridian commented 3 years ago

I just caught a bug that may have been the one. See if it works now.

BTW, I didn't know the library worked with anything other than the C8/C6/CB. Good to know if I need some extra memory.

Indeed, your latest commit has fixed the issue. I now get all reports exactly once! :)