adc_nrfx_saadc: set_resolution: ADC resolution value 0 is not valid.

[builder01]

Hi,

I took a look a your battery.c file. However, I found it had too much overhead code that does not apply to me (I'm using the nRF52 DK nrf52832 and the nRF Connect SDK).

So, I came up with the following code:

battery.c

/**
 * @file battery.c
 * @author Ernesto Gonzales
 * @brief Battery Measurement Module (using nRF52 ADC)
 * @version 0.1
 * @date 2023-02-16
 * 
 * @copyright Copyright (c) 2023
 * 
 */
#include <math.h>
#include <stdio.h>
#include <stdlib.h>

#include <zephyr/kernel.h>
#include <zephyr/init.h>
#include <zephyr/drivers/gpio.h>
#include <zephyr/drivers/adc.h>
#include <zephyr/drivers/sensor.h>
#include <zephyr/logging/log.h>

#include <zephyr/device.h>
#include <zephyr/devicetree.h>

#include "battery.h"
#define LOG_LEVEL CONFIG_BT_BAS_LOG_LEVEL
LOG_MODULE_REGISTER(BATTERY,CONFIG_ADC_LOG_LEVEL);

#define ZEPHYR_USER DT_PATH(zephyr_user)

//Since the ADC was set with a devicetree overlay, we can call the settings with:
//static const struct adc_channel_cfg ch4_cfg = ADC_CHANNEL_CFG_DT(DT_CHILD(DT_NODELABEL(adc),channel_4));

static const struct adc_dt_spec adc_ch4 = ADC_DT_SPEC_GET_BY_IDX(ZEPHYR_USER,0);
/* Initializes the adc_ch4 to:
 {
     .dev = DEVICE_DT_GET(DT_NODELABEL(adc)),
     .channel_id = 4,
     .channel_cfg_dt_node_exists = true,
     .channel_cfg = {
         .channel_id = 4,
         .gain = ADC_GAIN_1_5,
         .reference = ADC_REF_INTERNAL,
         .acquisition_time = ADC_ACQ_TIME_DEFAULT,
     },
     .vref_mv = 1200,
     .resolution = 12,
     .oversampling = 1,
 }
*/

bool battery_ok = false;
struct battery_measurement_periphs {

    const struct adc_dt_spec *adc_ch4;
    struct adc_sequence *adc_seq;
    uint16_t raw_data;

};

// Structure only for this file, not to be shared outside
static struct battery_measurement_periphs batt_meas = {
    .adc_ch4 = &adc_ch4,
    .adc_seq = &(struct adc_sequence) {

        .channels = BIT(4),
        .buffer = &(batt_meas.raw_data),
        .buffer_size = sizeof(batt_meas.raw_data),
        .calibrate = true,
    },
};

static int battery_meas_setup(const struct device *arg){

    const struct battery_measurement_periphs *batt_meas_setup = &batt_meas; 
    int success;

    /*
        Check wether device is ready, first.
    */
    if(!device_is_ready(batt_meas_setup->adc_ch4->dev)){
        LOG_INF("ADC device is NOT ready!");
        return -ENOENT;
    }

    // 0 = success
    success = adc_channel_setup_dt(batt_meas_setup->adc_ch4);
    if(success != 0){
        LOG_INF("Could NOT setup the ADC channel!");
        return -ENOENT;
    }

    battery_ok = true;
    LOG_INF("Battery measurement setup correctly!");
    return success;
}


//System drivers: Use SYS_INIT() when you need to run a device's function at boot.
SYS_INIT(battery_meas_setup,APPLICATION,CONFIG_APPLICATION_INIT_PRIORITY);

int battery_sample(void){
    int success = -ENOENT;
    int32_t val;
    if(battery_ok){
        const struct battery_measurement_periphs *batt_setup = &batt_meas;
        struct adc_sequence *adc_seq = batt_setup->adc_seq;

        success = adc_read(batt_setup->adc_ch4->dev, adc_seq);
        adc_seq->calibrate = false;
        if (success == 0){

            val = batt_setup->raw_data;
            adc_raw_to_millivolts_dt(batt_setup->adc_ch4, &val);
            LOG_INF("Battery read (mV) = %d",val);
            return (int)(val/(batt_setup->adc_ch4->vref_mv));

        } else {

            LOG_INF("Error getting ADC reading.");
            return success;
            
        }
        
    } else {
        LOG_INF("Battery not ok!");
        return success;
        
    }
}

and battery.h only shows the battery_sample() function:

/**
 * @file battery.h
 * @author Ernesto Gonzales
 * @brief Battery Measurement Module (using nRF52 ADC)
 * @version 0.1
 * @date 2023-02-16
 * 
 * @copyright Copyright (c) 2023
 * 
 */
#ifndef APPLICATION_BATTERY_H
#define APPLICATION_BATTERY_H

/**
 * @brief Takes sample measurement from the battery connected to the relevant GPIO  
 * 
 * @return an integer representing the battery capacity in percentage (0-100)
 */
int battery_sample(void);

#endif

I also implemented a simple bas and in the main.c I'm using the following function in an infinite loop to measure battery:

static void bas_notify(void)
{
	uint8_t batt_level = battery_sample();
	if (batt_level > 100) {
		LOG_INF("Battery level measured is higher than 100 percent");
	}

	bt_bas_set_battery_level(batt_level);
}

As you can see from battery.c, I'm obtaining the ADC initialization structure from a Devicetree overlay as follows:

/ {
    zephyr,user {
    io-channels = <&adc 4>;
    };
};

&adc {
    #address-cells = <1>;
    #size-cells = <0>;

    channel@4{
        reg = <4>;
        zephyr,gain = "ADC_GAIN_1_5";
        zephyr,reference = "ADC_REF_INTERNAL";
        zephyr,acquisition-time = <ADC_ACQ_TIME(ADC_ACQ_TIME_MICROSECONDS, 20)>;
        zephyr,input-positive = <NRF_SAADC_AIN4>;
        zephyr,resolution = <12>;
        zephyr,vref-mv=<1200>;
    };
};

However, I'm getting a failure using the ADC read function, as it's not returning 0.

On top of that I get the following message: "adc_nrfx_saadc: set_resolution: ADC resolution value 0 is not valid"

How can I solve this? I thought the devicetree should've taken care of initializing with the right resolution.

[haakonsh]

Can you share the value of adc_seq->resolution before calling adc_read(batt_setup->adc_ch4->dev, adc_seq) ?

[builder01]

I added the following code around adc_read():

//...
if(battery_ok){
        const struct battery_measurement_periphs *batt_setup = &batt_meas;
        struct adc_sequence *adc_seq = batt_setup->adc_seq;

        LOG_INF("ADC Resolution: %d", batt_setup->adc_ch4->resolution);
        
        success = adc_read(batt_setup->adc_ch4->dev, adc_seq);
        adc_seq->calibrate = false;
        //..

The following is printed:

So, it seems that the resolution is correctly obtained.
Do you find, perhaps, something wrong in my battery.c code?

[haakonsh]

batt_setup->adc_ch4->resolution is not equal to adc_seq->resolution.

You've only partially defined the adc_seq struct. 

Also, I believe the adc node property 'zephyr,vref-mv' should be removed as adc_nrfx_saadc.c sets dev->api.ref_internal to 600. 

[builder01]

Thank you, this solved it.

I have a couple of follow-up questions though, hope you don't mind answering them:

1) I tested the voltage measurement with a power supply (feeding 1V only) through a divider. The accuracy of the measurement is good enough for my purposes. However, when I turn off the power supply, the battery reading (in mV, for 10 bits) jumps between 191900 and 2. How can you account for a battery disconnection that will not give you wrong results? is it possible to pull it down in case I read such a high a value?

2) In my main code, I'm running the following infinite while() loop for battery measurement:

	while (1){
		k_sleep(K_SECONDS(1));
		bas_notify();
	}

The bas_notify() function I showed it in my original post.

Of course, for the real implementation, I'll space this battery measurement as much as a possible (perhaps 10s of seconds or a minute). If I space them out, will this allow me to save battery (so that my ADC is not measuring continuously), or do I have to do something extra in my code so that the ADC is only turned on and measuring as my while() loop says.

3) In https://devzone.nordicsemi.com/nordic/nordic-blog/b/blog/posts/measuring-lithium-battery-voltage-with-nrf52 says that I choose a low enough sampling frequency, the ADC input impedance will appear large, which will benefit the accuracy of the battery measurement. However, I cannot see any function in the ADC documentation (https://developer.nordicsemi.com/nRF_Connect_SDK/doc/latest/zephyr/hardware/peripherals/adc.html) to modify the sampling frequency. How can I do it, then?

4) In that same article, it is said that if I use a capacitor to ground big enough, I can reduce the acquisition time, and thus, reduce ADC current consumption. I think it has to do with the R||C impedance of the shunt length of the R-divider being small at a high frequency, but I don't see exactly how to put it in numbers. Do you have some extra documentation on that?

5) In my code, the adc_ch4 is an static const struct. I'd like to be able to retrieve the resolution from it and put it in the sequence struct. However, I cannot do so because the resolution member of the adc_ch4 is also an static const.

For now, I just hardcoded it, but this is error-prone because I need to set the same resolution in both dt overlay and this struct.

How can I do to retrieve it from the dt overlay without hardcoding it?

Thanks

[haakonsh]

1. I don't fully undestand the issue here. 

   builder01 said:

   However, when I turn off the power supply, the battery reading (in mV, for 10 bits) jumps between 191900 and 2.
2. battery_sample seems to be optimized already as the SAADC is turned off between measurements, so you just need to decide on how often you want to sample.
   
   The syscall adc_read() ends up in a call to adc_nrfx_read(), who in turn calls 
   start_read(), who in turn starts reading with a call to 

   adc_context_start_read(), and then waits for completion with a call to 

   adc_context_wait_for_completion(). 
   
   When the saadc_irq_handler is called with a NRF_SAADC_EVENT_END it will disable the SAADC and release the sempahore that adc_context_wait_for_completion() is waiting for. 
   
   See zephyr\drivers\adc\adc_nrfx_saadc.c line 358 and 386.
   
   
3. Aquisition time, not sample rate, and it should have been take care of by battery.c already, though you should verify it. 
   
   The Aquisition time is the time allotted for the sample-and-hold circuit to charge its input capacitor. If the signal current and aquisuition time is too low the input capacitor will not reach the level of the signal when it gets sampled. 
   
   
4. The extra capacitor is effectively an extra input capacitor. As the internal input capacitor will only get connected during a sampling event it will not get charged between samples. Wheras an external capacitor will. 
   Though you need to take care as battery.c uses oversampling you will need to tune the external cap value and acquisition time accordingly.
   
   
5.  

   builder01 said:

   In my code, the adc_ch4 is an static const struct. I'd like to be able to retrieve the resolution from it and put it in the sequence struct. However, I cannot do so because the resolution member of the adc_ch4 is also an static const.

   I don't understand why you can't pass the value of the resolution member of adc_ch4, like this: *batt_meas->adc_seq = adc_ch4->resolution;

[builder01]

1) Ok, I realized my problem was a bit involved than I thought. I found that, if I disconnect the voltage divider out of the ADC input, the readings I get in software oscillate between 19V and 2mV.

In other words, I get garbage readings.

This would mimic a situation where there's a connection problem in my board between the voltage divider that measures the battery voltage and the ADC input.

Sounds to me that, the easiest way to handle this, is to collect a few samples of these garbage readings and decide that I cannot sense the battery and provide an error message. What do you think?

2) Got it, thanks.

3) This article: Measuring Lithium battery voltage with nRF52 says explicity, and I quote:

Choosing the sampling frequency

Over time the SAADC input can be seen as a resistor with the value 1/(f*C). The sampling frequency should be chosen such that the saadc input impedance is much larger than the resistor values in the voltage divider

I know the acquisition time can be chosen, and I already did. I also guess acquisition time is correlated with sampling frequency, but from the article I quote it's implied that you can choose the sampling frequency independently, but they don't say how, nor do I find anything related in the documentation. Since I want to measure a DC quantity, lowering the sampling rate would be very beneficial.

4) Got it, thanks.

5) So, I did the following and it worked:

static struct battery_measurement_periphs batt_meas = {
    .adc_ch4_ptr = &adc_ch4,
    .adc_seq = &(struct adc_sequence) {

        .channels = BIT(4),
        .buffer = &(batt_meas.raw_data),
        .buffer_size = sizeof(batt_meas.raw_data),
        .calibrate = true,
        //.resolution = adc_ch4.resolution,
    },
};

static int battery_meas_setup(const struct device *dev){

    ARG_UNUSED(dev);
    //Defining resolution here
    batt_meas.adc_seq->resolution = adc_ch4.resolution;
    
    const struct battery_measurement_periphs *batt_meas_setup = &batt_meas; 
    int success;
    //...

My question was: how can I, instead of doing what I just did, have the resolution designated initializer within the "batt_meas" designated initializers?
I tried:
.resolution = adc_ch4.resolution, but I got an error about trying to initialize .resolution with a non-const struct member.

[haakonsh]

1)

builder01 said:

Ok, I realized my problem was a bit involved than I thought. I found that, if I disconnect the voltage divider out of the ADC input, the readings I get in software oscillate between 19V and 2mV.

This must be an artefact of the battery measurement module/driver. Can you verify whether this is a physical transient or not?

At 19V there's likely damage to the ESD diodes of the analog input pin, and the measurements should not be trusted. If this is expected operating conditions of your design you need to add some extra input protection.

3)
That statement is a tiny bit misleading as the increased acquisition time has already limited the sample frequency such that the SAADC can sample an 800kOhm source without adding additional sampling errors.

If you add an external capacitor and have a source impedance greater than 800kOhm you will need to calculate and set your external capacitance and sample frequency accordingly.

5)
//.resolution = adc_ch4.resolution. If I'm not mistaken it seems your are assigning a 32-bit address to a uint8_t variable that is not a pointer.


Does this work?

static struct battery_measurement_periphs batt_meas = {
    .adc_ch4_ptr = &adc_ch4,
    .adc_seq = &(struct adc_sequence) {

        .channels = BIT(4),
        .buffer = &(batt_meas.raw_data),
        .buffer_size = sizeof(batt_meas.raw_data),
        .calibrate = true,
        .resolution = adc_ch4_ptr->resolution,
    },
};