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

[BUG] - ADC polling LPADC_GetConvResult #138

Closed
AndreaRuberti opened this issue Sep 4, 2023 · 7 comments
Closed

[BUG] - ADC polling LPADC_GetConvResult #138

AndreaRuberti opened this issue Sep 4, 2023 · 7 comments
Labels
bug Something isn't working

Comments

@AndreaRuberti
Copy link

Describe the bug
Last implementation of function LPADC_GetConvResult does not allow correct polling management.

To Reproduce

  • Environment:
    • Tag/Commit hash: MCUX_2.14.0 - fsl_lpadc.c/.h
    • Toolchain: ARMGCC 12.1
    • Board/SoC: LPC5506 on Custom board.
    • Example: temperature_measurement (original source from SDK 2.13), lpadc_polling (SDK 2.14)
  • Steps to reproduce the behavior:
    • Just execute the polling example without the LPADC_DoSoftwareTrigger.
    • execute "temperature_measurement" example from SDK 2.13 and comment LPADC_DoSoftwareTrigger.

Expected behavior
In polling management, function LPADC_GetConvResult should return false in case of not ready value (as in previous SDK 2.13).
The 2.14 implementation provide a WHILE loop inside the function that could "stop" the program execution if no data ready.
Furthermore, if no LPADC_DoSoftwareTrigger are execute and no WDOG is started... the system loop forever inside LPADC_GetConvResult fx.

Screenshots and console output
none

Additional context

  • Temporay solution for data polling:
    • without lib changes: add/test if (ADC_RESFIFO_VALID_MASK & base->RESFIFO) before LPADC_GetConvResult
    • with lib Changes: replace fsl_lpadc.c/.h from older SDK 2.13 version.
@AndreaRuberti AndreaRuberti added the bug Something isn't working label Sep 4, 2023
@mcuxsusan
Copy link
Contributor

Thanks for reporting the issue, already asked development team to check, reply could be delayed.

@ZhaoxiangJin
Copy link
Contributor

Hi @AndreaRuberti, in MCUX-SDK 2.14 version, the change to the function LPADC_GetConvResult is due to the needs of the NXP Connectivity Team and has been reverted, in the MCUX-SDK release version after the connectivity branch merges back, the implementation of this function will return to the style of the previous version. We recommend calling the function LPADC_GetConvResultCount before calling the function LPADC_GetConvResult to ensure that it does not enter an infinite loop (in fact, such a calling flow is more reasonable).

@herrmanthegerman
Copy link

... in MCUX-SDK 2.14 version, the change to the function LPADC_GetConvResult is due to the needs of the NXP Connectivity Team ...

I don't understand. Are you saying that "due to the needs of the NXP Connectivity Team", you are providing an implementation that

  • likely breaks existing software
  • is not reflected in any documentation, not even in the doxygen comment of the function?

I think that is no adequate software quality management.

I wonder if there are similar changes hidden somewhere else in SDK 2.14?

@ZhaoxiangJin
Copy link
Contributor

ZhaoxiangJin commented Sep 15, 2023

Hi @herrmanthegerman, Sorry that the impact of this change was not fully considered during development, and the current test case hasn't covered the behavior change issue. We will add a test case for it. We have checked the changed parts of the driver in MCUX-SDK version 2.13.1 and 2.14, there are no other similar changes hidden in the driver. We will create a patch ASAP to change the function LPADC_GetConvResult back and add a new API LPADC_GetConvResultBlocking which waits until the conversion is done. Thank you for your feedback.

@herrmanthegerman
Copy link

We have checked the changed parts of the driver in MCUX-SDK version 2.13.1 and 2.14, there are no other similar changes hidden in the driver. Thank you for your feedback.

Thanks @ZhaoxiangJin. Just to avoid any misunderstanding: Regarding "similar changes", I was referring to all drivers (see directory drivers/) not only the ADC driver. So which drivers did you check? Thank you for your support.

@mcuxsusan
Copy link
Contributor

Hi @herrmanthegerman, for your information, the team has checked all driver change under the drivers/ folder and confirmed that no similar issue for other drivers.

@mcuxsusan
Copy link
Contributor

Hi, I will close the issue for now. The fix has been applied to latest main branch, please see 1d72f41.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants