Confusion about Zephyr ADC API semantics


Marti Bolivar
 

Hi,

I've been reading through the ADC API, its users, and its test cases,
and I'm confused about the semantics of struct adc_seq_entry, in
particular the field named "buffer". The API docs are vague and the
users contradict each other; something seems wrong to me.

(I volunteer to try to improve the documentation if we can clear
things up; I'm working on an ADC driver and would like to make sure
I'm doing the right thing.)

The main header says "buffer" is a byte array:

https://github.com/zephyrproject-rtos/zephyr/blob/master/include/adc.h#L39

But it doesn't say anything about the contents. That's strange,
especially since common ADC IPs can do 12+ bit conversions. (I at
least expected a u16*, and something written about the left- or
right-alignment of sample data, e.g. how a 12 bit sample is stored in
a 16 bit word.)

That same header only has this to say about the returned values from
an adc_read() call:

* The sample data can be retrieved from the memory buffers in
* the sequence table structure.

So I looked at the API users to find out more, but the results were
even more confusing.

This "simple" test wants to interpret the results as though the buffer
field points at an array of u32 (note the _print_sample_in_hex
implementation and the "delta" printk in adc_test):

https://github.com/zephyrproject-rtos/zephyr/blob/master/tests/drivers/adc/adc_simple/src/main.c#L36

It also says buffer should be void*, which isn't true:

https://github.com/zephyrproject-rtos/zephyr/blob/master/tests/drivers/adc/adc_simple/src/main.c#L78

This "api" test thinks buffer is u16*:

https://github.com/zephyrproject-rtos/zephyr/blob/master/tests/drivers/adc/adc_api/src/test_adc.c#L53

The ADC-based grove temperature driver treats Zephyr samples as if
they were 12 bit right aligned values in a u16 array, and has a
comment saying that's the common convention:

https://github.com/zephyrproject-rtos/zephyr/blob/master/drivers/grove/temperature_sensor.c#L46

Can anyone clarify what the correct interpretation of this field is?

Thanks,
Marti


Marti Bolivar
 

Hello again,

Since about a week has passed by with no response, I assume that the
ADC infrastructure is currently unmaintained (or the maintainers are
on vacation), and will do my best to fix the core semantics as best I
can. I will also make a best effort to contact authors of existing ADC
drivers if I need help as regards to changes that might impact those.

I hope and expect to submit some PRs around this in the first week of
2018, now that I've spent a few days hacking on an nRF SAADC driver
and feel I understand the issues a bit better.

Please let me know if you'd like to help!

Thanks,
Marti

On Fri, Dec 15, 2017 at 9:00 PM, Marti Bolivar
<marti@opensourcefoundries.com> wrote:
Hi,

I've been reading through the ADC API, its users, and its test cases,
and I'm confused about the semantics of struct adc_seq_entry, in
particular the field named "buffer". The API docs are vague and the
users contradict each other; something seems wrong to me.

(I volunteer to try to improve the documentation if we can clear
things up; I'm working on an ADC driver and would like to make sure
I'm doing the right thing.)

The main header says "buffer" is a byte array:

https://github.com/zephyrproject-rtos/zephyr/blob/master/include/adc.h#L39

But it doesn't say anything about the contents. That's strange,
especially since common ADC IPs can do 12+ bit conversions. (I at
least expected a u16*, and something written about the left- or
right-alignment of sample data, e.g. how a 12 bit sample is stored in
a 16 bit word.)

That same header only has this to say about the returned values from
an adc_read() call:

* The sample data can be retrieved from the memory buffers in
* the sequence table structure.

So I looked at the API users to find out more, but the results were
even more confusing.

This "simple" test wants to interpret the results as though the buffer
field points at an array of u32 (note the _print_sample_in_hex
implementation and the "delta" printk in adc_test):

https://github.com/zephyrproject-rtos/zephyr/blob/master/tests/drivers/adc/adc_simple/src/main.c#L36

It also says buffer should be void*, which isn't true:

https://github.com/zephyrproject-rtos/zephyr/blob/master/tests/drivers/adc/adc_simple/src/main.c#L78

This "api" test thinks buffer is u16*:

https://github.com/zephyrproject-rtos/zephyr/blob/master/tests/drivers/adc/adc_api/src/test_adc.c#L53

The ADC-based grove temperature driver treats Zephyr samples as if
they were 12 bit right aligned values in a u16 array, and has a
comment saying that's the common convention:

https://github.com/zephyrproject-rtos/zephyr/blob/master/drivers/grove/temperature_sensor.c#L46

Can anyone clarify what the correct interpretation of this field is?

Thanks,
Marti


Jonas Pfaff
 

Hi Marti,

I understand your confusion as I had the same problem a few months ago.
I implemented the sam_afec driver using a 16bit array containing right
aligned 12bit values (can go up to 16bit depending on the ADC config)
because it seemed to be the most plausible and straightforward solution
in my use case.
I am curious to see your solution and would be happy to help where I
can.

Regards,
Jonas

Hello again,

Since about a week has passed by with no response, I assume that the
ADC infrastructure is currently unmaintained (or the maintainers are
on vacation), and will do my best to fix the core semantics as best I
can. I will also make a best effort to contact authors of existing ADC
drivers if I need help as regards to changes that might impact those.

I hope and expect to submit some PRs around this in the first week of
2018, now that I've spent a few days hacking on an nRF SAADC driver
and feel I understand the issues a bit better.

Please let me know if you'd like to help!

Thanks,
Marti

On Fri, Dec 15, 2017 at 9:00 PM, Marti Bolivar
<marti@opensourcefoundries.com> wrote:
Hi,

I've been reading through the ADC API, its users, and its test cases,
and I'm confused about the semantics of struct adc_seq_entry, in
particular the field named "buffer". The API docs are vague and the
users contradict each other; something seems wrong to me.

(I volunteer to try to improve the documentation if we can clear
things up; I'm working on an ADC driver and would like to make sure
I'm doing the right thing.)

The main header says "buffer" is a byte array:

https://github.com/zephyrproject-rtos/zephyr/blob/master/include/adc.h#L39

But it doesn't say anything about the contents. That's strange,
especially since common ADC IPs can do 12+ bit conversions. (I at
least expected a u16*, and something written about the left- or
right-alignment of sample data, e.g. how a 12 bit sample is stored in
a 16 bit word.)

That same header only has this to say about the returned values from
an adc_read() call:

* The sample data can be retrieved from the memory buffers in
* the sequence table structure.

So I looked at the API users to find out more, but the results were
even more confusing.

This "simple" test wants to interpret the results as though the buffer
field points at an array of u32 (note the _print_sample_in_hex
implementation and the "delta" printk in adc_test):

https://github.com/zephyrproject-rtos/zephyr/blob/master/tests/drivers/adc/adc_simple/src/main.c#L36

It also says buffer should be void*, which isn't true:

https://github.com/zephyrproject-rtos/zephyr/blob/master/tests/drivers/adc/adc_simple/src/main.c#L78

This "api" test thinks buffer is u16*:

https://github.com/zephyrproject-rtos/zephyr/blob/master/tests/drivers/adc/adc_api/src/test_adc.c#L53

The ADC-based grove temperature driver treats Zephyr samples as if
they were 12 bit right aligned values in a u16 array, and has a
comment saying that's the common convention:

https://github.com/zephyrproject-rtos/zephyr/blob/master/drivers/grove/temperature_sensor.c#L46

Can anyone clarify what the correct interpretation of this field is?

Thanks,
Marti


Marti Bolivar
 

Hi Jonas,

Thanks for your response! I hope you don't mind I've added back the
users list as well.

I am leaving for holiday break at end of day and so likely won't have
more to say until the new year, but I will be sure to handle 12-bit
right aligned values in a u16_t array in anything I propose for the
core API.

FWIW, STM32 ADCs also support 12 bit conversion with configurable alignment.

Best,
Marti

On Thu, Dec 21, 2017 at 2:24 AM, Jonas Pfaff <jonas.pfaff@comsuisse.ch> wrote:
Hi Marti,

I understand your confusion as I had the same problem a few months ago.
I implemented the sam_afec driver using a 16bit array containing right
aligned 12bit values (can go up to 16bit depending on the ADC config)
because it seemed to be the most plausible and straightforward solution
in my use case.
I am curious to see your solution and would be happy to help where I
can.

Regards,
Jonas

Hello again,

Since about a week has passed by with no response, I assume that the
ADC infrastructure is currently unmaintained (or the maintainers are
on vacation), and will do my best to fix the core semantics as best I
can. I will also make a best effort to contact authors of existing ADC
drivers if I need help as regards to changes that might impact those.

I hope and expect to submit some PRs around this in the first week of
2018, now that I've spent a few days hacking on an nRF SAADC driver
and feel I understand the issues a bit better.

Please let me know if you'd like to help!

Thanks,
Marti

On Fri, Dec 15, 2017 at 9:00 PM, Marti Bolivar
<marti@opensourcefoundries.com> wrote:
Hi,

I've been reading through the ADC API, its users, and its test cases,
and I'm confused about the semantics of struct adc_seq_entry, in
particular the field named "buffer". The API docs are vague and the
users contradict each other; something seems wrong to me.

(I volunteer to try to improve the documentation if we can clear
things up; I'm working on an ADC driver and would like to make sure
I'm doing the right thing.)

The main header says "buffer" is a byte array:

https://github.com/zephyrproject-rtos/zephyr/blob/master/include/adc.h#L39

But it doesn't say anything about the contents. That's strange,
especially since common ADC IPs can do 12+ bit conversions. (I at
least expected a u16*, and something written about the left- or
right-alignment of sample data, e.g. how a 12 bit sample is stored in
a 16 bit word.)

That same header only has this to say about the returned values from
an adc_read() call:

* The sample data can be retrieved from the memory buffers in
* the sequence table structure.

So I looked at the API users to find out more, but the results were
even more confusing.

This "simple" test wants to interpret the results as though the buffer
field points at an array of u32 (note the _print_sample_in_hex
implementation and the "delta" printk in adc_test):

https://github.com/zephyrproject-rtos/zephyr/blob/master/tests/drivers/adc/adc_simple/src/main.c#L36

It also says buffer should be void*, which isn't true:

https://github.com/zephyrproject-rtos/zephyr/blob/master/tests/drivers/adc/adc_simple/src/main.c#L78

This "api" test thinks buffer is u16*:

https://github.com/zephyrproject-rtos/zephyr/blob/master/tests/drivers/adc/adc_api/src/test_adc.c#L53

The ADC-based grove temperature driver treats Zephyr samples as if
they were 12 bit right aligned values in a u16 array, and has a
comment saying that's the common convention:

https://github.com/zephyrproject-rtos/zephyr/blob/master/drivers/grove/temperature_sensor.c#L46

Can anyone clarify what the correct interpretation of this field is?

Thanks,
Marti


Piotr Mienkowski
 

Hi Marti,

Sorry for a late reply. I would like to add a few points to the
discussion. At first I'll quote your email as you wrote it a while ago.

On 16.12.2017 03:00, Marti Bolivar wrote:
I've been reading through the ADC API, its users, and its test cases,
and I'm confused about the semantics of struct adc_seq_entry, in
particular the field named "buffer". The API docs are vague and the
users contradict each other; something seems wrong to me.

(I volunteer to try to improve the documentation if we can clear
things up; I'm working on an ADC driver and would like to make sure
I'm doing the right thing.)

The main header says "buffer" is a byte array:

https://github.com/zephyrproject-rtos/zephyr/blob/master/include/adc.h#L39

But it doesn't say anything about the contents. That's strange,
especially since common ADC IPs can do 12+ bit conversions. (I at
least expected a u16*, and something written about the left- or
right-alignment of sample data, e.g. how a 12 bit sample is stored in
a 16 bit word.)

That same header only has this to say about the returned values from
an adc_read() call:

* The sample data can be retrieved from the memory buffers in
* the sequence table structure.

So I looked at the API users to find out more, but the results were
even more confusing.

This "simple" test wants to interpret the results as though the buffer
field points at an array of u32 (note the _print_sample_in_hex
implementation and the "delta" printk in adc_test):

https://github.com/zephyrproject-rtos/zephyr/blob/master/tests/drivers/adc/adc_simple/src/main.c#L36

It also says buffer should be void*, which isn't true:

https://github.com/zephyrproject-rtos/zephyr/blob/master/tests/drivers/adc/adc_simple/src/main.c#L78

This "api" test thinks buffer is u16*:

https://github.com/zephyrproject-rtos/zephyr/blob/master/tests/drivers/adc/adc_api/src/test_adc.c#L53

The ADC-based grove temperature driver treats Zephyr samples as if
they were 12 bit right aligned values in a u16 array, and has a
comment saying that's the common convention:

https://github.com/zephyrproject-rtos/zephyr/blob/master/drivers/grove/temperature_sensor.c#L46

Can anyone clarify what the correct interpretation of this field is?
The alignment of the data within the buffer is unfortunately not the
only problem with this API. Also the meaning of the 'buffer_length'
field is pretty mysterious. I will not go into the details, suffice to
say that every single adc driver (we have a few of them already)
understands and implements it differently.

The fact that the implementation of ADC API among existing drivers is
incompatible is actually the smaller of the issues. The bigger problem
is that this API does not support typical use cases in a straightforward
way and neither takes into account capabilities of common hardware. IMHO
improving the documentation will not help much here. We should better
deprecate this API and come up with a new design.

Regards,
Piotr


Marti Bolivar
 

Hi everyone,

Thanks to everybody who responded.

Unfortunately due to some priority changes at my company, I'm not able to continue this work as much as I had originally thought. I've heard from some people who are interested in using an nRF SAADC driver, even out of tree. I have something hackish up here:


I also spoke on IRC with Giuliano Franchetto, who pointed me at this driver:


I think these could be a good starting point for people who need something now, and hopefully this can be taken up again in the future.

Thanks,
Marti



On Sun, Jan 7, 2018 at 6:55 PM, Piotr Mienkowski <piotr.mienkowski@...> wrote:
Hi Marti,

Sorry for a late reply. I would like to add a few points to the
discussion. At first I'll quote your email as you wrote it a while ago.

On 16.12.2017 03:00, Marti Bolivar wrote:
> I've been reading through the ADC API, its users, and its test cases,
> and I'm confused about the semantics of struct adc_seq_entry, in
> particular the field named "buffer". The API docs are vague and the
> users contradict each other; something seems wrong to me.
>
> (I volunteer to try to improve the documentation if we can clear
> things up; I'm working on an ADC driver and would like to make sure
> I'm doing the right thing.)
>
> The main header says "buffer" is a byte array:
>
> https://github.com/zephyrproject-rtos/zephyr/blob/master/include/adc.h#L39
>
> But it doesn't say anything about the contents. That's strange,
> especially since common ADC IPs can do 12+ bit conversions. (I at
> least expected a u16*, and something written about the left- or
> right-alignment of sample data, e.g. how a 12 bit sample is stored in
> a 16 bit word.)
>
> That same header only has this to say about the returned values from
> an adc_read() call:
>
> * The sample data can be retrieved from the memory buffers in
> * the sequence table structure.
>
> So I looked at the API users to find out more, but the results were
> even more confusing.
>
> This "simple" test wants to interpret the results as though the buffer
> field points at an array of u32 (note the _print_sample_in_hex
> implementation and the "delta" printk in adc_test):
>
> https://github.com/zephyrproject-rtos/zephyr/blob/master/tests/drivers/adc/adc_simple/src/main.c#L36
>
> It also says buffer should be void*, which isn't true:
>
> https://github.com/zephyrproject-rtos/zephyr/blob/master/tests/drivers/adc/adc_simple/src/main.c#L78
>
> This "api" test thinks buffer is u16*:
>
> https://github.com/zephyrproject-rtos/zephyr/blob/master/tests/drivers/adc/adc_api/src/test_adc.c#L53
>
> The ADC-based grove temperature driver treats Zephyr samples as if
> they were 12 bit right aligned values in a u16 array, and has a
> comment saying that's the common convention:
>
> https://github.com/zephyrproject-rtos/zephyr/blob/master/drivers/grove/temperature_sensor.c#L46
>
> Can anyone clarify what the correct interpretation of this field is?
>

The alignment of the data within the buffer is unfortunately not the
only problem with this API. Also the meaning of the 'buffer_length'
field is pretty mysterious. I will not go into the details, suffice to
say that every single adc driver (we have a few of them already)
understands and implements it differently.

The fact that the implementation of ADC API among existing drivers is
incompatible is actually the smaller of the issues. The bigger problem
is that this API does not support typical use cases in a straightforward
way and neither takes into account capabilities of common hardware. IMHO
improving the documentation will not help much here. We should better
deprecate this API and come up with a new design.

Regards,
Piotr
_______________________________________________
Zephyr-devel mailing list
Zephyr-devel@lists.zephyrproject.org
https://lists.zephyrproject.org/mailman/listinfo/zephyr-devel