pio_claim_free_sm_and_add_program_for_gpio_range does not clean up claimed state machines

[qqqlab]

The RP2350B requires the use of gpio_base to shift the pins for a PIO. The pio_claim_free_sm_and_add_program_for_gpio_range function handles this incorrectly, and forgets to unclaim unused state machines. This PR fixes that.

Steps to reproduce

Execute this program on a RP2350B

#if !defined(PICO_RP2350) || PICO_RP2350A
    #error "use RP2350B"
#endif

static const uint16_t ws2812_program_instructions[] = {
    //     .wrap_target
    0x6221, //  0: out    x, 1            side 0 [2]
    0x1123, //  1: jmp    !x, 3           side 1 [1]
    0x1400, //  2: jmp    0               side 1 [4]
    0xa442, //  3: nop                    side 0 [4]
            //     .wrap
};

static const struct pio_program ws2812_program = {
    .instructions = ws2812_program_instructions,
    .length = 4,
    .origin = -1,
};

void setup() {
  int pin;
  PIO pio_claim;
  uint sm_claim;
  uint pio_program_offset;
  bool success;

  Serial.begin();
  delay(5000);

  //start with all SM unclaimed and base=0
  for(uint p = 0; p < NUM_PIOS; p++) {
    PIO pio = PIO_INSTANCE(p);
    for(uint sm = 0; sm < NUM_PIO_STATE_MACHINES; sm++) {
        if(pio_sm_is_claimed(pio, sm)) {
            pio_sm_unclaim(pio, sm);
            pio_set_gpio_base(pio, 0);
        }
    }
  }

  //claim SM for pin 0 (base=0)
  pin = 0;
  success = pio_claim_free_sm_and_add_program_for_gpio_range(&ws2812_program, &pio_claim, &sm_claim, &pio_program_offset, pin, 1, true);
  if (!success) {
    Serial.println("claim1 failed");
    return; 
  }
  Serial.printf("\nclaim1: claimed pio%d-sm%d\n", PIO_NUM(pio_claim), sm_claim);
  for(uint p = 0; p < NUM_PIOS; p++) {
    PIO pio = PIO_INSTANCE(p);
    for(uint sm = 0; sm < NUM_PIO_STATE_MACHINES; sm++) {
        int base = pio_get_gpio_base(pio);
        bool claimed = pio_sm_is_claimed(pio, sm);
        Serial.printf("  pio%d-sm%d ", p, sm);
        Serial.printf("%-10s ", (claimed ? "claimed" : "free"));
        Serial.printf("base=%2d ", base);
        Serial.println();
    }
  }

  //claim SM for pin 47 (base=16)
  pin = 47;
  success = pio_claim_free_sm_and_add_program_for_gpio_range(&ws2812_program, &pio_claim, &sm_claim, &pio_program_offset, pin, 1, true);
  if (!success) {
    Serial.println("claim2 failed");
    return; 
  }
  Serial.printf("\nclaim2: claimed pio%d-sm%d\n", PIO_NUM(pio_claim), sm_claim);
  for(uint p = 0; p < NUM_PIOS; p++) {
    PIO pio = PIO_INSTANCE(p);
    for(uint sm = 0; sm < NUM_PIO_STATE_MACHINES; sm++) {
        int base = pio_get_gpio_base(pio);
        bool claimed = pio_sm_is_claimed(pio, sm);
        Serial.printf("  pio%d-sm%d ", p, sm);
        Serial.printf("%-10s ", (claimed ? "claimed" : "free"));
        Serial.printf("base=%2d ", base);
        Serial.println();
    }
  }

}

void loop() {}

Result:

claim1: claimed pio2-sm0
  pio0-sm0 free       base= 0 
  pio0-sm1 free       base= 0 
  pio0-sm2 free       base= 0 
  pio0-sm3 free       base= 0 
  pio1-sm0 free       base= 0 
  pio1-sm1 free       base= 0 
  pio1-sm2 free       base= 0 
  pio1-sm3 free       base= 0 
  pio2-sm0 claimed    base= 0 
  pio2-sm1 free       base= 0 
  pio2-sm2 free       base= 0 
  pio2-sm3 free       base= 0 

claim2: claimed pio1-sm0
  pio0-sm0 free       base= 0 
  pio0-sm1 free       base= 0 
  pio0-sm2 free       base= 0 
  pio0-sm3 free       base= 0 
  pio1-sm0 claimed    base=16 
  pio1-sm1 free       base=16 
  pio1-sm2 free       base=16 
  pio1-sm3 free       base=16 
  pio2-sm0 claimed    base= 0 
  pio2-sm1 claimed    base= 0 
  pio2-sm2 claimed    base= 0 
  pio2-sm3 claimed    base= 0

I.e. 3 additional SMs are claimed with the second pio_claim_free_sm_and_add_program_for_gpio_range() call

[peterharperuk]

We will need some test code to demonstrate this issue if it's really a problem

[qqqlab]

Time permitting I'll add some sample code. I have this issue in https://github.com/qqqlab/madflight

Just looking at the current source code it is clear that on pass==1 the unclaim section which is commented with // always un-claim all SMs other than the one we need (array index 0) never gets executed.

This PR moves the unclaim section out of if() statement, so that it is really always executed.

[qqqlab]

Sample program added in updated PR description

[magy00]

Maybe use error codes from error.h instead of hardwiring them to 0 and -1.

[lurch]

Sample program added in updated PR description

It'll be easier for us to test this if you just create your example using standard pico-sdk features, rather than making use of the Arduino extensions. Thanks.

[qqqlab]

create your example using standard pico-sdk features

Sorry, I don't have the standard toolchain installed, nor do I have a great desire to do so.

use error codes from error.h

For me the code is clearer with 0 and -1. The rc variable is used locally only, it is not returned by the function.

[kilograham]

we should probably simplify this to either (num_claimed == pass ? 1 : NUM_PIO_STATE_MACHINES) ? 0 : -1 or even (int) (num_claimed - pass ? 1 : NUM_PIO_STATE_MACHINES)