[RFC][DNM] drivers: adc: new API proposal #6176

anangl · 2018-02-13T13:49:48Z

This is an initial proposal of a new API for the ADC driver, so that we could start discussion and conclude what is actually needed in the API, and what will be convenient to use.
I tried to keep similarity with the new SPI API (#5921).

A few questions/doubts that I have:

Is there a need to use a bigger type than s16_t for sample values?
How can gain/prescaling settings be effectively passed to the driver? I suppose that these settings will vary significantly between different controllers.
Are there any other important configuration settings (of the ADC as a whole, or particular channels) that I haven't included yet?

@nashif @carlescufi @tbursztyka @MaureenHelm @andrewboie @galak

codecov-io · 2018-02-13T14:07:01Z

Codecov Report

Merging #6176 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #6176   +/-   ##
=======================================
  Coverage   58.59%   58.59%           
=======================================
  Files         464      464           
  Lines       47483    47483           
  Branches     8796     8796           
=======================================
  Hits        27822    27822           
  Misses      15823    15823           
  Partials     3838     3838

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 597517c...c171e53. Read the comment docs.

tbursztyka

samples value size will depend on resolution, so 8, 16, 24, 32, 64...?
Instead, I would make in void *, and depending on the resolution, the driver would fill it in accordingly.
Only thing would be that driver would check the buffer length vs resolution at first.
Just thinking out loud, I don't have much experience in ADCs (besides the awful adc_ti_adc108s102 I did back in the days)

tbursztyka · 2018-02-13T15:07:56Z

include/adc.h

some description?

tbursztyka · 2018-02-13T15:08:51Z

include/adc.h

so 0 would mean bare result?

tbursztyka · 2018-02-13T15:10:25Z

include/adc.h

ticks is hw dependent. Wouldn't it be better to work on µs or ms? (Well, µs is only supported through busy wait loop)

I'm torn on this one. It's a pain to do the conversion in the driver and often inefficient due to divide operations, but I can see the value in having common units like us or ms for the application.

galak

I get why adc_channel_setup() takes a dev pointer and the others dont, but the inconsistency is unfortunate.

galak · 2018-02-13T15:53:14Z

include/adc.h

we should add defines & macros to access the fields your are defining.

Do these things ever change on a per-channel basis? They don't on NXP devices

galak · 2018-02-13T15:59:40Z

include/adc.h

Wonder if sample_cfg should be separated from sample_results

It would be more consistent with spi to separate them

galak · 2018-02-13T16:03:32Z

include/adc.h

This comment seems incorrect.

We should document the following - is it required to call adc_channel_setup before adc_read*?

galak · 2018-02-13T16:09:50Z

include/adc.h

any reason channel_id isn't in adc_channel_cfg?

vaussard · 2018-02-15T11:36:50Z

include/adc.h

6 bits for the acquisition time looks a bit short, as 63 microseconds may be too small if you sample a high-impedance source or your ADC runs on a very slow clock.
On the other hand, 1us is rather high if you go high speed. For the STM32L4A6, I can setup Tacq as low as 30 nanoseconds.
So maybe we need a more flexible approach to specify the conversion time?

vaussard · 2018-02-15T11:43:30Z

include/adc.h

s16_t is not enough, I used external 24-bit ADC in the past. And you need more bits when doing oversampling.

vaussard · 2018-02-15T11:49:56Z

A missing feature is the ability to setup the trigger source. Here a software trigger is implied, but we have to use hardware triggers sometime. I do not see a straightforward way to setup it as it is usually hardware dependant, but maybe some kind of driver-specific API (like uart_drv_cmd() or Linux ioctl) ?

anangl · 2018-02-16T21:06:58Z

I will be on vacation all next week. I'll address all the comments (already existing ones and any new that may appear, and are welcome) when I'm back.

MaureenHelm

A missing feature is the ability to setup the trigger source. Here a software trigger is implied, but we have to use hardware triggers sometime. I do not see a straightforward way to setup it as it is usually hardware dependant,

Agreed

MaureenHelm · 2018-03-27T12:26:47Z

include/adc.h

It would be more consistent with spi to separate them

MaureenHelm · 2018-03-27T12:33:09Z

include/adc.h

I'm torn on this one. It's a pain to do the conversion in the driver and often inefficient due to divide operations, but I can see the value in having common units like us or ms for the application.

MaureenHelm · 2018-03-27T12:33:25Z

include/adc.h

MaureenHelm · 2018-03-27T12:41:27Z

include/adc.h

Do these things ever change on a per-channel basis? They don't on NXP devices

MaureenHelm · 2018-03-27T12:44:03Z

include/adc.h

Please add the dev pointer argument back and remove it from the adc_config struct

anangl · 2018-03-27T14:16:04Z

Tried to address gathered feedback, to some extent at least.
Please consider it still as a work in progress, more a base for discussion than a real code.

tbursztyka · 2018-03-27T16:12:48Z

include/adc.h

tiny left over from previous PR push

Right. I haven't tried asynchronous part yet. That's why I haven't got any compile error for this.

tbursztyka

tiny changes requested. (mostly on struct alignment)

tbursztyka · 2018-04-12T10:06:48Z

include/adc.h

to be consistent with other APIs, create function typedefs for the functions first

tbursztyka · 2018-04-12T10:07:44Z

include/adc.h

I would reorder the attributes so these 2 u8_t would be at the end of the struct.

tbursztyka · 2018-04-12T10:09:01Z

include/adc.h

I would move this attribute at the end of the struct

tbursztyka · 2018-04-12T10:09:34Z

include/adc.h

move these 2 attributes at the beginning of the struct

MaureenHelm · 2018-04-16T15:09:21Z

include/adc.h

Do they need to be unique? (no duplicates)

MaureenHelm · 2018-04-16T15:13:23Z

include/adc.h

Maybe this should be channel_count? It's a little confusing to have "samplings" and "samples" be different things

I simplified the structure and used a bit-mask for specifying the channels to be included in the sampling. The previous proposal with the channel_ids pointer and sample_count was too confusing and probably troublesome in implementation (regarding duplicates, as you pointed out in the comment above, and order of the channels).

My idea was to define a sampling as reading of samples from one or more channels. Such samplings can be then arranged in sequences with optional triggers. I added more comments in the header file and changed some names to hopefully present the idea clearer. I am open to change the terminology to a better one. English is not my native language so the names I propose may not be the best (or even good enough) ones.

MaureenHelm · 2018-04-16T15:17:29Z

include/adc.h

Are the triggers one-off or recurring (such as from a hardware timer)? Can you explain the difference between the DELAY triggers and the SAMPLE_RATE trigger?

I need to work more on these. The rough idea, just to start with something, was that the DELAY triggers will be done in software (with some sleeping or busy loop before doing next sampling) and the SAMPLE_RATE one (the name was rather a place holder) by hardware, e.g. some timer providing means to achieve accurate sampling intervals.

MaureenHelm · 2018-04-16T15:18:01Z

include/adc.h

A sampling is a group of channels?

Yes. I added better, hopefully, descriptions of the structure and its fields.

MaureenHelm · 2018-04-16T15:21:32Z

include/adc.h

Are these numbers non-integer gain factors? We will need more values in the enum, as the nxp adc supports gain factors of {1,2,4,8,16,32,64}

Yes, these are fractional factors. I added proper comments, and renamed "prescaling" to "gain".

anangl · 2018-04-17T09:32:24Z

Updated the proposal according to the recent suggestions. Added more descriptions to make things clearer.
Some parts, like triggers, need more work. I continue to work on them and I will update the proposal when I got something worth presenting.

This is a proposal of a new API for ADC driver to address reported issues (zephyrproject-rtos#3980) and clarify confusing things, like the buffer length. Asynchronous requests and channel configuration were added as well. Signed-off-by: Andrzej Głąbek <[email protected]>

anangl · 2018-04-23T15:43:58Z

I updated the proposal once more. I got rid of the adc_sampling structure - its fields became members of the adc_sequence structure. I think this way it will be less confusing and simpler for example for single channel samplings, and the possibility of having a sequence of samplings each performed for a different sets of selected channels, which is no longer present, was actually not that much useful.
I added also actions that the application can pass from the callback to driver to alter the performed sequence.
And finally, I updated the adc_api test to show my vision of the usage of the proposed API.
Sample output:

***** Booting Zephyr OS v1.11.0-763-gd27904790 *****
Running test suite adc_basic_test
===================================================================
starting test - test_adc_sample_one_channel
Samples read: 0x034c 0x0000 0x0000 0x0000 0x0000 0x0000
PASS - test_adc_sample_one_channel.
===================================================================
starting test - test_adc_sample_two_channels
Samples read: 0x034b 0x034e 0x0000 0x0000 0x0000 0x0000
PASS - test_adc_sample_two_channels.
===================================================================
starting test - test_adc_sample_with_delay_trigger
delay_trigger_callback: sampling 0
delay_trigger_callback: sampling 1
delay_trigger_callback: sampling 2
delay_trigger_callback: sampling 3
delay_trigger_callback: sampling 4
Samples read: 0x034d 0x034b 0x034c 0x034b 0x034b 0x0000
PASS - test_adc_sample_with_delay_trigger.
===================================================================
starting test - test_adc_repeated_samplings
repeated_samplings_callback: done 1
Samples read: 0x034b 0x034b 0x0000 0x0000 0x0000 0x0000
repeated_samplings_callback: done 2
Samples read: 0x0348 0x034b 0x0000 0x0000 0x0000 0x0000
repeated_samplings_callback: done 3
Samples read: 0x034e 0x034b 0x0000 0x0000 0x0000 0x0000
repeated_samplings_callback: done 4
Samples read: 0x034b 0x034b 0x0000 0x0000 0x0000 0x0000
repeated_samplings_callback: done 5
Samples read: 0x034c 0x034c 0x0000 0x0000 0x0000 0x0000
repeated_samplings_callback: done 6
Samples read: 0x034b 0x034b 0x0000 0x0000 0x0000 0x0000
repeated_samplings_callback: done 7
Samples read: 0x034c 0x034c 0x0000 0x0000 0x0000 0x0000
repeated_samplings_callback: done 8
Samples read: 0x034c 0x034c 0x0000 0x0000 0x0000 0x0000
repeated_samplings_callback: done 9
Samples read: 0x034a 0x034a 0x0000 0x0000 0x0000 0x0000
repeated_samplings_callback: done 10
Samples read: 0x034c 0x034c 0x0000 0x0000 0x0000 0x0000
PASS - test_adc_repeated_samplings.
===================================================================
===================================================================
PROJECT EXECUTION SUCCESSFUL

Updated API calls and added a few new test cases so that the new possibilities of the API could be presented and checked. Signed-off-by: Andrzej Głąbek <[email protected]>

tbursztyka · 2018-05-04T12:12:14Z

Seems fine to me.
You seem to have forgotten the drivers update? (to get your test running)

anangl · 2018-05-21T15:09:13Z

Replaced by #7691.

galak mentioned this pull request Feb 13, 2018

Driver API review/cleanup/rework #5697

Closed

33 tasks

tbursztyka reviewed Feb 13, 2018

View reviewed changes

galak requested changes Feb 13, 2018

View reviewed changes

galak reviewed Feb 13, 2018

View reviewed changes

include/adc.h Outdated

Copy link

Contributor

galak Feb 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason channel_id isn't in adc_channel_cfg?

nashif added the area: API Changes to public APIs label Feb 13, 2018

vaussard reviewed Feb 15, 2018

View reviewed changes

nashif mentioned this pull request Feb 20, 2018

drivers: adc: deprecate adc_enable/adc_disable functions #1167

Closed

MaureenHelm reviewed Mar 27, 2018

View reviewed changes

anangl force-pushed the adc_api_proposal branch from 0fdcbc6 to 3b03667 Compare March 27, 2018 14:11

tbursztyka reviewed Mar 27, 2018

View reviewed changes

tbursztyka reviewed Apr 12, 2018

View reviewed changes

MaureenHelm reviewed Apr 16, 2018

View reviewed changes

anangl force-pushed the adc_api_proposal branch from 3b03667 to e12acbf Compare April 17, 2018 09:25

anangl force-pushed the adc_api_proposal branch from e12acbf to 433bf0b Compare April 17, 2018 12:09

nashif added the DNM This PR should not be merged (Do Not Merge) label Apr 21, 2018

anangl force-pushed the adc_api_proposal branch from 433bf0b to d279047 Compare April 23, 2018 15:25

tests: drivers: Update adc_api test to check the new API

c171e53

Updated API calls and added a few new test cases so that the new possibilities of the API could be presented and checked. Signed-off-by: Andrzej Głąbek <[email protected]>

anangl force-pushed the adc_api_proposal branch from d279047 to c171e53 Compare April 23, 2018 16:35

anangl mentioned this pull request May 21, 2018

Introduce reworked API for ADC drivers #7691

Merged

5 tasks

anangl closed this May 21, 2018

anangl deleted the adc_api_proposal branch September 24, 2018 06:57

patstew mentioned this pull request Mar 15, 2024

Bluetooth: userchan: Support TCP fragmentation of HCI packets #70322

Merged

[RFC][DNM] drivers: adc: new API proposal #6176

[RFC][DNM] drivers: adc: new API proposal #6176

Uh oh!

Conversation

anangl commented Feb 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Feb 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tbursztyka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vaussard commented Feb 15, 2018

Uh oh!

anangl commented Feb 16, 2018

Uh oh!

MaureenHelm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anangl commented Mar 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbursztyka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

anangl commented Feb 13, 2018 •

edited

Loading

codecov-io commented Feb 13, 2018 •

edited

Loading

anangl commented Apr 17, 2018 •

edited

Loading

anangl commented Apr 23, 2018 •

edited

Loading