Skip to content

arch/arm/src/stm32hx/stm32_adc: Reset channel counter before conversions.#19013

Merged
xiaoxiang781216 merged 2 commits into
apache:masterfrom
sammytranGeo:stm32hx/reset-channel-on-trigger-ioctl
Jun 2, 2026
Merged

arch/arm/src/stm32hx/stm32_adc: Reset channel counter before conversions.#19013
xiaoxiang781216 merged 2 commits into
apache:masterfrom
sammytranGeo:stm32hx/reset-channel-on-trigger-ioctl

Conversation

@sammytranGeo
Copy link
Copy Markdown
Contributor

@sammytranGeo sammytranGeo commented Jun 1, 2026

Summary

When an ADC conversion sequence is triggered via the ANIOC_TRIGGER ioctl, the priv->current channel counter was not being reset before starting the conversion. This meant that if a previous conversion sequence had ended mid-way through the channel list (e.g., due to an error or partial read), subsequent triggers would resume dispatching au_receive callbacks from the wrong channel index, corrupting the channel-to-data mapping reported to the upper half.

This fix resets priv->current to 0 immediately before calling adc_startconv() in the ANIOC_TRIGGER handler, ensuring the interrupt handler always starts dispatching results from the first channel in the sequence. The same fix is applied consistently to both the STM32H5 and STM32H7 ADC drivers, which share the same architecture and bug.

Impact

Users relying on the ANIOC_TRIGGER ioctl to initiate ADC conversion sequences on STM32H5 or STM32H7 targets could receive channel data attributed to the wrong channel number if the internal current counter was in a non-zero state when the trigger fired. This is a silent data corruption — no error is returned. After this fix, every triggered conversion sequence correctly begins at channel index 0.

Testing

Tested on an STM32H5-based board with the following defconfig and 12 ADC channels

CONFIG_ADC=y
CONFIG_ADC_FIFOSIZE=16
CONFIG_ANALOG=y

With an application that does the following:

struct adc_msg_s msg;
int nchannels;
uint8_t first_ch;

int fd = open("/dev/adc0", O_RDONLY);

nchannels = ioctl(fd, ANIOC_GET_NCHANNELS, 0);
printf("nchannels = %d\n", nchannels);

ioctl(fd, ANIOC_TRIGGER, 0);
read(fd, &msg, sizeof(msg));

first_ch = msg.am_channel;

ioctl(fd, ANIOC_RESET_FIFO, 0);
ioctl(fd, ANIOC_TRIGGER, 0);

read(fd, &msg, sizeof(msg));
if (msg.am_channel == first_ch)
{
  printf("PASS: priv->current was reset on retrigger\n");
}
else
{
  printf("FAIL: priv->current was NOT reset -- expected ch %u, got ch %u\n",
         first_ch, msg.am_channel);
}

Pre-fix

nchannels = 12
FAIL: priv->current was NOT reset -- expected ch 0, got ch 15

Post fix

nchannels = 12
PASS: priv->current was reset on retrigger

@github-actions github-actions Bot added Arch: arm Issues related to ARM (32-bit) architecture Size: XS The size of the change in this PR is very small labels Jun 1, 2026
sammytranGeo and others added 2 commits June 1, 2026 14:53
Signed-off-by: Sammy Tran <sammytran@geotab.com>

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
AI-Model: claude-sonnet-4.6
Signed-off-by: Sammy Tran <sammytran@geotab.com>

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
AI-Model: claude-sonnet-4.6
@sammytranGeo sammytranGeo force-pushed the stm32hx/reset-channel-on-trigger-ioctl branch from 823b82a to cf88150 Compare June 1, 2026 18:55
@sammytranGeo sammytranGeo changed the title Stm32hx/reset channel on trigger ioctl arch/arm/src/stm32hx/stm32_adc: Reset channel counter before conversions. Jun 1, 2026
@linguini1
Copy link
Copy Markdown
Contributor

linguini1 commented Jun 1, 2026

Why is this the correct behaviour?

To my knowledge, the ADC drivers with multiple channels take the approach that doing multiple reads return channel measurements one-by-one (i.e. first read call return CH1, then second call is CH2, etc.).

With this change, anyone using ANIOC_TRIGGER cannot trigger any channel besides channel 0, right? That doesn't seem correct.

@sammytranGeo
Copy link
Copy Markdown
Contributor Author

@linguini1 This particular driver does not have single channel conversion supported. adc_set_ch is never called externally,
It's only called internally via adc_setup with ch=0, which sets rnchannels = cchannels (all channels).

Conversion of all channels are triggered by the ioctl, and samples are read from the file. Depending on when the file is read (between ISRs or after all the channels are converted), it can be one or more samples.

@linguini1
Copy link
Copy Markdown
Contributor

Ah, I see. So right now the flow is:

  • Start trigger with ANIOC_TRIGGER
  • Read some number of channels
  • Start another trigger with ANIOC_TRIGGER
  • If you didn't read all of the channels, your next read picks up where you left off before

And this change makes it:

  • Start trigger with ANIOC_TRIGGER
  • Read some number of channels
  • Start another trigger with ANIOC_TRIGGER
  • Read some number of channels, starting from 0

Is that right?

If so, I still don't fully understand why it's more desirable for a re-trigger to start from 0 again? It seems fine but hopefully this isn't different behaviour from other NuttX ADC drivers or that will get confusing. The ADC driver is kind of poorly designed to begin with.

@sammytranGeo
Copy link
Copy Markdown
Contributor Author

@linguini1 Yes, that's correct. The reason it's necessary is correctness rather than preference: priv->current isn't a read cursor — it's the software's index into the hardware's scan sequence, used to tag each EOC result with the right channel number in au_receive(). The hardware always restarts its scan from position 0 on a new trigger, so priv->current must match. Without the reset, if a re-trigger happens before all EOCs from the previous scan are drained, the next results get tagged with the wrong channel numbers.

@xiaoxiang781216 xiaoxiang781216 merged commit 3e3427d into apache:master Jun 2, 2026
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Arch: arm Issues related to ARM (32-bit) architecture Size: XS The size of the change in this PR is very small

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants