Allow setting full-on and full-off bits via set_all_on_off()

[mhthies]

Problem

I use the PCA9685 for driving multichannel LED strips with animations. In every animation step, I update all on/off values with set_all_on_off() and afterwards use set_channel_full_on() resp. set_channel_full_off() for all channels that shall be full on/off. This got me issues with visible flickering of the full on/off channels.

Thus, I want to be able to update all channels' on/off counter values and the full-on and full-off bits in a single I2C transaction.

Solution

I extended the allowed range of values in the slice arguments of set_all_on_off(), so it's possible to set the 13th bit (the full-on/full-off bit) for each individual channel. This is a backwards-compatible change.

Alternatives

If you don't like the relaxed range check of the on/off values in the existing set_all_on_off() method, because of its mixed-up value semantics (I understand that setting a channel to full-off is semantically different from setting the off counter value), I can refactor the PR to add another method for this combined functionality. (Any ideas for the name?)

This separate method could also take two additional &[bool; 16] slices for the full-on/full-off values, but I considered this is a bit inefficient and rather unergonomically to use.

Alternatively, I thought about adding public constants FULL_ON_BIT: u16 and FULL_OFF_VALUE: u16 to make the new combined functionality of set_all_on_off() easier to use.

[coveralls]

coverage: 83.15% (-0.7%) from 83.883%
when pulling 63b95c6 on mhthies:feature/set_all_full_on_off
into 9865710 on eldruin:master.

[eldruin]

Thanks for the PR. I understand the need for something like this.
I am not very happy with the value semantics, indeed, but I am not sure there is a better alternative.
What do you think about a method set_all or set_all_channels with these arguments:

(&mut self, on: &[u16; 16], off: &[u16; 16], full_on: &[bool; 16], full_off: &[bool; 16])

Inside the method would just zip all lists together and or the on values with some FULL_ON_MASK and similarly for full_off.
It would be 4 lists to handle in the user code, though.
What do you think?

[mhthies]

This method signature with four slice arguments, which you propose, was exactly my first idea. I started implementing it and then changed my mind, because of the amount of boilerplate code for creating and handling the four arrays. But I see why you prefer it and I'd be okay with implementing and using it. (And I like set_all_channels() as its name.)

As an alternative: What do you think about combining the four arrays like this:

pub struct ChannelOnOff {
    pub on: u16,
    pub off: u16,
    pub full_on: bool,
    pub full_off: bool,
}

impl<...> Pca9685<...> {
    // ...
    
    set_all_channels(&mut self, values: &[ChannelOnOff; 16]) -> ... { ... } 
}

[eldruin]

That looks like a nice alternative.
How ergonomic do you think it would be?

[mhthies]

In my use case, that would be quite nice, because I already have a function that encapsulates the calculation of all channels' on and off counter values, based on all target duty cycles. Currently it returns the two on and off arrays, which are then passed to set_all_on_off(). Creating a single array with all the combined information would be even better for the surrounding code to not worry about the technical details of that data structure. (Passing four arrays from that function to the Pca9685 library api, on the other hand, would be worse.)

[mhthies]

One disadvantage of the struct-based solution: Due to alignment, it will probably have an overhead of 32 padding bytes for the whole array (assuming 4-byte alignment: 16*2byte). This may be an issue, when keeping the data for controlling multiple Pca9685 chips in memory on a microcontroller.

[eldruin]

I defined a type like this: pub struct ChannelOnOffArray ([ChannelOnOffControl; 16]); and asked the compiler what the size and alignment are with cargo +nightly rustc --target=thumbv7em-none-eabihf -- -Zprint-type-sizes:

`ChannelOnOffArray`: 96 bytes, alignment: 2 bytes
    field `.0`: 96 bytes
`ChannelOnOffControl`: 6 bytes, alignment: 2 bytes
    field `.on`: 2 bytes
    field `.off`: 2 bytes
    field `.full_on`: 1 bytes
    field `.full_off`: 1 bytes

Without mention of any additional padding, so I think we are ok.
Am I missing anything?

[mhthies]

Ah, okay, forget what I said. We only have one or two bytes long fields in this struct, so there is no need for the compiler to 4-byte-align the struct (independent from the CPU architecture).

I already fully implemented this new approach in PR #18.

[eldruin]

Closing in favor of #18