Prevent deadlock with BULK transfert and reduce log noise

[acolombier]

• Set a timeout (currently 5sec) to prevent deadlock (occurred with the S4Mk3, as the device seems to have froze, leading to the Mixxx controller thread to starve.)
• Make success log message only displayed when using --controller-debug

[JoergAtGithub]

In #13622 we have a open discussion because of the interface number for bulk already

[acolombier]

Ah yeah, completely forgot about that one. I guess we might still want to get that PR for timeout and error detection fix?

[JoergAtGithub]

I think this one is not in conflict with #13622

[acolombier]

Realised that the signature change makes more sense to move into #13737 as it used in there to stop upon comm failure.

Repurposing the PR for only timeout and spamming log messages

[acolombier]

@Swiftb0y the code suggestion doesn't seem to work. Any suggestion to fix it?

[daschuer]

I think it is:

    using namespace std::literals::chrono_literals; 
    constexpr std::chrono::seconds kBulkSendTimeout = 5s;

However the struggle here, confirms IMHO that using 5s is a bit over engineered here when not using this for chronio functions. So I would release the compiler from some work and use use the libusb type without a cast:

constexpr unsigned int kBulkSendTimeoutMs = 5000;

The Ms prefix documents the value unit.
Both is fine for me.

[Swiftb0y]

I disagree honestly. The added compile time is negligible. This is about avoiding primitive obsession.

[acolombier]

The resource you linked make sense, but in our case, we don't own the API and simply must comply to libusb asking for a tasteless uint.
To be honest, I'm leaning towards @daschuer's suggestion as I find it more readable - std::milliseconds(kBulkSendTimeout).count() feels a bit cryptic to me (had to go to the std lib to see what count was doing). Appreciate you are trying to roll out std on time within the Mixxx API tho so can't make up my mind.
Would a @mixxxdj/developers help us with the tie here?

[JoergAtGithub]

In general I'm in favor for these literal types, and we should all get common to this std C++ syntax, as libraries like Qt convert their timing types towards this standard.
In this case, the literal type give us no type safety, as it's converted to the raw integer type before use.
Regarding readability, where a const value is only used once in the whole code - inline with the unit as comment is IMHO the most readable style:

c_function(5000, // Send timeout in milliseconds
           otherArgument); 

[acolombier]

Thanks @JoergAtGithub , that does sound like a great middle ground. @daschuer @Swiftb0y happy with that option?

[Swiftb0y]

In this case, the literal type give us no type safety, as it's converted to the raw integer type before use.

Yeah thats unfortunate, but the conversion is done at the API boundary (so its like calling .get() on a unique_ptr when passing to a C API). Its also easier (in theory/general) to upgrade to the newer API if the conversion is done here, but since this is a C API, that will never happen. I still prefer my original solution (an unnamed constant is not much better), but I won't insist on it. Just my two cents for what is best in the general case. This is only a one-off so as you rightfully pointed out, it doesn't make much difference. I'm fine with anything in this particular case.

[daschuer]

As I already mentioned, all suggestions are fine for me as well.

I can confirm the original request of @Swiftb0y to replace a comment by code.

Comment difficult code, but avoid comments by making the code itself expressive when possible.

[Swiftb0y]

whoops, yeah. Its std::chrono::unit.

#include <chrono>

using namespace std::literals; 
constexpr std::chrono::seconds kBulkSendTimeout = 5s;

int usage() {
    return std::chrono::milliseconds(kBulkSendTimeout).count();
}

[Swiftb0y]

not blocking this any further. please fix pre-commit and clang-tidy and then its LGTM.

[Swiftb0y]

🤷

[daschuer]

Thank you.LGTM