Skip to content
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.

Sign up for GitHub

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

Jump to bottom

CDC and HID don't seem to coexist properly (STM32F072) #12

Open
bentwire opened this issue Apr 30, 2019 · 34 comments
Open

CDC and HID don't seem to coexist properly (STM32F072) #12

bentwire opened this issue Apr 30, 2019 · 34 comments
Labels
bug Something isn't working

Comments

@bentwire
Copy link

I got all my CDC issues fixed, so now I am moving on to the HID portion of my code. I have the hid device defined (it is a simple 2 axis joystick, the report desc is 57 bytes), but when I mount it and the CDC interface Linux can't properly detect the HID interface. If I comment out the mounting of the CDC interface the HID interface gets detected fine (I've not tried to send data yet).

Any suggestions on what I may have done wrong? I can post a gist of the HID code if you need, it is fairly short.

Unfortunately on this hardware I don't have access to the JTAG/SWD interface so I can't attach a debugger. :(
@benedekkupper
Copy link
Member

Does the CDC interface (function really, but let's not get too technical) get detected though? How do you set the endpoint addresses?

@bentwire
Copy link
Author

Yep, the CDC interface gets detected and functions. The string descriptors don't work properly (lsusb says error) but it does get detected and I can read from it.

I set them as follows:

void UsbDevice_Init(void)
{
    /* Initialize the device */
    USBD_Init(UsbDevice, dev_cfg);

    /* All fields of Config have to be properly set up */
    console_if->Config.InEpNum  = 0x81;
    console_if->Config.OutEpNum = 0x01;
    console_if->Config.NotEpNum = 0x82;

    joy_if->Config.InEpNum      = 0x83;

    /* Mount the interfaces to the device */
    USBD_CDC_MountInterface(console_if, UsbDevice);
    //USBD_HID_MountInterface(joy_if, UsbDevice);

    /* The device connection can be made */
    USBD_Connect(UsbDevice);
}

@benedekkupper
Copy link
Member

This part look ok. Does the HID interface work on its own?

@bentwire
Copy link
Author

Yes if I comment out the CDC mount the HID interface gets detected fine. I have not tried to send data yet however.

@benedekkupper
Copy link
Member

This works just fine for me, e.g. with the DebugDongle, so it's more likely that something specific to your implementation is the problem.

@bentwire
Copy link
Author

It sends data fine as well.

@bentwire
Copy link
Author

Hmm I wonder what it could be. I do have a USB Beagle 12 that I could do a bus dump with while it enumerates. Would that be of help?

Yeah I used DebugDongle as an example actually.

@bentwire
Copy link
Author

Ok this is weird... I reversed the mount order, and now the both work:

void UsbDevice_Init(void)
{
    /* Initialize the device */
    USBD_Init(UsbDevice, dev_cfg);

    /* All fields of Config have to be properly set up */
    console_if->Config.InEpNum  = 0x81;
    console_if->Config.OutEpNum = 0x01;
    console_if->Config.NotEpNum = 0x82;

    joy_if->Config.InEpNum      = 0x83;

    /* Mount the interfaces to the device */
    USBD_HID_MountInterface(joy_if, UsbDevice);
    USBD_CDC_MountInterface(console_if, UsbDevice);
    
    /* The device connection can be made */
    USBD_Connect(UsbDevice);
}

The strings still show (error) in lsusb output, but both interfaces work...

@benedekkupper
Copy link
Member

Do you enable this flag: USBD_HID_REPORT_STRINGS ? Do you have anything specific about your string handling?

@bentwire
Copy link
Author

I do not. Here is my config:

#ifndef __USBD_CONFIG_H_
#define __USBD_CONFIG_H_

/** @addtogroup USBD_Exported_Macros
 * @{ */

/** @brief Must be set according to the highest number of interfaces for a given USB Device. */
#define USBD_MAX_IF_COUNT           8

/** @brief Must be set higher than:
 * @arg the length of the entire USB device configuration descriptor
 * @arg the longest USB string (as Unicode string descriptor)
 * @arg the length of the longest class-specific control request */
#define USBD_EP0_BUFFER_SIZE        256

/** @brief Set to 1 if peripheral support and application demand
 * for High-Speed operation both exist. */
#define USBD_HS_SUPPORT             0

/** @brief When set to 0, no SerialNumber is readable by the host.
 * Otherwise the SerialNumber will be converted from USBD_SERIAL_BCD_SIZE / 2
 * amount of raw bytes to string BCD format and sent to the host. */
#define USBD_SERIAL_BCD_SIZE        48



/** @brief Set to 1 if notifications are sent by a CDC-ACM interface.
 * In this case notification EP will be allocated and opened if its address is valid. */
#define USBD_CDC_NOTEP_USED         1

/** @brief Set to 1 if SET_CONTROL_LINE_STATE request is used by a CDC-ACM interface. */
#define USBD_CDC_CONTROL_LINE_USED  0

/** @brief Set to 1 if SEND_BREAK request is used by a CDC-ACM interface. */
#define USBD_CDC_BREAK_SUPPORT      0



/** @brief Set to 1 if a DFU interface holds more than one applications as alternate settings. */
//#define USBD_DFU_ALTSETTINGS        0

/** @brief Set to 1 if DFU STMicroelectronics Extension
 *  protocol (v1.1A) shall be used instead of the standard DFU (v1.1). */
//#define USBD_DFU_ST_EXTENSION       0



/** @brief Set to 1 if a HID interface holds more than one applications as alternate settings. */
#define USBD_HID_ALTSETTINGS        0

/** @brief Set to 1 if a HID interface uses an OUT endpoint. */
#define USBD_HID_OUT_SUPPORT        0

/** @brief Set to 1 if a HID interface defines strings in its report descriptor. */
#define USBD_HID_REPORT_STRINGS     0

/** @} */

#endif /* __USBD_CONFIG_H_ */

@bentwire
Copy link
Author

Here is all the HID code:

#include <usbd_hid.h>
#include <hid/usage_consumer.h>
#include <hid/usage_desktop.h>
#include <hid/usage_button.h>
#include <accell_tasks.h>
#include <hid.h>

/**
 * HID Report for joystick
 *
 */
__alignment(USBD_DATA_ALIGNMENT)
static const uint8_t JoystickReport[] __align(USBD_DATA_ALIGNMENT) = {
        HID_USAGE_PAGE_DESKTOP,
        HID_USAGE_DT_JOYSTICK,
        HID_COLLECTION_APPLICATION(
                HID_USAGE_PAGE_BUTTON,
                HID_USAGE_MIN_8(1),
                HID_USAGE_MAX_8(8),
                HID_LOGICAL_MIN_8(0),
                HID_LOGICAL_MAX_8(1),
                HID_PHYSICAL_MIN_8(0),
                HID_PHYSICAL_MAX_8(1),
                HID_REPORT_SIZE(1),
                HID_REPORT_COUNT(8),
                HID_INPUT(HID_ITEM_FLAG_VAR),
                HID_USAGE_PAGE_DESKTOP,
                HID_USAGE_DT_X,
                HID_LOGICAL_MIN_16(-2500),
                HID_LOGICAL_MAX_16(5500),
                HID_PHYSICAL_MIN_16(-2500),
                HID_PHYSICAL_MAX_16(5500),
                HID_REPORT_SIZE(16),
                HID_REPORT_COUNT(1),
                HID_INPUT(HID_ITEM_FLAG_VAR),
                HID_USAGE_DT_Y,
                HID_REPORT_SIZE(16),
                HID_REPORT_COUNT(1),
                HID_INPUT(HID_ITEM_FLAG_VAR),
        ),
};

void
sendReport(hid_joydata_t * joyData) {
    USBD_HID_ReportIn(joy_if, joyData, sizeof(hid_joydata_t));
}

static void
getReport(void * itf, USBD_HID_ReportType type, uint8_t id) {
    uint8_t buf[8] = { 0 };
    hid_joydata_t data = { 0 };
    if (type == HID_REPORT_INPUT) {
        sendReport(&data);
    } else {
        USBD_HID_ReportIn(itf, buf, sizeof(buf));
    }
}

static void
setReport(void * itf, USBD_HID_ReportType type, uint8_t * data, uint16_t len) {

}

static void joyInit(void *itf) {

}

static void joyDeinit(void *itf) {

}

static const USBD_HID_ReportConfigType joystickReportConfig = {
        .Desc = JoystickReport,
        .DescLength = sizeof(JoystickReport),
        .Input.MaxSize = sizeof(hid_joydata_t),
        .Input.Interval_ms = 20,
};

static const USBD_HID_AppType joyApp = {
        .Name = "Joystick",
        .Init = joyInit,
        .Deinit = joyDeinit,
        .SetReport = setReport,
        .GetReport = getReport,
        .Report = &joystickReportConfig,
};

USBD_HID_IfHandleType hjoy_if = {
        .App = &joyApp,
        .Base.AltCount = 1,
}, *const joy_if = &hjoy_if;

Nothing obvious sticks out to me.

@bentwire
Copy link
Author

BTW after I get this working I will send a PR for the Joystick additions I did to the HidReportDef project.

@benedekkupper
Copy link
Member

I could only tell more with USB trace information available, otherwise I have no ideas.

@bentwire
Copy link
Author

composite-hand.zip

Hopefully the capture will help. It looks like the device replies with an invalid PID sequence during a setup transaction. You will see the rows marked "I" in the error column in the .csv.

@benedekkupper
Copy link
Member

The only failure I see here is that the host fails to set the CDC line coding, and afterwards also fails to read the interface descriptor of the console_if (which it succeeded to read before). These are all functioning operations on my side, so I don't know what's exactly wrong with your setup.

@bentwire
Copy link
Author

bentwire commented May 1, 2019

Huh, weird. Well it is working for now with just that issue. I think I will just ignore it for now. Should I close this or keep it open just in case I find something else?

@bentwire
Copy link
Author

bentwire commented May 1, 2019

I have a patch to fix reading in console_if.c It wasn't "priming the pump" so to say.

diff --git a/Core/Src/console_if.c b/Core/Src/console_if.c
index caa0766..dd97ad5 100644
--- a/Core/Src/console_if.c
+++ b/Core/Src/console_if.c
@@ -107,6 +107,7 @@ static void console_if_open(void* itf, USBD_CDC_LineCodingType * lc)
 #endif
 #if (STDIN_BUFFER_SIZE > 0)
     console_if_OUT.head = console_if_OUT.tail = 0;
+    USBD_CDC_Receive(console_if, &console_if_OUT.buffer[0], console_out_size);
 #endif
 }

@benedekkupper
Copy link
Member

It would make more sense to call console_if_recv() directly.

@bentwire
Copy link
Author

bentwire commented May 1, 2019

Ahh yep! Fixed it to do that instead.

benedekkupper added a commit that referenced this issue May 3, 2019
@benedekkupper
console_if: start receiving when opened (#12)
d70cac5
@bentwire
Copy link
Author

Hrm I closed this but it is still an issue. Quite annoying I can't seem to get anywhere with it. :(

CDC by itself sort of works, and the descriptors seem to come back OK depending on timing (If I do a lsusb too soon after enumeration getting the string descriptors will fail and the device thinks it is both self powered and bus powered).

HID by itself works very well and has no issues.

HID+CDC sort of works, but the descriptors don't all come back (for instance with only CDC or only HID I get the BOS descriptor, but with both I don't and none of the string descriptors work.) and for some reason the device reports itself as both Bus Powered AND Self Powered... Which makes no sense...

CDC+HID shows up as a CDC/HID composite device but the HID driver can't probe it, and the descriptors are again wonky, and no working string descriptors.

I've been trying to use a debugger to figure it out but the timing is very sensitive, so my sessions don't catch much if I want the device to actually enumerate...

Is there some sort of link time ordering I need to adhere to, to get the descriptors to work right?

Do you have any other suggestions I can try to get things working?

I can probably send you a zip file of the code if you want to take a look... Maybe it is some weird issue with using the STM32CubeMX interface with your library?

Do you have a NUCLEOF072RB? The code I have will run on that board.

@bentwire bentwire reopened this May 13, 2019
@benedekkupper
Copy link
Member

Did you try to run only the USB stack, disabling the rest of the system, to see if it's an interupt starvation issue? Using the Cube and especially HAL is not the best idea for actually functioning products.

I don't have an F0 nucleo, but I have the next best thing, an F072 Discovery, which has the very same chip onboard, and has USB connection, so I can try your code if you send it to me.

@bentwire
Copy link
Author

Yeah the HAL kinda sucks. I'm slowly porting away from it. I may just move completely to Rust-lang at the end of all this, not sure.

I did try with my timer ISR disabled, and got the same results. I was thinking it could be interrupt starvation as well.

Where is the best place to send the zip file?

@benedekkupper
Copy link
Member

Maybe just push it to a private github repo and add me as collaborator.

@bentwire
Copy link
Author

Ok sent you an invite.

@bentwire
Copy link
Author

bentwire commented May 16, 2019 via email

@benedekkupper
Copy link
Member

It might have been something on my side as well, on Windows I'm struggling with git quite a lot as well. Anyway thanks for the PR, it's merged now.

@bentwire
Copy link
Author

bentwire commented May 16, 2019 via email

@benedekkupper
Copy link
Member

It does. There are some strange things being done here, why do you connect the USB and then disconnect-reconnect it after 1 second? Also why aren't the GPIOs initialized in the MSP callout? The attached zip contains patch files for these, please retry with this. I have no issues when using this and disabling other processing in interrupts.

These would still not fix the underlying design issue that the ADC and I2C blocking operations are performed in interrupt context, while there is no reason they couldn't be done in thread mode (in main()). You could take a look at the delay implementation of the STM32_XPD to see how system time elapsing can be detected via polling (hint: COUNTFLAG bit in SysTick).

IC-review.zip

@bentwire
Copy link
Author

bentwire commented May 16, 2019 via email

@bentwire
Copy link
Author

bentwire commented May 16, 2019 via email

@bentwire
Copy link
Author

bentwire commented May 16, 2019 via email

@benedekkupper
Copy link
Member

I use TrueStudio 9.0.0, and my Windows 10 machine as host.

@benedekkupper
Copy link
Member

Another thing that you should try (and even change permanently), is that your application (practically your timer) is only started by the Init callback of the HID application, and it's stopped at Deinit.

@bentwire
Copy link
Author

bentwire commented May 20, 2019 via email

@benedekkupper benedekkupper added the bug Something isn't working label Nov 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@bentwire @benedekkupper