nrf52 adc fixes

[jacobrosenthal]

Oddly enough I separately came across the same issues last night #951, though I completely disagree with that patch.

The actual bug was that neither the nrf51 or nrf52 init takes its saadc nordic config as input, and SHOULDNT. Resolution and oversample etc are runtime options. The nrf52 didnt know that though and was incorrectly attempting to input those via syscfg, hence @rymanluk trying to fudge them into place.

INSTEAD what should happen is that PR should be reverted and those syscfg values should be replaced with nadc_refmv like the nrf51 driver.

Please see this fix and I request to roll back that change. Or at least we have other breaking

cc @sterlinghughes

[sterlinghughes]

Sorry @jacobrosenthal -- I'm just merge happy, so when I see a approved pull request, I merge it! I'll let @rymanluk comment here, but I think what you're proposing sounds good.

[rymanluk]

@sterlinghughes we had this rule that author does merge it's PRs if has a commit rights. That was handy as sometimes PRs might be there for review but still under testing or similar. Anyway - I think we need to have some ways to notify others to not merge PRs ;) I will start with "Do not merge" in the PR topic.

@jacobrosenthal I suggest to rebase your PR on top of master. Also I think it needs some changes.

Regarding the change. In my patch there was [RFC] not by accident :) So now I understand that idea is that init function takes nrf5X_adc_dev_cfg and open function takes nrfx_saadc_config_t for nrf52 and nrfx_adc_config_t for nrf51. That is fine for me.

However, the issue I see here is that user of ADC needs to have this magic knowledge on what to provide to open function. My solution was not perfect but at least in adc_nrf52.h you could get the idea. Anyway, I believe that for adc resolution/oversample and interrupt priority we could have real HAL API in adc.h which later will be translated by each driver to appropriate values. Does it make sense? If so would it be something you could include into your PR?

[rymanluk]

Look like an error here. This is nrf51 and we don't want to use nrf52 structs here.

[rymanluk]

should we remove ADC_0_REFMV_0:? isn't used above?

[rymanluk]

update: Ah I see you moved it to nrf51xxx which is fine however this is not related to this issue. I would suggest separate patch for this.

[jacobrosenthal]

Id rather fix it here once and for all than edit a bunch more bsps

[rymanluk]

don't we miss related changes in hal_bsp.c for this bsp?

[jacobrosenthal]

Im super excited to talk about changes to the existing ADC api, separately from this. So no I dont want to rebase on master, Id like to revert #951 and merge this PR first to make upstream functional.

[rymanluk]

@jacobrosenthal if there is an issue with rebasing,could you please add "revert" patch into this PR. That should also work. Ping me on slack if you need support in this.

[jacobrosenthal]

If people think rebasing in master is correct we can do that. Ive done a revert here as that feels safer (and fixed that nrf51 error you found @rymanluk )

[jacobrosenthal]

Note Id still like some adc testing done independently. I took some readings with this patch, but not from a known source

[jacobrosenthal]

BTW. This is what I think a blocking read might look like

//syscfg.yml
syscfg.vals:
    ADC_0: 1
	# ADC_0_REFMV_0: 1800
	# ADC_0_REFMV_1: 5000
	# ADC_0_REFMV_VDD: 3200
	
//main.c
#include <adc/adc.h>
#include <nrfx_saadc.h>

void uint8_t
adc_read(){

    int rc;
    struct adc_dev *adc;
    int cnum = 0;
    int adc_result;
    int my_result_mv = 0;

    nrfx_saadc_config_t adc_config = {
        .resolution         = NRF_SAADC_RESOLUTION_8BIT,
        .oversample         = NRF_SAADC_OVERSAMPLE_DISABLED,
        .interrupt_priority = 7,
        .low_power_mode     = NRFX_SAADC_CONFIG_LP_MODE
    };

    nrf_saadc_channel_config_t cc = NRFX_SAADC_DEFAULT_CHANNEL_CONFIG_SE(NRF_SAADC_INPUT_AIN0);

    cc.gain = NRF_SAADC_GAIN1_5;
    cc.reference = NRF_SAADC_REFERENCE_INTERNAL;

    adc = (struct adc_dev *) os_dev_open("adc0", 0, &adc_config);
    assert(adc != NULL);

    rc = adc_chan_config(adc, cnum, &cc);
    assert(rc == 0);

    rc = adc_chan_read(adc, cnum, &adc_result);
    assert(rc == 0);

    my_result_mv = adc_result_mv(adc, cnum, adc_result);

    rc = os_dev_close((struct os_dev *)adc);
    assert(rc == 0);
}

[rymanluk]

Thanks for putting into this PR a "revert patch".
Code looks good to me. The only comment I have is commit message. I think it would be useful for others to have comment here why we need this patch and how resolution, oversample etc are expected to be set.

[rymanluk]

@jacobrosenthal could you please fix commit message so we can merge this PR?

[jacobrosenthal]

I dont know what commit message you want, seems best if you just merge and alter the commit message to suit your request.

[rymanluk]

@jacobrosenthal I wrote it two comments above ;)

" I think it would be useful for others to have comment here why we need this patch and how resolution, oversample etc are expected to be set."

[rymanluk]

In order to make it merged I fixed commit message and merged it as #1052 Hope that this fine with you.
Closing this PR now.