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.

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

hidapi/linux: retry hid_send_feature_report() if the ioctl() fails with EPIPE (e.g. the device stalled) #579

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

slouken
Copy link
Contributor

@slouken slouken commented May 26, 2023

No description provided.

…th EPIPE (e.g. the device stalled)

Signed-off-by: Sam Lantinga <[email protected]>
@mcuee mcuee added the hidraw Related to Linux/hidraw backend label May 27, 2023
@mcuee
Copy link
Member

mcuee commented May 27, 2023

Just wondering what is the device which has this issue. Thanks. Retry count of 50 seems to be quite high.

@slouken
Copy link
Contributor Author

slouken commented May 27, 2023

I don't remember offhand, this fix is several years old, but it's 50 retries without any delay, so I believe this was a very fast loop.

@Youw
Copy link
Member

Youw commented Jun 1, 2023

50 is definitely too much (that is a lot of flood on the driver/bus in some cases when EPIPE is even expected.
I cannot imagine a case, where the ioctl fails for 47 times and then suddenly succeeds for the 48-th time.
By my experience - if it fails for 2-3 times in a row - it will not recover on its own.

And this workaround is too specific - why only send_feature_report? What about input/output reports, etc.?

And if we're to handle the return codes of ioctl - we better handle not only EPIPE, but at least EINTR (often a debugger attachement) as well.

In one of my projects I use:

template <typename ...ARGS>
inline int xioctl(int fd, unsigned long int request, ARGS&&... args) {
    int result = 0;

    int fails = 5;

    do {
        result = ::ioctl(fd, request, std::forward<ARGS>(args)...);

        if (result < 0) {
            const auto err = errno;
            
            if (err == EINTR // interruptions, like debugger attachments, etc.
                    || err == EPIPE) {
                if (fails-- > 0) {
                    continue;
                }
            }
        }

        break;
    }
    while(true);

    return result;
}
}

I suggest we adapt it (make a C macro instead of C++ template) for our case, and use in all report related functions.

And 5 attempts should be enough, unless you have specific measurements where 50 is justified.

@slouken
Copy link
Contributor Author

slouken commented Jun 1, 2023

5 was definitely not enough. I ran into this on the Steam Link hardware, which is fairly slow, and I think the actual number in that case was the low 20s and I doubled it for safety. My goal was to provide the lowest latency possible while still handling EPIPE conditions. This happens only rarely, so my assumption was that leaving the number high was fine.

In any case, I just wanted to share in case this was helpful. If you go with a different approach, no worries.

@Youw
Copy link
Member

Youw commented Jun 1, 2023

In that case maybe make the number of retries configurable. Make it small (5?) by default and give some means to configure it for projects like SDL.

This happens only rarely

The thing is that I encounter EPIPE quite often and consistently with some devices in some scenarios and in all of those - it is not recoverable (due to HW/FW bug/inconsistency, driver error, etc.). And too many retries would just flood the logs with no real improvement(s).

@Youw Youw marked this pull request as draft April 6, 2024 10:25
@Youw
Copy link
Member

Youw commented Apr 6, 2024

Making a draft for now, until we have a better implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hidraw Related to Linux/hidraw backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants