Conflict between input.lightLevel and pins.analogReadPin on makecode

[satakagi]

As there are functions on makecode, I do not know whether it is appropriate to put ISSUE here.
See the test code below.

After calling input.lightLevel, when you call pins.analogReadPin, the analog pin value oscillates.
Temporary measures are described in the test code below.

// Press button B and then press button A. Then the bar graph vibrates.
input.onButtonPressed(Button.B, function () {
    mode = 0
})
input.onButtonPressed(Button.A, function () {
    mode = 1
    // It works well if you uncomment the following two lines.
    // pins.digitalWritePin(DigitalPin.P2, 0)
    // led.setDisplayMode(DisplayMode.BlackAndWhite)
})
let mes = 0
let mode = 0
mode = 1
basic.forever(function () {
    if (mode == 0) {
        mes = input.lightLevel()
    } else {
        mes = pins.analogReadPin(AnalogPin.P2) / 4
    }
    led.plotBarGraph(
        mes,
        255
    )
})

https://makecode.microbit.org/_YLeAM8JDAF5x

[martinwork]

The following change may be a fix. I haven't properly tested or looked up the details of the CONFIG settings. The differences are to match analogin_init and to reset NRF_ADC->ENABLE. Should it in fact avoid setting the bits of the config that it doesn't need to?

@jamesadevine ? @finneyj ?

void MicroBitLightSensor::analogDisable()
{
  uint32_t was = NRF_ADC->ENABLE;

    NRF_ADC->ENABLE = ADC_ENABLE_ENABLE_Disabled;

    //NRF_ADC->CONFIG = (ADC_CONFIG_RES_8bit << ADC_CONFIG_RES_Pos) |
    //                  (ADC_CONFIG_INPSEL_SupplyTwoThirdsPrescaling << ADC_CONFIG_INPSEL_Pos) |
    //                  (ADC_CONFIG_REFSEL_VBG                       << ADC_CONFIG_REFSEL_Pos) |
    //                  (ADC_CONFIG_PSEL_Disabled                    << ADC_CONFIG_PSEL_Pos) |
    //                  (ADC_CONFIG_EXTREFSEL_None                   << ADC_CONFIG_EXTREFSEL_Pos);

    NRF_ADC->CONFIG = (ADC_CONFIG_RES_10bit << ADC_CONFIG_RES_Pos) |
                      (ADC_CONFIG_INPSEL_AnalogInputOneThirdPrescaling << ADC_CONFIG_INPSEL_Pos) |
                      (ADC_CONFIG_REFSEL_SupplyOneThirdPrescaling << ADC_CONFIG_REFSEL_Pos) |
                      (ADC_CONFIG_PSEL_Disabled                    << ADC_CONFIG_PSEL_Pos) |
                      (ADC_CONFIG_EXTREFSEL_None << ADC_CONFIG_EXTREFSEL_Pos);

    NRF_ADC->ENABLE = was;
}

[jamesadevine]

@martinwork I think your patch makes sense, it should definitely align with the ADC init code if we want to cause minimal disruption to application code. However, it's unclear to me whether the issue is caused by differing ADC configurations or by competition over the ADC peripheral.

For instance, there is a period of time between triggering a light sense and receiving the final value (around 4ms) in which application code could use the ADC for another pin and leave it in a bad state for when the light sensor returns. The more likely scenario is that the light sensor is triggering in the middle of an ADC sampling from the pins API.

[martinwork]

Thanks @jamesadevine . You're right, of course! The ADC is shared and the light sensor code is going to read values at the same time as MicroBitPin. The above change improves things, but doesn't fix the conflict.

A related issue... Changing any pin from analogue to digital disables the ADC for all pins in MicroBitPin::disconnect(). Changing any pin from digital to analogue fixes it for all pins.

[cpseager]

See #315 for that related issue

[martinwork]

Thanks @cpseager . Maybe something like the above could fix that.

[kevinjwalters]

Is this problem limited to the micro:bit?

[tsunyi]

I fix the issue with follow code, but my micropython code based on this modified dal run error with display.read_light_level().

void MicroBitLightSensor::analogDisable()
{
    //NRF_ADC->ENABLE = ADC_ENABLE_ENABLE_Disabled;

    // NRF_ADC->CONFIG = (ADC_CONFIG_RES_8bit << ADC_CONFIG_RES_Pos) |
    //                   (ADC_CONFIG_INPSEL_SupplyTwoThirdsPrescaling << ADC_CONFIG_INPSEL_Pos) |
    //                   (ADC_CONFIG_REFSEL_VBG                       << ADC_CONFIG_REFSEL_Pos) |
    //                   (ADC_CONFIG_PSEL_Disabled                    << ADC_CONFIG_PSEL_Pos) |
    //                   (ADC_CONFIG_EXTREFSEL_None                   << ADC_CONFIG_EXTREFSEL_Pos);

    NRF_ADC->CONFIG = (ADC_CONFIG_RES_10bit << ADC_CONFIG_RES_Pos) |
                      (ADC_CONFIG_INPSEL_AnalogInputOneThirdPrescaling << ADC_CONFIG_INPSEL_Pos) |
                      (ADC_CONFIG_REFSEL_SupplyOneThirdPrescaling << ADC_CONFIG_REFSEL_Pos) |
                      (ADC_CONFIG_PSEL_Disabled << ADC_CONFIG_PSEL_Pos) |
                      (ADC_CONFIG_EXTREFSEL_None << ADC_CONFIG_EXTREFSEL_Pos);
}