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

slouken commented

2023-05-26 11:24:40 -05:00

(Migrated from github.com)

No description provided.

mcuee commented

2023-05-26 20:34:26 -05:00

(Migrated from github.com)

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

slouken commented

2023-05-26 22:37:19 -05:00

(Migrated from github.com)

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 commented

2023-06-01 04:37:04 -05:00

(Migrated from github.com)

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.

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: ```cpp 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 commented

2023-06-01 09:20:03 -05:00

(Migrated from github.com)

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.

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 commented

2023-06-01 09:54:52 -05:00

(Migrated from github.com)

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).

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).

👍 2

Youw commented

2024-04-06 05:26:00 -05:00

(Migrated from github.com)

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

This pull request is broken due to missing fork information.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin 'slouken/0001-hidapi-linux-retry-hid_send_feature_report-if-the-io.patch:slouken/0001-hidapi-linux-retry-hid_send_feature_report-if-the-io.patch'

git switch 'slouken/0001-hidapi-linux-retry-hid_send_feature_report-if-the-io.patch'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'master'

git merge --no-ff 'slouken/0001-hidapi-linux-retry-hid_send_feature_report-if-the-io.patch'

git switch 'slouken/0001-hidapi-linux-retry-hid_send_feature_report-if-the-io.patch'

git rebase 'master'

git switch 'master'

git merge --ff-only 'slouken/0001-hidapi-linux-retry-hid_send_feature_report-if-the-io.patch'

git switch 'slouken/0001-hidapi-linux-retry-hid_send_feature_report-if-the-io.patch'

git rebase 'master'

git switch 'master'

git merge --no-ff 'slouken/0001-hidapi-linux-retry-hid_send_feature_report-if-the-io.patch'

git switch 'master'

git merge --squash 'slouken/0001-hidapi-linux-retry-hid_send_feature_report-if-the-io.patch'

git switch 'master'

git merge --ff-only 'slouken/0001-hidapi-linux-retry-hid_send_feature_report-if-the-io.patch'

git switch 'master'

git merge 'slouken/0001-hidapi-linux-retry-hid_send_feature_report-if-the-io.patch'

git push origin 'master'

Rows
Columns

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

Checkout

Merge