Bugfix: aux channel modes should use low brightness for level 1 (not off)

[aslotnick]

In the current implementation of using aux LEDs as channels, level 1 (the lowest level) results in the aux being set to "off" while higher levels result in the aux set to "high".

With this change, level 1 will result in aux "low" while higher levels continue to result in aux "high".

This makes the chan-aux and chan-rgbaux feature slightly more useful. Fixes #65 , also fixes #62.

Testing

• ts10 v1 (single aux): low aux works for the aux channel at level 1
• ts10 v2 (rgb aux): low aux works for all channels (R, R+G, G, G+B, B, R+B, R+G+B) at level 1

[aslotnick]

Rebased and updated the description to refer to "level 1" as the lowest level, matching the manual.

[SammysHP]

In the current implementation of using aux LEDs as channels, level 1 (the lowest level) results in the aux being set to "off"

Level 0 is off, everything else was converted to 1 in the expressions affected by this pull request. This is the intended behavior.

Your change to the single color aux results in either 1 (low) or 3 (undefined).

How are aux LEDs turned off after this change? Doesn't it break number readout via aux LEDs?

[aslotnick]

Thanks for checking this out @SammysHP.

This is the intended behavior.

To clarify the discussion of the problem and the proposed solution, I have opened a corresponding issue: #65. I do not think that the behavior described there is intended (using aux as the active channel and selecting the lowest level resulting in the aux turning off). But my solution might not be the right one.

I am still new to this codebase, but I made this change based on trial-and-error on my ts10 and ts10-rgb. So I will try to understand what is actually happening now.

Level 0 is off, everything else was converted to 1 in the expressions affected by this pull request. This is the intended behavior. How are aux LEDs turned off after this change?

It seems that there are two different definitions of "level 0" in use. In the Anduril UI code, there appears to be an offset of 1. So in ramping.c, when the level is "level 0", that corresponds to "off" and is treated as a special case. Since that uses set_level_zero, I don't think it is affected by my change, and the aux LEDs turn off as expected in my testing.

    if (0 == level) {
        set_level_zero();

Then when a level is 1 or higher, there is an offset of -1 applied before calling the corresponding set_level_*.

    } else {
        // call the relevant hardware-specific set_level_*()
        SetLevelFuncPtr set_level_func = channels[channel_mode].set_level;
        set_level_func(level - 1);
    }

set_level_func is what gets translated to either set_level_aux (for ts10) or set_level_auxred etc. (for ts10-rgb).

So before my change, when the user selects Anduril's level 1, that results in a call of the set_level_func(0), which results in the aux being turned off (either indicator_led(0) or rgb_led_set(0b000000)). After my change, a call to set_level_func(0) results in the aux being turned on at low level.

Your change to the single color aux results in either 1 (low) or 3 (undefined).

Yes, set_level_aux(0) results in a call of indicator_led(1) and set_level_aux(1) (or any number greater than 1) results in a call of indicator_led(3). This is a bit confusing, however 3 is not undefined! indicator_led only defines cases for 0, 1, and default. So 3 resulted in a setting to "high", which is the behavior I wanted.

But now that I'm taking a second look, there is a better solution which is less confusing: I can replace this with !(!(level)) +1. That will result in outputs of 1 (for input 0) and 2 (for input >=1) which seems slightly less confusing. I will make this update.

Doesn't it break number readout via aux LEDs?

I don't think so, but I might be missing something.

[SammysHP]

Then when a level is 1 or higher, there is an offset of -1 applied before calling the corresponding setlevel*.

Oh right, I remember there was some refactoring going on recently. Sounds like this might have introduced the bug.

[ToyKeeper]

Thanks for spotting this and fixing it. It looks like I missed that part of the code during a refactor at some point, and it should have been off/low/high all along instead of just off/high.

[aslotnick]

thanks @ToyKeeper for accepting!