Assumption in QLCFixtureHead::cacheChannels may be invalid

[lightsbybrian]

I'm working on a fork that adds the feature of an Android app making a TCP connection to QLC+ and being able to edit scene parameters (primarily pan & tilt, but I'm sure it'll grow from there). So, I was working on testing the mechanisms involved in enumerating fixtures and heads used in a scene. I decided that my app should know whether a head's pan & tilt are 8- or 16-bit, so I checked to see if the value of fixture->channelNumber(QLCChannel::Pan, QLCChannel::LSB, head) is equal to QLCChannel::invalid().

In order to test the robustness of my new code, I decided to create some very silly fixtures in the QLC Fixture Editor. I created a fixture with two heads, but one head only has 8-bit pan/tilt, while the second head has 16-bit. So there is a LSB pan/tilt channel for head 1, but not head 0.

I discovered that fixture->channelNumber(...) was giving me a valid channel for pan LSB on head 0, even though it didn't have one (only head 1 did). At first I thought this was a bug, but eventually I found the section of code that was doing it, clearly on purpose. The code is in qlcfixturehead.cpp in the cacheChannels(...) method:

Code: Select all

    // if this head doesn't include any Pan/Tilt channel
    // try to retrieve them from the fixture Mode
    if (channelNumber(QLCChannel::Pan, QLCChannel::MSB) == QLCChannel::invalid())
        setMapIndex(QLCChannel::Pan, QLCChannel::MSB, mode->channelNumber(QLCChannel::Pan, QLCChannel::MSB));
    if (channelNumber(QLCChannel::Pan, QLCChannel::LSB) == QLCChannel::invalid())
        setMapIndex(QLCChannel::Pan, QLCChannel::LSB, mode->channelNumber(QLCChannel::Pan, QLCChannel::LSB));
    if (channelNumber(QLCChannel::Tilt, QLCChannel::MSB) == QLCChannel::invalid())
        setMapIndex(QLCChannel::Tilt, QLCChannel::MSB, mode->channelNumber(QLCChannel::Tilt, QLCChannel::MSB));
    if (channelNumber(QLCChannel::Tilt, QLCChannel::LSB) == QLCChannel::invalid())
        setMapIndex(QLCChannel::Tilt, QLCChannel::LSB, mode->channelNumber(QLCChannel::Tilt, QLCChannel::LSB));

I wonder if maybe that's not a good idea? It definitely makes sense if someone created a fixture and defined some heads but forgot to assign pan/tilt channels. But it also means that if there is some weird fixture that, e.g., has some heads that don't move, or has some heads that (like I defined for my test) are 8-bit p/t and some that are 16-bit, then this approach causes a problem. I don't know if any such fixtures exist, but who knows?

Anyway, I suggest one of a few possible changes:
1. do the above code only if ALL the pan/tilt channels are missing, instead of one-by-one independently. If they're ALL missing, then fix them all according to the above code.
2. remove the above code altogether and trust / force people to create correct fixture definitions.
3. option 1, but only head 0, so that different heads' channels don't get mixed together

I am going to go for option 3 in my fork, because it seems to make the most sense to me, but I'm starting this thread to get other opinions (especially massimo's) since I easily could be overlooking some case. Option 2 might break some existing fixture definitions.

PS: I've attached the fixture definition I made which exposes this, because I might not be describing it very well. The mode in the definition is called "Buggy" and has one 8-bit head and one 16-bit head.