Add adc driver nrf52 #7515
Add adc driver nrf52 #7515
Conversation
|
I think there are quite some things in the |
cpu/nrf52/periph/adc.c
Outdated
| int adc_sample(adc_t line, adc_res_t res) | ||
| { | ||
| int val; | ||
|
|
There was a problem hiding this comment.
add assert(line < ADC_NUMOF);
cpu/nrf52/periph/adc.c
Outdated
| NRF_SAADC->CH[0].PSELP = adc_config[line] + 1; | ||
| NRF_SAADC->CH[0].CONFIG = (SAADC_CH_CONFIG_GAIN_Gain1_4 << SAADC_CH_CONFIG_RESN_Pos) | | ||
| (SAADC_CH_CONFIG_REFSEL_VDD1_4 << SAADC_CH_CONFIG_REFSEL_Pos); | ||
| NRF_SAADC->RESOLUTION = res - 1; |
There was a problem hiding this comment.
this could lead to a negative res value (in case res := 0)? Why not override adc_res_t for the CPU, so that it takes exactly the values allowed/needed?
cpu/nrf52/periph/adc.c
Outdated
| prep(); | ||
|
|
||
| /* set line, resolution and use VDD (+5V) as reference */ | ||
| NRF_SAADC->CH[0].PSELP = adc_config[line] + 1; |
There was a problem hiding this comment.
this should most likely be NRF_SAADC->CH[line]..., right?!
cpu/nrf52/periph/adc.c
Outdated
|
|
||
| /* set the data pointer */ | ||
| NRF_SAADC->RESULT.PTR = (uint32_t)&val; | ||
| NRF_SAADC->RESULT.MAXCNT = 1; |
There was a problem hiding this comment.
why not use a static global var instead of val? Then you can program RESULT.PTR once during adc_init() and don't have to care about it ever again.
cpu/nrf52/periph/adc.c
Outdated
| NRF_SAADC->RESULT.MAXCNT = 1; | ||
|
|
||
| /* calibrate the SAADC */ | ||
| NRF_SAADC->TASKS_CALIBRATEOFFSET = 1; |
There was a problem hiding this comment.
Calibration should be only carried out once during initialization, or not?!
cpu/nrf52/periph/adc.c
Outdated
| /* start the SAADC and wait for the started event */ | ||
| NRF_SAADC->TASKS_START = 1; | ||
| while (NRF_SAADC->EVENTS_STARTED == 0) {} | ||
| NRF_SAADC->EVENTS_STARTED = 0; |
There was a problem hiding this comment.
the safer variant here (and below) would be:
NRF_SAADC->EVENTS_STARTED = 0;
NRF_SAADC->TASKS_START = 1;
while (NRF_SAADC->EVENTS_STARTED == 0) {}
cpu/nrf52/periph/adc.c
Outdated
| /* free device */ | ||
| done(); | ||
|
|
||
| if (val > (1 << 15)) { |
There was a problem hiding this comment.
what is this about? The maximum resolution is 14-bit, so this is always false.
| */ | ||
| #define ADC_CONFIG {0, 4, 5, 6} /* BAT, POT, LIGHT, TEMP */ | ||
| #define ADC_NUMOF (4) | ||
| /** @} */ |
There was a problem hiding this comment.
We could actually remove this block (and the need for board specific configuration for this driver), and simply always map all available (8) channels in order to RIOT adc lines. So using ADC_LINE(x) will always map to the NRFs line x.
There was a problem hiding this comment.
Then we just need to define ADC_NUMOF in the CPUs periph_cpu.h and we are done.
cpu/nrf52/periph/adc.c
Outdated
| /** | ||
| * @brief Load the ADC configuration | ||
| */ | ||
| static const uint8_t adc_config[] = ADC_CONFIG; |
There was a problem hiding this comment.
This also is not needed anymore if we simply map all ADC channels per default.
|
@haukepetersen new commit :) |
| @@ -116,6 +126,7 @@ typedef enum { | |||
| ADC_RES_14BIT = 0xf2, /**< ADC resolution: 14 bit */ | |||
| ADC_RES_16BIT = 0xf3 /**< ADC resolution: 16 bit */ | |||
| } adc_res_t; | |||
| #endif | |||
There was a problem hiding this comment.
looking at this now, I think we should move the typedefs into nrf51/periph_cpu.h and nrf52/periph_cpu.h. That way there is no need for an #ifdef anymore...
|
please squash |
|
Can we get this completed for the release? |
|
I guess so. |
|
Hm, did you significantly change the implementation from the first time I reviewed this? The current implementation is quite inefficient in both run-time and resource usage! Why do you always sample all configured channels if we are only interested in the result of a single one? The way the ADC interface is build, there is just no need for that! |
|
37.5.1 One-shot mode 37.5.4 Scan mode Therefore?! How else we should handle it? |
It is one thing what the hardware has to offer, but another what the interface is abstracting. In our case, the ADC interface is only providing access to the most simple one-shot mode, so there is no need to implement any scan mode behavior in the driver -> the driver should simply implement the functionality needed to comply with the interface in the most efficient way possible! |
|
Yeah, I'm understand. |
Yes, it will and does. See https://github.com/dnahm/RIOT/pull/1 |
|
What is the status: are we progressing here? |
|
@haukepetersen How should we continue? |
depends on the angle.
How about you look at the changes I proposed and you merge them into your branch if you find them valid? Then its just a matter of verifying the changes and we can merge this PR I would say. |
cpu/nrf52: optimized and fixed ADC driver
|
closing in favor of #7985. This was agreed upon offline with @PeterKietzmann and @dnahm |
This PR includes the ADC driver for nrf52.
Based on PR #7501.