RFC: Expand ADC API to support batch_mode sampling

[heinwessels]

Introduction

This RFC entails expanding the ADC API with support for batch_sampling sampling. This will allow a driver to, for example, sample an ADC continiously using a DMA and a circular buffer without ever triggering an interrupt a large amount of samples continiously using extra_samples and then only generate a single interrupt. This allows a driver to optionally offload more processing to the silicon if possible, f.i. using a DMA.

Initially mentioned in #39640. I'm creating a new issue as that discussion seems to be more driver specific and includes DMA changes. This RFC is purely for the ADC API.

Problem description

In time constraint scenarios where the MCU needs to take high frequency ADC readings it's benificial to offload as much processing to the silicon as possible, instead of handling the data through code and interrupts.

The ideal solution is to allow the ADC to sample continously, and the DMA continuously places new samples into a circular buffer, without the need of interrupts or polling. The sampling is completely handled by the silicon. This will be hard to implement with the current infrastructure, but a balance can be found by allow the DMA to sample 1 + extra_samples samples continuously, all with a DMA if possible, and only raise a single callback after it's done.

In our use-case we have a STM32H7 that needs to sample 16 channels at 100Hz on two ADCs. The addition of sampling multiple channels (#53534) along with DMA support (#52965) already improves this. But it's still interrupt based and creates way too many interrupts, about 200 per second, which is causing many stability issues. This proposed change would allow us to expand the ADC driver to remove all these interrupts, except one, without reducing the amount of samples.

This additional will cause minimal change to existing drivers.

Proposed change

Expand the existing ADC API to support continous sampling. The actual implementation will be driver dependent and is not part of this RFC.

Detailed RFC

Proposed change (Detailed)

---

Edit: Updated proposed solution as mentioned in #55783 (comment).

/**
 * Sets ADC reading in batch_mode mode
 * The driver will complete reading of all channels and sample required extra_samples
 * in one shot, and only afterwards call the callback.
 * The driver will attempt to offload as processing to the silicon as possible.
 * For example, if coupled with DMA, this entire read sequence can be completed
 * without any interrupts, meaning context changes, except for when it's completed.
 */
 bool batch_mode;

This can then be called in sync or async mode as the normal read, and will act accordingly. The adc_context will have to modified slightly to not call callbacks until the all samples has been taken if this option is enabled. The actual implementation of this is ofloaded to the driver, and can function without DMA to remain portable.

When implemented in STM32 with DMA: The DMA will not be set in circular buffer mode, but rather have a buffer of length 1 + extra_samples, and attempt to fill it. It will only raise an ISR when this buffer is full, which will then call adc_context, which will raise the callback.

Alternative naming suggestion of bool one_shot, but I prefer continuous.

Pros:

• The buffer will be safe to use after a read until the user explicitly starts a new read.
• Devices with different ADCs of varying capability can handle each case correctly internally.
• Portable, as the driver can choose to implement it in any way.
• Existing drivers will still function normally, as the main change is made in adc_context. A driver can optionally add additional functionality to offload more processing to the silicon.

Cons:

---

First, the API needs a way to start a continuous read. My first thought was to expand adc_sequence_options with a bool continuous variable. However, this might not be a good solution, because then it can theorically be used with both adc_read and adc_read_async, which would not make sense. Continuous mode would always be async. It will also mean the bool can be set on a driver which does not implement it yet, without letting the user know.

A better solution would be to a dedicated function:

/**
 * @brief Start a continuous ADC read.
 *
 * @note This function is available only if @kconfig{CONFIG_ADC_CONTINUOUS}
 * is selected.
 *
 * @note This function does not support callbacks, or a given interval.
 *
 * @param dev       Pointer to the device structure for the driver instance.
 * @param sequence  Structure specifying requested sequence of samplings.
 *
 * @returns 0 on success, negative error code otherwise.
 *          See adc_read() for a list of possible error codes.
 *
 */
__syscall int adc_read_continuous(const struct device *dev,
			     const struct adc_sequence *sequence);


#ifdef CONFIG_ADC_CONTINUOUS
static inline int z_impl_adc_read_continuous(const struct device *dev,
					const struct adc_sequence *sequence)
{
	const struct adc_driver_api *api =
				(const struct adc_driver_api *)dev->api;

	return api->read_continuous(dev, sequence);
}
#endif /* CONFIG_ADC_CONTINUOUS */

This is better because:

• It which will follow the convention used by existing and similar adc_read_async.
• This means that if the function is called on a driver which it's not implemented on yet it will cause a linker error, instead of undefined behaviour.
• The execution of a continiuous read will be vary significantly from adc_read and adc_read_async, because it won't need adc_context, which means a seperate function will be more intuitive.

This means a new kconfig value will be introduced, something this is adc/kconfig:

config ADC_CONTINUOUS
	bool "Continuous sampling support"
	help
	  This option enables the continuous ADC sampling.

The driver would also need some way to stop the continuous sampling.

__syscall int adc_stop_continuous_read(const struct device *dev);


#ifdef CONFIG_ADC_CONTINUOUS
static inline int z_impl_adc_stop_continuous_read(const struct device *dev)
{
	const struct adc_driver_api *api =
				(const struct adc_driver_api *)dev->api;

	return api->stop_continuous_read(dev);
}
#endif /* CONFIG_ADC_CONTINUOUS */

The circular buffer size can be set to hold 1 + extra_samples samples. It's up to the caller to provide this buffer through the existing adc_sequence.buffer.

The adc_context does not have to be changed, because it will not be used. It's main purpose is managing callbacks and the order of samples. This RFC does not use callbacks, and the sampling is explicitly handled by the silicon.

Dependencies

This change will not affect existing drivers. Drivers will need to add explicit support for it to function.

Concerns and Unresolved Questions

• Should CONFIG_ADC_CONTINUOUS depend on CONFIG_DMA? Are there boards where DMA is not required for continuous sampling? Ideally this should be board specific, but I don't see how a board specific kconfig value could force CONFIG_ADC_CONTINUOUS=n if CONFIG_DMA=n for example.
• The adc_stop_continuous_read could be be made more generic, as in adc_stop_read and possibly allow stopping an ongoing adc_read_async. However, I do not think this is the right direction.
• In STM32: Add DMA+ADC support #39640 they wanted to add a callback on DMA buffer half-full, and this RFC currently doesn't include supporting callbacks. In our opinion it would defeat the purpose of ofloading the ADC sampling to the silicon to prevent unnecessary interrupts. And adding the ability for optional callbacks would bring in unnecessary complexity, and the user might use it without realizing the effect it might have on performance.
• While the user is reading the circular buffer then interrupts should be disabled using irq_lock() to ensure the data remains unchanged while being accessed. This may lead to unexpected behaviour when not used correctly. IMO we can leave this caveat as-is, however I would love to hear if there is a better way to do it.

Alternatives

No alternatives were considered.

[xyzzy42]

Continuous mode would always be async.

Does that really need to be the case? It seems like starting a non-async continuous sampling should work fine. It blocks forever, or until there is an error.

I've written code that does this now, with the existing API, by having the callback always return ADC_ACTION_REPEAT. The callback handles processing the data. The thread that called adc_read blocks until there is an error, in which case it handles it. If it was written to use async read, it would do the same thing, just in a more complex way.

This means that if the function is called on a driver which it's not implemented on yet it will cause a linker error, instead of undefined behaviour.

Couple things here. Firstly, the example code won't cause a linker error. It'll call the function pointer in the api struct, and that pointer is NULL, and it will crash. What you want to do is check if the pointer is NULL first and return ENOSYS if it is, otherwise call the method. This is how other optional api methods work.

Secondly, can drivers not return EINVAL or ENOTSUP for adc_sequence_options they do not support? I see plenty of drivers do this. So if a driver does not support continuous sampling, it can return an error.

Should CONFIG_ADC_CONTINUOUS depend on CONFIG_DMA?

I see no reason why one needs DMA. In fact, the adc_context.h code could support continuous mode in a way that would make it work with most existing drivers without needing to modify the driver.

Consider app portability. I write code that uses a circular buffer and continuous sampling because that makes the most sense. Then I want to run that code on another board that doesn't have DMA or needs the channels for something else. Now the app needs to write a totally new block of code to run the ADC in a different mode without the circular buffer.

this RFC currently doesn't include supporting callbacks

How does one know what data in the circular buffer is new and what is not?

I think it in now way defeats the purpose of DMA to have a callback. The callback is no longer every sample, but every many samples. The interrupt load is decreased by orders of magnitude. Also consider, if you have no callback, how do you process the data? Have something on a timer that periodically reads the circular buffer? And what causes this timer to fire? Is it an interrupt? So there are still interrupts, just timer interrupts instead of DMA interrupts.

[heinwessels]

@xyzzy42 just to be clear, this RFC is not about using adc_context to mimic continuous sampling. This is of course possible, and what we are currently doing anyway. The problem with this, even if there are no callbacks, is that it still creates interrupts which should be handled by adc_context, which creates context switches. Do this at high frequencies, especially on smaller MCU's, and it will lock up your MCU as it constantly switches between ISRs and other code execution.

This RFC is purely to be able to access some MCU's ability to continously sample an ADC completely in hardware, the physical silicon, without the need for maintaining it using interrupts. This is typically achieved with a DMA. As mentioned, this is very useful, and often used, in high frequency sampling.

The only compromise I'm willing to make on this is to create one interrupt after the buffer is full, with one callback, if it's possible. And in that case it might make more sense to add a bool continuous to adc_sequence.

Continuous mode would always be async.

Does that really need to be the case? It seems like starting a non-async continuous sampling should work fine. It blocks forever, or until there is an error.

I've written code that does this now, with the existing API, by having the callback always return ADC_ACTION_REPEAT. The callback handles processing the data. The thread that called adc_read blocks until there is an error, in which case it handles it. If it was written to use async read, it would do the same thing, just in a more complex way.

Depends on what we decide on what continuous means. I

f we decide that a single interrupt should be, then it would make more sense to be part of adc_sequence, which in that case would mean that it could sync or async, yes.

This means that if the function is called on a driver which it's not implemented on yet it will cause a linker error, instead of undefined behaviour.

Couple things here. Firstly, the example code won't cause a linker error. It'll call the function pointer in the api struct, and that pointer is NULL, and it will crash. What you want to do is check if the pointer is NULL first and return ENOSYS if it is, otherwise call the method. This is how other optional api methods work.

Secondly, can drivers not return EINVAL or ENOTSUP for adc_sequence_options they do not support? I see plenty of drivers do this. So if a driver does not support continuous sampling, it can return an error.

It looks to me like it would cause a linker header. If CONFIG_ADC_CONTINUOUS=n then the prototype would be defined, but no implementation.

Regardless, relying on the driver to return ENOTSUP isn't very scalable or modular, because then every driver needs now needs to explicitly state that it doesn't support continuous. And each time a new driver is added the contributor needs to remember to add the ENOTSUP. Much better to offload that check to adc.h where it's generic, as mentioned.

And again, depends on how we define a continuous read, where it would make most sense.

Should CONFIG_ADC_CONTINUOUS depend on CONFIG_DMA?

I see no reason why one needs DMA. In fact, the adc_context.h code could support continuous mode in a way that would make it work with most existing drivers without needing to modify the driver.

Consider app portability. I write code that uses a circular buffer and continuous sampling because that makes the most sense. Then I want to run that code on another board that doesn't have DMA or needs the channels for something else. Now the app needs to write a totally new block of code to run the ADC in a different mode without the circular buffer.

As mentioned earlier in this comment, this RFC is specifically to (in the STM32 at least) offload the sampling procedure in it's entirety to the hardware. Relying on adc_context with ADC_ACTION_REPEAT does defeat this purpose completely. Therefore, in the case of STM32 it needs DMA.

The only reason I asked this, is because I don't know how other boards would implement this, or if it's possible. So if you have knowlegde of that, please share.

this RFC currently doesn't include supporting callbacks

How does one know what data in the circular buffer is new and what is not?

I think it in now way defeats the purpose of DMA to have a callback. The callback is no longer every sample, but every many samples. The interrupt load is decreased by orders of magnitude. Also consider, if you have no callback, how do you process the data? Have something on a timer that periodically reads the circular buffer? And what causes this timer to fire? Is it an interrupt? So there are still interrupts, just timer interrupts instead of DMA interrupts.

Adding a single interrupt/callback when the entire buffer is full might be good enough and a good idea, but IMO it's not really needed. It would also make the logic simpler I guess. I'm willing to go in that direction.

As to when you would read the buffer, well, whenever. If it's every second, maybe it's done with timer, maybe it's not, but importantly it's up to the user to decide.

[xyzzy42]

just to be clear, this RFC is not about using adc_context to mimic continuous sampling. This is of course possible, and what we are currently doing anyway. The problem with this, even if there are no callbacks, is that it still creates interrupts which should be handled by adc_context, which creates context switches.

Having a product with 4 channel 200 Hz per channel ADC that wants to go to 10 channels at 200 Hz per channel, I understand this fully. If anything I said appeared to be about only supporting emulation of proper continuous sampling with per-sample interrupts or per-sample callbacks, this was not my intention.

It looks to me like it would cause a linker header. If CONFIG_ADC_CONTINUOUS=n then the prototype would be defined, but no implementation.

Consider if there are two different ADCs. Everyone with an external ADC on SPI/I2C will almost certainly have also an internal ADC in the MCU, and thus different ADCs. So this is not strange at all, but very common. The configuration option is global. Just because it is on doesn't mean all ADCs (or even any ADCs, but then why turn it on?) have support for continuous sampling.

It is the same as say CONFIG_UART_LINE_CTRL. Just because this option is turned on, doesn't mean any given UART driver actually supports api->line_ctrl_set. Most don't. So it's necessary to check if the driver supplies that API method and return -ENOSYS if not, exactly as the Zephyr UART API code does. You should see most of the Zephyr "optional" driver APIs work this way. Those that don't, IMHO, are bugs. I'm looking at you adc_read_async....

Consider app portability. I write code that uses a circular buffer and continuous sampling because that makes the most sense. Then I want to run that code on another board that doesn't have DMA or needs the channels for something else. Now the app needs to write a totally new block of code to run the ADC in a different mode without the circular buffer.
As mentioned earlier in this comment, this RFC is specifically to (in the STM32 at least) offload the sampling procedure in it's entirety to the hardware. Relying on adc_context with ADC_ACTION_REPEAT does defeat this purpose completely.

I think you're missing the point. It's nothing to do with callbacks and repeat. It's about making a useful API extension. If the API can support only one precise use case and one specific vendor HAL's method of operation, then it's not as useful as an API.

A Zephyr user wants to sampling continuously from an ADC into a circular buffer, and do it efficiently. This is surely a common design. So they use this API. Of course this should DMA when it can. But suppose someone wants to run this app on hardware where they can't use DMA? What is better: a) Too bad, you need DMA, write another app for DMA-less hardware. Or b), the API keeps working without DMA! It's less efficient on this hardware, sure, but the app still works without needing to be re-written or even modified.

I think b) is a totally achievable goal. It should be possible extend the code in adc_context.h that most ADC drivers use continuous sampling, even if the driver itself can't support it natively.

The only reason I asked this, is because I don't know how other boards would implement this, or if it's possible. So if you have knowlegde of that, please share.

If you want to see how to implement continuous DMA is a portable way that experience has proven successfull, look to the Linux ALSA API. This supports DMA efficiently on a huge variety of hardware, from decades old ISA soundcards to every phone application processor, while presenting a consistent API to user code that doesn't need to know details about the hardware it runs on.

It is however probably too complex for Zephyr.

But the basic concepts of buffer size, fragment size, application pointer should still work. The constraints system should be made much much simpler.

As to when you would read the buffer, well, whenever. If it's every second, maybe it's done with timer, maybe it's not, but importantly it's up to the user to decide.

But how can you know what to read from the buffer? You've got to know where the "head" pointer is currently at, where it was last time you read, and if it wrapped around more than once. Obviously the code doing this periodic reading can keep track of its last head pointer, but how does it get the current pointer? How does it detect multiple wraps of the pointer?

The answer to detecting wraps is that you must interrupt and check the pointer location more often than it takes to DMA one entire circular buffer. Making the interval between checks triggered on the samples of data transfered is very a easy way to do this!

Some DMA controllers don't give you a register with some kind of current position pointer you can always read from. They process data in fragments and interrupt after each fragment is done. You have no idea where DMA is between fragments and if you want to know where the DMA pointer is, you must count fragments in software.

This is where the two interrupts per circular buffer come from. It's the minimum number to make it work on this type of hardware. If you think about one interrupt per buffer, you'll see it is fatally flawed in that upon being notified that DMA has completed one buffer and being able to start processing the buffer, DMA will also be overwriting the buffer from the beginning!

[heinwessels]

@xyzzy42 valid points. Then I propose we change the approach to be an option, like this

/**
 * Sets ADC reading in continuous mode
 * The driver will complete reading of all channels and sample required extra_samples
 * in one shot, and only afterwards call the callback.
 * The driver will attempt to offload as processing to the silicon as possible.
 * For example, if coupled with DMA, this entire read sequence can be completed
 * without any interrupts, meaning context changes, except for when it's completed.
 */
 bool continuous;

This can then be called in sync or async mode as the normal read, and will act accordingly. The adc_context will have to modified slightly to not call callbacks until the all samples has been taken if this option is enabled. The actual implementation of this is ofloaded to the driver, and can function without DMA to remain portable.

When implemented in STM32 with DMA: The DMA will not be set in circular buffer mode, but rather have a buffer of length 1 + extra_samples, and attempt to fill it. It will only raise an ISR when this buffer is full, which will then call adc_context, which will raise the callback.

Alternative naming suggestion of bool one_shot, but I prefer continuous.

Pros:

• The buffer will be safe to use after a read until the user explicitly starts a new read.
• Devices with different ADCs of varying capability can handle each case correctly internally.
• Portable, as the driver can choose to implement it in any way.

Cons:

• Existing drivers will silently ignore this option if not handled explicitly.

[heinwessels]

FYI @erwango @anangl

[hakehuang]

NXP adc driver already has a continues conversion mode, which is not the same meaning for the RFC, but to avoid confusion, maybe change the name form continues to batch_mode be more preferred.

[heinwessels]

NXP adc driver already has a continues conversion mode, which is not the same meaning for the RFC, but to avoid confusion, maybe change the name form continues to batch_mode be more preferred.

Interesting. It's specified in the ADC node, which I guess is also an option.

zephyr/dts/bindings/adc/nxp,kinetis-adc16.yaml

Lines 43 to 45 in b6c6de3

	continuous-convert:
	type: boolean
	description: If use continuous convert

Although, IMO would be more understandable and maintainable if it was an generic API, and not something each driver implements in it's own way.

[hakehuang]

@heinwessels , NXP hardware has a continue_convert feature which is not related to your proposal, so to avoid confusion, maybe change 'continues' to 'batch_mode/

[tagunil]

The problem with batch mode for STM32 specifically is that not all STM32 series support one-shot (aka limited) mode. E.g. one cannot just stop continuous mode on F1/F2/F4/F7 without disabling the entire peripheral. And, of course, it's not exactly a replacement for a proper continuous mode, as it introduces jitter between batches. Apart from that, this feature looks like a useful addition, and I'm already debugging some rough implementation for STM32 on my STM32L4 board.