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

Macro LL_ADC_SetChannelSingleDiff behaves incorrectly when setting a channel to single ended #53

Open
microbug opened this issue Jan 6, 2025 · 2 comments
Assignees
Labels
adc Analog-to-Digital Converter bug Something isn't working hal HAL-LL driver-related issue or pull-request. needs clarification Needs clarification or inputs from the user

Comments

@microbug
Copy link

microbug commented Jan 6, 2025

Describe the set-up

  • Followed this guide on the ST Wiki to set up Nucleo with USB Type-C PD sink.
  • Boards: Nucleo-G474RE with X-NUCLEO-SNK1M1 USB Type-C PD sink daughterboard.
  • STM32CubeIDE v1.17.0

Describe the bug
The macro function LL_ADC_SetChannelSingleDiff should clear bit ADCx->DIFSEL[ 1 << n ] when setting channel n to single-ended. Instead, the bit is set. This causes the channel to be incorrectly set to differential, resulting in invalid measurements.

See STM32G4 Reference Manual, RM0440 Rev 8 section 21.7.21.

How To Reproduce

  1. Indicate the global behavior of your application project.
    When running the application, ADC readings are incorrect. This makes it impossible to verify that the USB Type-C PD VBUS voltage is safe.

  2. The modules that you suspect to be the cause of the problem (Driver, BSP, MW ...).
    This macro function in HAL-LL Drivers.

  3. The use case that generates the problem.
    Generate default project following link below, and measure VBUS voltage by setting a breakpoint in the relevant function.

  4. How we can reproduce the problem.
    Generate a project following this guide on the ST Wiki using Nucleo-G474RE and X-NUCLEO-SNK1M1 boards.

Run project in Debug mode. Place a breakpoint on line *pVoltage = voltage in function BSP_USBPD_PWR_VBUSGetVoltage, file USBPD/usbpd_pwr_user.c. Observe that when a USB charger is plugged into the USB Type-C port, the voltage calculated does not match expectations.

Because the ADC is running in continuous mode, you can also stop execution and inspect register ADC1->DR. When no USB Type-C device is plugged in, it should be close to 0x0. It will instead be much higher.

Also observe that when placing a breakpoint on the call to LL_ADC_SetChannelSingleDiff in file Drivers/STM32G4xx_HAL_Driver/Src/stm32g4xx_hal_adc.c (line 2885 in attached project), ADC1->DIFSEL is 0x0 before the call and 0x8000 after the call (bit 15 has been set).

Additional context
The macro (in file stm32g4xx_hal_adc.h) currently looks like this (comment is not mine):

__STATIC_INLINE void LL_ADC_SetChannelSingleDiff(ADC_TypeDef *ADCx, uint32_t Channel, uint32_t SingleDiff)
{
  /* Bits of channels in single or differential mode are set only for         */
  /* differential mode (for single mode, mask of bits allowed to be set is    */
  /* shifted out of range of bits of channels in single or differential mode. */
  MODIFY_REG(ADCx->DIFSEL,
             Channel & ADC_SINGLEDIFF_CHANNEL_MASK,
             (Channel & ADC_SINGLEDIFF_CHANNEL_MASK)
             & (ADC_DIFSEL_DIFSEL >> (SingleDiff & ADC_SINGLEDIFF_CHANNEL_SHIFT_MASK)));
}

Channel is (1 << n) where n is channel number, plus some other stuff beyond bit 24, which is masked out by ADC_SINGLEDIFF_CHANNEL_MASK
ADC_SINGLEDIFF_CHANNEL_MASK is 0x7FFFF
ADC_DIFSEL_DIFSEL is 0x7FFFF
ADC_SINGLEDIFF_CHANNEL_SHIFT_MASK is 24

So the simplified expansion is:

ADCx->DIFSEL = ADCx->DIFSEL & ~(1 << n) | ( (1 << n) & (0x7FFFF >> (SingleDiff & 24)) )

In the case where n=15 and SingleDiff=0 (which should set the channel to single-ended), this expands to:

ADCx->DIFSEL = ADCx->DIFSEL & ~(0x8000) | ( 0x8000 & 0x7FFFF )

which expands to:

ADCx->DIFSEL = ADCx->DIFSEL & ~(0x8000) | 0x8000

Bit 15 is set by this macro.

Solution
There is a project SNK1M1_Sink in X-CUBE-TCPP (the STM32CubeIDE project is in X-CUBE-TCPP\4.2.0\Projects\NUCLEO-G474RE\Applications\USB_PD\SNK1M1_Sink\STM32CubeIDE). This project is very similar to the example project created by following the Wiki, but it has a different version of stm32g4xx_ll_adc.h. This version has a different macro that appears to work correctly:

__STATIC_INLINE void LL_ADC_SetChannelSingleDiff(ADC_TypeDef *ADCx, uint32_t Channel, uint32_t SingleDiff)
{
  /* Bits for single or differential mode selection for each channel are set  */
  /* to 1 only when the differential mode is selected, and to 0 when the      */
  /* single mode is selected.                                                 */
  
  if (SingleDiff == LL_ADC_DIFFERENTIAL_ENDED)
  {
    SET_BIT(ADCx->DIFSEL,
            Channel & ADC_SINGLEDIFF_CHANNEL_MASK);
  }
  else
  {
    CLEAR_BIT(ADCx->DIFSEL,
            Channel & ADC_SINGLEDIFF_CHANNEL_MASK);
  }
}

Screenshots
None.

Project
Demo project files are attached: stm32g4xx_adc_bug.zip.

@ALABSTM ALABSTM added bug Something isn't working hal HAL-LL driver-related issue or pull-request. adc Analog-to-Digital Converter labels Jan 6, 2025
@RJMSTM
Copy link
Contributor

RJMSTM commented Jan 10, 2025

Hellon @microbug,

First thanks for your contribution, i am trying to analyze the code you sent in the zip file, but I am having trouble.

image

Could you please specify which part of your code is causing the ADC issue 'when setting channel n to single-ended. Instead, the bit is set. This causes the channel to be incorrectly set to differential'?

What is the version of STM32CubeIDE being used?

Regards,

@RJMSTM RJMSTM moved this from To do to Analyzed in stm32cube-mcu-fw-dashboard Jan 10, 2025
@RJMSTM RJMSTM added the needs clarification Needs clarification or inputs from the user label Jan 10, 2025
@microbug
Copy link
Author

The cause of the problem is file Drivers/STM32G4xx_HAL_Driver/Inc/stm32g4xx_ll_adc.h lines 6495-6498. When SingleDiff is 0, DIFSEL[n] should be set to 0 according to the reference manual (n is the channel number). This sets the channel to single ended.

However, this macro call does not do that, as explained above. Instead it sets DIFSEL[n] to 1, which causes incorrect measurements.

STM32CubeIDE is version 1.17.0, build 23558_20241125_2245 (UTC).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adc Analog-to-Digital Converter bug Something isn't working hal HAL-LL driver-related issue or pull-request. needs clarification Needs clarification or inputs from the user
Projects
Development

No branches or pull requests

3 participants