CorsairOfficial / cue-sdk

Corsair iCUE SDK
https://corsairofficial.github.io/cue-sdk/
220 stars 24 forks source link

Wrong LED positions for M800(C) mouse pad #2

Closed xoblite closed 4 years ago

xoblite commented 4 years ago

Hi Corsair & Co,

I have noticed the following issues controlling the M800(C) mouse pad (latest firmware v1.02.50) using the latest CUE SDK v3.0.301 + the latest iCUE version 3.25.60, running on Windows 10:

Thanks for making the SDK available; keep up the good work!

BR//KHH (@xoblite)

PS. "CUE SDK" and "iCUE SDK" are used interchangeably across the Corsair forums and here on GitHub; which one is the correct one to refer to if/when so applicable? (i.e. respecting any trademarks etc)

intrueder commented 4 years ago

Hi @xoblite, really appreciate your detailed feedback!

As far as I understand, the LEDs in question were obtained as a result of CorsairGetLedPositionsByDeviceIndex function call. iCUE does not guarantee the order of LEDs in pLedPosition array, but each CorsairLedPosition struct in the array has the identifier of led:

struct CorsairLedPosition
{
    CorsairLedId ledId;
    double top;
    double left;
    double height;
    double width;
};

The LEDs have exactly the same order of ids as you described in the second bullet. Top left is CLMM_Zone1 and top right is CLMM_Zone15. I assume the SDK client app, depending on its needs, can order LEDs by either ledId or top/left fields

xoblite commented 4 years ago

Hi @intrueder ,

thank you for your reply!

Yes, I was using CorsairGetLedPositionsByDeviceIndex for this particular use case, see the following truncated code example:

int numberOfDevices = CorsairGetDeviceCount();
if (numberOfDevices == 0) return;

for (int i = 0; i < numberOfDevices; i++)
{
    CorsairLedPositions* clp = CorsairGetLedPositionsByDeviceIndex(i);
    CorsairLedColor* ledColors = new CorsairLedColor;

    for (int n = 1; n <= clp->numberOfLed; n++)
    {
        SetCorsairLedColorsFromRGB(ledColors, HWLEDColors[n]);
        ledColors->ledId = clp->pLedPosition[n-1].ledId;
        CorsairSetLedsColorsBufferByDeviceIndex(i, 1, ledColors);
    }

    delete ledColors;
}

CorsairSetLedsColorsFlushBuffer();

Basically, in this particular case, HWLEDColors[] is an array that I fill with sequential-and-then-sometimes-mirrored patterns of up to 8 colours, e.g. 1-2-3-4-5-6-7-8-7-6-5-4-3-2 and then repeated as needed. These 8 colours are assigned freely by upper layers to create various effects coordinated between on-screen elements (which in turn are sometimes subject to various other input) and now then also propagated to Corsair iCUE HW LEDs via the SDK. It already looks pretty good, but the non-sequential nature of the LED positions is not ideal and sometimes diminishes or spoils the effect, that is - if I understand you correctly? - I don't add another layer of device-specific parsing to my code? If so, I still don't see the rationale for this behaviour; there must be lots of use cases where there is no need to consider exact physical locations but where the sequence of the LEDs is more important, no? This said, if I'm missing something obvious, please enlighten me! :)

BR//KHH (@xoblite)

xoblite commented 4 years ago

PS. (+see above) Hmm, can I assume that CorsairLedId enums for a given device type will always be consecutive? If so, I guess it'd be possible to subtract every ID returned by the SDK - assuming you've previously identified the device type at least (e.g. CDT_Mousemat) - by the first member (e.g. CLMM_1 in the case of a mouse pad), to allow the kind of simple "zero based offset" ordering I'm after (nb. I'd still need to add said limited device type knowledge for each defined device type of course, but that list isn't that long at least; only 9 at the moment). Would this then satisfy the essence of my original second bullet above maybe? Any thoughts?

intrueder commented 4 years ago

Hey @xoblite, that subtraction logic looks complex to me. What I was talking about is sorting (it is less performant than subtraction though)

I'd suggest to change your code example to something like this:

for (int i = 0; i < numberOfDevices; i++)
{
    CorsairLedPositions* clp = CorsairGetLedPositionsByDeviceIndex(i);
    // the next line requires #include <algorithm>
    std::sort(clp->pLedPosition, clp->pLedPosition + clp->numberOfLed,
        [](const CorsairLedPosition& a, const CorsairLedPosition& b) {
            return a.ledId < b.ledId;
        });
    for (int n = 0; n < clp->numberOfLed; n++) // I prefer 0-based loops
    {
        CorsairLedColor ledColors = { clp->pLedPosition[n].ledId };
        SetCorsairLedColorsFromRGB(&ledColors, HWLEDColors[n]);
        CorsairSetLedsColorsBufferByDeviceIndex(i, 1, &ledColors);
    }
}

CorsairSetLedsColorsFlushBuffer();

Would that work for you?

xoblite commented 4 years ago

Hi @intrueder ,

ah, it didn't occur to me that I could sort() the clp's, thanks for the heads-up! I will try that approach instead; the less device specific glue the better in this case. Will keep you posted.

Oh, by the way, do you also happen to have any recommendations with regards to what actions to perform when the system is e.g. resuming from sleep/hibernation? Should I perform a new CorsairPerformProtocolHandshake then for example? Anything else one should be aware of?

Thanks again!

BR//KHH (@xoblite)

intrueder commented 4 years ago

Please open another issue for this different question.