Fix incorrect bitfield name in ADC_Init #220
Conversation
| @@ -96,6 +96,13 @@ void ADC_Init(ADC_Type *base, const adc_config_t *config) | |||
| tmp32 |= ADC_CTRL_TSAMP(config->sampleTimeNumber); | |||
| #if (defined(FSL_FEATURE_ADC_SYNCHRONOUS_USE_GPADC_CTRL) && FSL_FEATURE_ADC_SYNCHRONOUS_USE_GPADC_CTRL) | |||
| } | |||
| else | |||
There was a problem hiding this comment.
Hello @tcv-git, thanks for your effort. I'd like to know what ADC clock mode in your project? From the K32W0 user manual information, the device doesn't support synchronous mode, and the ADC sample time is determined by GPADC_CTRL0[GPADC_TSAMP], the CTRL[TSAMP] should stay to default during application. So, for your patch, in asynchronous mode, you have no way to set the GPADC_CTRL0[GPADC_TSAMP].
Could you please close this PR? as we need to do internal devices feature header and driver update to fix this bug.
There was a problem hiding this comment.
Hi,
The device does support synchronous mode. See UM11323 Fig. 82. Perhaps you are reading the manual for another part?
I have set CLOCK_AttachClk(kNONE_to_ADC_CLK); and .clockMode = kADC_ClockSynchronousMode,. It is reading values correctly using the code fixed code I have supplied.
Have you tried this with hardware?
There was a problem hiding this comment.
By the way, I have seen the part that you are referring to in section 17.1.2 of the register manual. The lack of correct punctuation and the fact that it contradicts the sentence immediately before and after it make It looks like a copy+paste error. Perhaps you could arrange for the manual to be updated.
There was a problem hiding this comment.
Hello @tcv-git, yes, I have tried both synchronous mode and asynchronous mode on my local, ADC can convert in both modes. I checked with the internal team, it is strongly recommended you use asynchronous mode, using synchronous mode may result in reduced accuracy in some environments. The UM11323 needs to be updated to hide information about synchronous mode. We currently only describe related information in CTRL[ASYNMODE] and CTRL[CLKDIV] bitfield description field.
There was a problem hiding this comment.
Hi Zhaoxiang,
Thankyou for your help. Both the pdf documentation and example projects in the SDK need to be updated because they all use synchronous mode. Can you also please confirm that this will be published in the device errata document? because many customers will have already written applications based on the pre-existing examples.
More importantly for me, if you look for the function update_ctrl0_adc_register in board_utility.c, it modifies the sample time in the GPADC_CTRL0 register. If this is required to be changed to synchronous mode then the sample time will have to be made in the CTRL register and I need to know which values to use to replace 0x1f and 0x14.
Thanks,
Tom
There was a problem hiding this comment.
Hi,
By the way when you say "result in reduced accuracy in some environments", perhaps this is just caused by the driver bug writing incorrectly to the PASS field? Maybe synchronous mode does work fine once you make this fix. Are you please able to check internally what was the reason for this problem and repeat the testing? Maybe both modes should work now.
There was a problem hiding this comment.
Hi Zhaoxiang,
Thankyou for your help. Both the pdf documentation and example projects in the SDK need to be updated because they all use synchronous mode. Can you also please confirm that this will be published in the device errata document? because many customers will have already written applications based on the pre-existing examples.
More importantly for me, if you look for the function
update_ctrl0_adc_registerinboard_utility.c, it modifies the sample time in the GPADC_CTRL0 register. If this is required to be changed to synchronous mode then the sample time will have to be made in the CTRL register and I need to know which values to use to replace 0x1f and 0x14.Thanks, Tom
Hello @tcv-git ,
Sorry for the late reply. We will update both ADC driver and driver examples, we will also push the ERRATA update.
The GPADC_CTRL0[GPADC_TSAMP] can be used in both asynchronous mode and synchronous mode. The CTRL0[TSAMP] can only be used in asynchronous mode (ADC working on 4MHz functional clock) in K32W041/061 devices.
The conclusion of ‘ADC conversion result accuracy reduces’ is based on the test results during the chip production process. I confirmed with the SoC designer, the synchronous mode is not recommended to be used. I'd like to know, the synchronous mode is must to be used on your project?
There was a problem hiding this comment.
I'd like to add a new variable extendSampleTimeNumber to indicate GPADC_CTRL0[GPADC_TSAMP], add a new feature macro FSL_FEATURE_ADC_HAS_GPADC_CTRL0_GPADC_TSAMP to indicate whether the ADC in the device has the GPADC_CTRL0 register. Then the code should be:
#if (defined(FSL_FEATURE_ADC_HAS_GPADC_CTRL0_GPADC_TSAMP) && FSL_FEATURE_ADC_HAS_GPADC_CTRL0_GPADC_TSAMP)
uint32_t tmp_ctrl0 = base->GPADC_CTRL0;
tmp_ctrl0 &= ~ADC_GPADC_CTRL0_GPADC_TSAMP_MASK;
tmp_ctrl0 |= ADC_GPADC_CTRL0_GPADC_TSAMP(config->extendSampleTimeNumber);
base->GPADC_CTRL0 = tmp_ctrl0;
#endif /* FSL_FEATURE_ADC_HAS_GPADC_CTRL0_GPADC_TSAMP */
#if defined(FSL_FEATURE_ADC_HAS_CTRL_TSAMP) & FSL_FEATURE_ADC_HAS_CTRL_TSAMP
/* Sample time clock count. */
#if (defined(FSL_FEATURE_ADC_SYNCHRONOUS_USE_GPADC_CTRL) && FSL_FEATURE_ADC_SYNCHRONOUS_USE_GPADC_CTRL)
if (config->clockMode == kADC_ClockAsynchronousMode)
{
#endif /* FSL_FEATURE_ADC_SYNCHRONOUS_USE_GPADC_CTRL */
tmp32 |= ADC_CTRL_TSAMP(config->sampleTimeNumber);
#if (defined(FSL_FEATURE_ADC_SYNCHRONOUS_USE_GPADC_CTRL) && FSL_FEATURE_ADC_SYNCHRONOUS_USE_GPADC_CTRL)
}
#endif /* FSL_FEATURE_ADC_SYNCHRONOUS_USE_GPADC_CTRL */
#endif /* FSL_FEATURE_ADC_HAS_CTRL_TSAMP */
There was a problem hiding this comment.
Hi Zhaoxiang,
Yes we can use asynchronous mode with a 4MHz clock. I was only trying to save a small amount of power by using synchronous mode.
After the updated SDK is available I am happy for you to reject this pull request.
Thanks,
Tom
This is a pretty clear bug that causes the ADC sample time to not be set as configured and also causes the PASS bit to be set incorrectly if the configured sample time was odd.
Setting PASS results in wildly incorrect ADC results. Not setting the sample time will also produce incorrect results if the ADC input signal is of high impedance.
I have put the fix in the same place as sampleTimeNumber is otherwise set, but you may which to reformat because I don't know if there is a strict order that registers must be written, and also I don't know all the possible silicon variations this might be used with. I will test this with my K32W041A and post the results through the NXP community forum.