Repeated use of attach / detach leaks channels

[Javerre]

The channel allocation mechanism in this library is primitive and buggy. As a result, if you use the automatic channel allocation feature, repeated calls to detach and attach will cause the library to exhaust the supply of channels and it will stop functioning.

I have worked around this by avoiding the automatic allocation and instead passing servo-specific channel references in the call to attach. However, this bug can be easily fixed as follows:

Instead of treating the "channel_next_free" static integer as single channel number, treat it as a bitfield. Then, when allocating a channel in attach(), set the bit associated with that channel to mark it as in use. Finally, clear said bit in the detach() function.

In other words in the implementation of attach() replace this code:

    if(channel == CHANNEL_NOT_ATTACHED) {
        if(channel_next_free == CHANNEL_MAX_NUM) {
            return false;
        }
        _channel = channel_next_free;
        channel_next_free++;
    } else {
        _channel = channel;
    }

with this:

    if(channel == CHANNEL_NOT_ATTACHED) {
        for (int mask = 1; mask != 0; mask <<= 1)  {
            ++channel;
            if((channel_next_free & mask) == 0) {
                break;
            }
        }
    }
    if(channel > CHANNEL_NOT_ATTACHED && channel < CHANNEL_MAX_NUM) {
        _channel = channel;
        channel_next_free |= (1 << channel);
    } else {
        return false;
    }

and in the implementation of detach() replace this code:

    if(_channel == (channel_next_free - 1))
        channel_next_free--;

with this:

    channel_next_free &= ~(1 << _channel);

[JarekParal]

@Javerre Thank you for your post. This problem is known for a long time #2. Unfortunately, I didn't find time to fix it. Could you please create PR with your changes and test it? Just one note: What use case you expect from this condition if(channel > CHANNEL_NOT_ATTACHED && channel < CHANNEL_MAX_NUM)?

[Javerre]

Hi @JarekParal how this works is this:

If the user supplies CHANNEL_NOT_ATTACHED then we enter the first if statement and search the bits of "channel_next_free" until we find a zero bit or run out of bits to try. If all channels are occupied this will terminate with channel = CHANNEL_MAX_NUM. The code will work for any CHANNEL_MAX_NUM up to 31.

    if(channel == CHANNEL_NOT_ATTACHED) {
        for (int mask = 1; mask != 0; mask <<= 1)  {
            ++channel;
            if((channel_next_free & mask) == 0) {
                break;
            }
        }
    }

If the user supplies any other value for the channel then we skip the search and fall straight through.

The next statement marks the channel as in use if we now have a valid channel number (either because we found one in the search above or because one was supplied by the user). Valid channels are in the range 0..CHANNEL_MAX_NUM-1. If channel is not in this range then this is an error - either a bad channel number was supplied by the user or we have used all available channels - so we early out returning false.

    if(channel > CHANNEL_NOT_ATTACHED && channel < CHANNEL_MAX_NUM) {
        _channel = channel;
        channel_next_free |= (1 << channel);
    } else {
        return false;
    }

Incidentally, on reflection I'd actually like to propose a slight improvement where this section is coded like this instead:

    if(channel == CHANNEL_NOT_ATTACHED) {
        channel = 0;
        for (int mask = 1; mask != 0; mask <<= 1)  {
            if((channel_next_free & mask) == 0) {
                break;
            }
            ++channel;
        }
    }
    if(channel >= 0 && channel < CHANNEL_MAX_NUM) {
        _channel = channel;
        channel_next_free |= (1 << channel);
    } else {
        return false;
    }

This has the advantage that it works even if the CHANNEL_NOT_ATTACHED magic number changes to something other than -1, and also works for CHANNEL_MAX_NUM values up to 32. Probably moot, I know, but it's more elegant and about the same cost in cycles and code size.