Converting a pin from ADC input to digital stops other ADCs too

[cpseager]

Changing a pin from adc_input back to digital state seems to kill all ADCs on other pins too.

Steps to reduce - I'm Using BLE with P1 and P2 starting out as analog inputs.

.then(()=>{
  // P0 as output for piezo, P1 and P2 as inputs
  return this.writeCharacteristicValue(MICROBIT_IO_PIN_CONFIG, [0x06, 0x00, 0x00, 0x00])
})
.then(()=>{
  // P0 as analog (output) for piezo, P1 and P2 as analog (inputs). All Ok here, get notifications on both ADC.
  return this.writeCharacteristicValue(MICROBIT_IO_PIN_AD_CONFIG, [0x07, 0x00, 0x00, 0x00])
})

All ok here, P1 and P2 notifying any adc change

Now convert P1 to digital output after being an analog input

.then(()=>{
  // P0 as output for piezo, P1 and P2 as inputs
  return this.writeCharacteristicValue(MICROBIT_IO_PIN_CONFIG, [0x06, 0x00, 0x00, 0x00])
})
.then(()=>{
  // P0 as analog (output) for piezo, P1 and P2 as analog (inputs). All Ok here, get notifications on both ADC.
  return this.writeCharacteristicValue(MICROBIT_IO_PIN_AD_CONFIG, [0x07, 0x00, 0x00, 0x00])
})
.then(()=>{
  // Now just P2 as input
  return this.writeCharacteristicValue(MICROBIT_IO_PIN_CONFIG, [0x04, 0x00, 0x00, 0x00])
})
.then(()=>{
  // P0 atill analog (output) for piezo, P1 as digital output, leave P2 as analog (input)
  return this.writeCharacteristicValue(MICROBIT_IO_PIN_AD_CONFIG, [0x05, 0x00, 0x00, 0x00])
})
.then(()=>{
  //switch P1 high. When this occurs P2 stops notifying adc changes. Why - all ADCs are now off.
  return this.writeCharacteristicValue(MICROBIT_IO_PIN_DATA, [0x01, 0x01])
})

We haven't changed P2 so it should still be sending ADC notifcations, but it is not.

The workaround is to change P2 back to digital and then back to analog again to kick the adc back into life.

.then(()=>{
  // P0 as output for piezo, P1 and P2 as inputs
  return this.writeCharacteristicValue(MICROBIT_IO_PIN_CONFIG, [0x06, 0x00, 0x00, 0x00])
})
.then(()=>{
  // P0 as analog (output) for piezo, P1 and P2 as analog (inputs). All Ok here, get notifications on both ADC.
  return this.writeCharacteristicValue(MICROBIT_IO_PIN_AD_CONFIG, [0x07, 0x00, 0x00, 0x00])
})
.then(()=>{
  // Now put P1 back to output state, leaving P2 as input
  return this.writeCharacteristicValue(MICROBIT_IO_PIN_CONFIG, [0x04, 0x00, 0x00, 0x00])
})
.then(()=>{
  // P0 atill analog (output) for piezo, P1 as digital output, leave P2 as analog (input)
  return this.writeCharacteristicValue(MICROBIT_IO_PIN_AD_CONFIG, [0x05, 0x00, 0x00, 0x00])
})
.then(()=>{
  //switch P1 high. When this occurs P2 stops notifying adc changes. Why?
  return this.writeCharacteristicValue(MICROBIT_IO_PIN_DATA, [0x01, 0x01])
})
.then(()=>{
  // workaround - have to take P2 to digital then back to ADC
  return this.writeCharacteristicValue(MICROBIT_IO_PIN_AD_CONFIG, [0x01, 0x00, 0x00, 0x00])
})
 .then(()=>{
  return this.writeCharacteristicValue(MICROBIT_IO_PIN_AD_CONFIG, [0x05, 0x00, 0x00, 0x00]);
});

I suspect this may be the cause, we don't want to turn the ADC module off if other pins are still using it....

https://github.com/lancaster-university/microbit-dal/blob/5fa55812c7be1e428d9b255ca31b21cd66c72e4a/source/drivers/MicroBitPin.cpp#L85

[finneyj]

Thanks @cpseager Nice analysis!

That line does indeed look wrong to me - as you say, just because one pin changes mode, it doesn't follow that the associated module should be disabled.

I don't remember writing this code, so I've been back through the commit history and can't find a reference to it... so it must have been there a while (pre github).

Two possible ways forward:

1) We maintain a count of active pins and only disable when the last one goes away 2) We just take that line of code out and see what happens. :)

The latter may not be a silly as it sounds - the implications of leaving the ADC running may only be additional energy use for this corner case... which is probably preferably to the additional code size and complexity.

Also folding in @jamesadevine here as the second most likely contributor of this. James - do you remember the rationale for introducing this line of code?

[jamesadevine]

It was definitely me, looking at the mbed history. It was added due to a bug when swapping from AnalogIn to DigitalOut, commit says that the pin is not released for GPIO. I do think this is the wrong fix however...

From the NRF PAN (Product Anomaly Notice):

3. ADC: Setting ENABLE register to Disabled does not release the pins captured for GPIO.
--
Symptoms:                               Pins used previously for the ADC cannot be used as GPIO.
Conditions:                                 After assigning those pins to the ADC and enabling the ADC.
Consequences:                               Pins cannot be used as GPIOs after being used as analog pins for the ADC.
Workaround:                                 Execute the following code after setting ADC.ENABLE register to Disabled.                                                                                                                                       NRF_ADC->CONFIG = (ADC_CONFIG_RES_8bit (ADC_CONFIG_INPSEL_SupplyTwoThirdsPrescaling                                 (ADC_CONFIG_REFSEL_VBG (ADC_CONFIG_PSEL_Disabled (ADC_CONFIG_EXTREFSEL_None                                                                                         << ADC_CONFIG_RES_Pos) \| << ADC_CONFIG_INPSEL_Pos) \| << ADC_CONFIG_REFSEL_Pos) \| << ADC_CONFIG_PSEL_Pos) \| << ADC_CONFIG_EXTREFSEL_Pos);

The code referenced is used in the LightSensor.

Perhaps MicroBitPin should use this instead?

[finneyj]

Thanks @jamesadevine - that explains it.

So I think there's a few different things here.

1) There's the Product Anomaly that means we need to perform the little dance you cite above when the ADC is no longer needed.

2) We still need to know when the ADC should be disabled/enabled (which is different, as other ADC channels might still be in use).

This kindof feels like it should be fixed in the underlying mbed driver... but looking here, there's no obvious callback for handling shutdown of an ADC... https://github.com/lancaster-university/mbed-classic/blob/master/targets/hal/TARGET_NORDIC/TARGET_MCU_NRF51822/analogin_api.c

So I guess we'll need to handle this in MicroBitPin by ref counting the number of ADC channels in use.

[cpseager]

Sorry hit wrong button

[cpseager]

Don't you already know which pins have adc enabled from their IO_STATUS_ANALOG_IN flag? As that section of code is so rarely used it may be possible to just disable everything as at present and then just cycle through all the pins and re-enable any that have the flag set? Not very fast but not often used either.

[finneyj]

Thanks @cpseager - good idea. As you say, very rarely used code, so burning a few extra cycles is no big deal.

I'm also a fan of the magical @smartyw mechanical turk compiler (it gives excellent and friendly debug output compared to your typically gcc build too!), but in this case I'll build this one up on a branch and share around a binary for testing...

[martinwork]

Could we simply do the following or is it necessary to change the resolution, etc. as under PAM 3?

uint32_t was = NRF_ADC->ENABLE;

NRF_ADC->ENABLE = ADC_ENABLE_ENABLE_Disabled;
NRF_ADC->CONFIG = ( configWas & ~ADC_CONFIG_PSEL_Msk) | ( ADC_CONFIG_PSEL_Disabled << ADC_CONFIG_PSEL_Pos);

NRF_ADC->ENABLE = was;