Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

periph/adc: add adc_sample_multi() #20622

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

benpicco
Copy link
Contributor

Contribution description

This adds an API to sample multiple ADC samples into a into a buffer.
Multiple ADC lines can be sampled in a round-robin fashion or in parallel if the hardware supports it.

Testing procedure

No integration in the test app yet, but I can produce some nice graphs of the sample data with Gnuplot:

two lines on a single ADC

4c00 dat

two lines on different ADCs

4c00 dat

Issues/PRs references

alternative to #20619

@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: drivers Area: Device drivers Area: cpu Area: CPU/MCU ports labels Apr 25, 2024
@benpicco benpicco requested review from kfessel and maribu April 25, 2024 14:04
Comment on lines 136 to 137
* @param[out] bufs target buffer(s)
* @param[in] buf_len number of samples to sample per line
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs to be reworded. I do not understand how many buffers I need and how to specify their size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I re-shuffled the arguments to make the compiler able to check the array sizes, I hope this also makes this easier to understand.

@Teufelchen1
Copy link
Contributor

Have you tried to implement it for another board as well? Just to see if the proposed API is not over-fitted for the SAM (unlikely).

@github-actions github-actions bot added the Area: build system Area: Build system label May 8, 2024
@benpicco
Copy link
Contributor Author

benpicco commented May 8, 2024

@maribu suggested to make the memory layout of the samples implementation defined and provide an accessor function for individual elements.

I'm not a huge fan, since this hurts the ergonomics of the result, but if this turns out to be necessary to reach full performance on some platforms I'm willing to do it - but I'd like to avoid uglifying the API 'just in case'.

@github-actions github-actions bot added the Area: tests Area: tests and testing framework label May 8, 2024
@benpicco benpicco force-pushed the cpu/sam0_common-adc_sample_multi branch from ffd1427 to a6a2fd4 Compare May 15, 2024 16:41
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 15, 2024
@riot-ci
Copy link

riot-ci commented May 15, 2024

Murdock results

✔️ PASSED

d5f1ac2 cpu/sam0_common: make ADC input resistance configurable per channel

Success Failures Total Runtime
10217 0 10218 13m:57s

Artifacts

Comment on lines +146 to +150
void adc_sample_multi(uint8_t lines_numof, const adc_t lines[lines_numof],
size_t num_samples, uint16_t bufs[lines_numof][num_samples],
adc_res_t res, uint32_t f_adc);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

arg, this does not work in C++, how about

Suggested change
void adc_sample_multi(uint8_t lines_numof, const adc_t lines[lines_numof],
size_t num_samples, uint16_t bufs[lines_numof][num_samples],
adc_res_t res, uint32_t f_adc);
#ifdef __cplusplus
void adc_sample_multi(uint8_t lines_numof, const adc_t lines[],
size_t num_samples, uint16_t **bufs,
adc_res_t res, uint32_t f_adc);
#else
void adc_sample_multi(uint8_t lines_numof, const adc_t lines[lines_numof],
size_t num_samples, uint16_t bufs[lines_numof][num_samples],
adc_res_t res, uint32_t f_adc);
#endif

Copy link
Contributor

@kfessel kfessel May 15, 2024

Choose a reason for hiding this comment

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

'uint16_t **bufs and 'bufs[lines_numof][num_samples] seem very dissimilar to me (pointer to pointers of uint16 vs pointer to memory of specific size of uint16 )

Copy link
Contributor

@crasbe crasbe May 15, 2024

Choose a reason for hiding this comment

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

@kfessel uint16_t bufs[lines_numof][numof_samples] actually does not decay into a uint16_t type but into a two dimensional array, as it should. You are correct that if you called a function with f(bufs[lines_numof][numof_samples]), the function would receive an uint16_t, but that is not the case for function declarations.

I did not find any examples that you can specify both array sizes like that in the function declaration, so I am a bit confused that the compiler does not complain about it.
However, this is a very interesting StackOverflow thread that explains the situation quite well: https://stackoverflow.com/questions/8767166/passing-a-2d-array-to-a-c-function

And it also gives a reference to a C FAQ that explicitly says that a pointer to a pointer is not the same as a two dimensional array: https://c-faq.com/aryptr/pass2dary.html
Therefore the proposed fix is not a valid solution.

The applicable solution for the issue would be approach 2 from stackoverflow with the corresponding call:

void adc_sample_multi(uint8_t lines_numof, const adc_t lines[lines_numof],
                      size_t num_samples, uint16_t (*bufs)[lines_numof][num_samples],
                      adc_res_t res, uint32_t f_adc);

...

adc_sample_multi(..., &samples, ...);

HOWEVER I don't know if the C compiler allows the dynamic boundaries. Perhaps it would require const size_t lines_numof and numof_samples or even LINES_NUMOF/NUMOF_SAMPLES as preprocessor defines, so it definitively known at compile time.
The comment says that the array size has to be fixed and known at compile time, so it probably won't compile.


(On a side note: IMO it would be better to have a consistent naming like num_samples and num_lines or samples_numof and lines_numof).

Copy link
Contributor

Choose a reason for hiding this comment

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

thank you @crasbe for providing more hints and looking into that.

For me my comment was more about the very dissimilar and the c++ header variant not having a compatible interface to the c implementation if written that way, it was more about putting the message out fast (before any merge happens).

A simple solution might also be to hide the function declaration entirely from C++ instead of making it less kiss for C.

@benpicco benpicco force-pushed the cpu/sam0_common-adc_sample_multi branch from a6a2fd4 to 03930cc Compare May 21, 2024 11:13
@benpicco benpicco force-pushed the cpu/sam0_common-adc_sample_multi branch 2 times, most recently from a6c3f85 to e1027df Compare July 11, 2024 16:19
@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 26, 2024
@benpicco benpicco force-pushed the cpu/sam0_common-adc_sample_multi branch from e1027df to 7507668 Compare July 26, 2024 14:00
@benpicco benpicco force-pushed the cpu/sam0_common-adc_sample_multi branch from 7507668 to 5f544cb Compare August 1, 2024 13:44
@benpicco benpicco requested a review from biboc as a code owner October 10, 2024 09:53
@github-actions github-actions bot added the Area: boards Area: Board ports label Oct 10, 2024
@Teufelchen1
Copy link
Contributor

@benpicco is this ready for another review?

@benpicco benpicco force-pushed the cpu/sam0_common-adc_sample_multi branch 2 times, most recently from 8391cb2 to 0869866 Compare October 16, 2024 15:27
@benpicco benpicco force-pushed the cpu/sam0_common-adc_sample_multi branch from 0869866 to d5f1ac2 Compare October 28, 2024 13:16
@github-actions github-actions bot removed the Area: boards Area: Board ports label Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants