STM32H7 ADCv4 patches

Report here problems in any of ChibiOS components. This forum is NOT for support.
User avatar
alex31
Posts: 334
Joined: Fri May 25, 2012 10:23 am
Location: toulouse, france
Has thanked: 33 times
Been thanked: 46 times
Contact:

Re: STM32H7 ADCv4 patches

Postby alex31 » Mon Apr 12, 2021 10:30 pm

Before continuing on the STM32 support forum, I try just once to continue here because it seems to me that there is a bug in the driver at least for the ADC3 case.

You give me a good advice to see if the memory was half filled, and it is the case :

I am using a DEPTH = 4, and I saw that the ADC correctly filled the first row of data. I tried with DEPTH=1 and bingo, in that case it works, so the problem is when DEPTH > 1, the transfer is stuck after the first row.

I made a try using enabling the CONT bit in the CFGR register when depth if > 1 and in one_shot mode, and now it works, and in this case I force adcstop with ADSTP bit in the transfert complete callback.

In fact before submitting a patch I 'll need to test all the combinations of ADC1/ADC3 oneshot/continous width=1/width>1 to see what combinations need this CONT bit.


Alexandre

User avatar
FXCoder
Posts: 363
Joined: Sun Jun 12, 2016 4:10 am
Location: Sydney, Australia
Has thanked: 168 times
Been thanked: 120 times

Re: STM32H7 ADCv4 patches

Postby FXCoder » Tue Apr 13, 2021 3:40 am

FYI there are some other patches related to ADC AWD support (which also affect/change interrupt handler) waiting review.

User avatar
alex31
Posts: 334
Joined: Fri May 25, 2012 10:23 am
Location: toulouse, france
Has thanked: 33 times
Been thanked: 46 times
Contact:

Re: STM32H7 ADCv4 patches

Postby alex31 » Tue Apr 13, 2021 11:28 am

Hello,
after tests, ADC_CFGR_CONT is needed whenever DEPTH > 1, in one shot or continuous mode, on ADC1 and ADC3.

I attach a patch which passes a test which try all combinations.

regarding AWD patch, I will test it asap.

I also have a question about adcSTM32EnableTS and adcSTM32EnableVREF : for the case of the H7, they also need to set MONEN bit in STM32_PWR_CR2 register. There is several options, let the user add the bit in STM32_PWR_CR2 in mcuconf.h and just test that the bit is enabled with a chDbgAssert in adcSTM32EnableTS/VREF or try to dynamically change STM32_PWR_CR2 in adcSTM32EnableTS/VREF, which one is the best ?

Alexandre
Attachments
adcv4_depth_gt_1.patch.zip
(924 Bytes) Downloaded 9 times

User avatar
Giovanni
Site Admin
Posts: 13520
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 861 times
Been thanked: 743 times
Contact:

Re: STM32H7 ADCv4 patches

Postby Giovanni » Tue Apr 13, 2021 12:36 pm

Hi,

Each driver should only touch its own peripheral so setting up PWR MONEN has to be done in mcuconf.h, putting an assertion could be a good idea.

Giovanni

User avatar
alex31
Posts: 334
Joined: Fri May 25, 2012 10:23 am
Location: toulouse, france
Has thanked: 33 times
Been thanked: 46 times
Contact:

Re: STM32H7 ADCv4 patches

Postby alex31 » Tue Apr 13, 2021 7:30 pm

Here is a second version of the patch with chDbgAssert in case of use of adcSTM32EnableTS and adcSTM32EnableVREF without MONEN bit set in STM32_PWR_CR2 register.

A.
Attachments
adcv4_depth_gt_1.patch.zip
second version of the patch
(1.05 KiB) Downloaded 10 times

User avatar
Giovanni
Site Admin
Posts: 13520
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 861 times
Been thanked: 743 times
Contact:

Re: STM32H7 ADCv4 patches

Postby Giovanni » Thu Apr 22, 2021 5:37 am

Hi Alex,

I did a small review of the patch, few points:

1) I am not sure that the CONT bit should be handled by the driver, if we do that then the DISCEN mode is no more possible, it is also not needed using HW triggering. In my opinion CONT or DISCEN should be specified in the ADCConversionGroup structure when required. Note that I looked to other ADC drivers and there is no consistency, some handle CONT internally some others not. Probably we should review them all.

From the RM:

It is not possible to have both discontinuous mode and continuous mode enabled: it is forbidden to set both DISCEN=1 and CONT=1


2) Please try to not use TAB characters in patches, it is against our style rules.

I have committed the missing definitions in the header and the assertions only.

Giovanni

User avatar
alex31
Posts: 334
Joined: Fri May 25, 2012 10:23 am
Location: toulouse, france
Has thanked: 33 times
Been thanked: 46 times
Contact:

Re: STM32H7 ADCv4 patches

Postby alex31 » Thu Apr 22, 2021 8:19 pm

Hi,

I have committed the missing definitions in the header and the assertions only.


Ok, I am diffing the diff :-)

So the patch sent is big, it's an aggregate of Bob patches for AWD an my patches for CONT, WIDE_SAMPLE (to permit the use of oversampling).

About CONT, As you say, it should be homogeneous between drivers. I have a preference for the driver managing this but, but I can live if I have to take care of it in the configuration structure, after all, it's illusory to think that you can use ADC without thoroughly reading the RF.

I saw that you did not take the AWD part of the patch, without that, it's not possible to use AWD, and there is bugs left in the .c :

Code: Select all

static void adc_lld_serve_interrupt(ADCDriver *adcp, uint32_t isr) {

  /* It could be a spurious interrupt caused by overflows after DMA disabling,
     just ignore it in this case.*/
  if (adcp->grpp != NULL) {
    /* Note, an overflow may occur after the conversion ended before the driver
       is able to stop the ADC, this is why the state is checked too.*/
    if ((isr & ADC_ISR_OVR) && (adcp->state == ADC_ACTIVE)) {
      /* ADC overflow condition, this could happen only if the DMA is unable
         to read data fast enough.*/
      _adc_isr_error_code(adcp, ADC_ERR_OVERFLOW);
    }
    if (isr & ADC_ISR_AWD1) {
      /* Analog watchdog error.*/
      _adc_isr_error_code(adcp, ADC_ERR_AWD1);
    }
    if (isr & ADC_ISR_AWD2) {
      /* Analog watchdog error.*/
      _adc_isr_error_code(adcp, ADC_ERR_AWD2);
    }
    if (isr & ADC_ISR_AWD3) {
      /* Analog watchdog error.*/
      _adc_isr_error_code(adcp, ADC_ERR_AWD3);
    }
  }
}


in this handler, if there is multiple concurrent trigger (which can be the case mith multiple AWD) the driver will hardfault because _adc_isr_error_code will be called multiple time and it is no meant to do that.

there is also typos left in the register setting from configuration :

Code: Select all

  adcp->adcm->LTR1  = grpp->ltr2;
    adcp->adcm->HTR1  = grpp->htr2;
                         ^^^^^             ^^^^^
    adcp->adcm->LTR1  = grpp->ltr3;
    adcp->adcm->HTR1  = grpp->htr3;
 .
 .
 .
    adcp->adcs->HTR1  = grpp->htr1;
    adcp->adcs->LTR1  = grpp->ltr2;
    adcp->adcs->HTR1  = grpp->htr2;
    adcp->adcs->LTR1  = grpp->ltr3;
    adcp->adcs->HTR1  = grpp->htr3;
 


About the DUAl bit, since it can be set in the config file, It has the same status that the CONT bit, user should take care.

The WIDE_SAMPLE option has to be handled by the driver if one want to use the oversampling option of the driver since the DMA width is now 32 bits.

So, if CONT dans DUAL are left out of the driver, I think that at least AWD should be fixed (.h and .c) , and WIDE_SAMPLE would be a plus.

Giovanni, if you want separates patches for AWD and WIDE_SAMPLE, i cant make them over trunk.

Alexandre

User avatar
Giovanni
Site Admin
Posts: 13520
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 861 times
Been thanked: 743 times
Contact:

Re: STM32H7 ADCv4 patches

Postby Giovanni » Thu Apr 22, 2021 8:27 pm

Hi,

Please don't make everything in a single huge patch, if you are fixing different issues then make different patches or it is going to be rejected as a whole even if there is a single problem.

Giovanni

User avatar
Giovanni
Site Admin
Posts: 13520
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 861 times
Been thanked: 743 times
Contact:

Re: STM32H7 ADCv4 patches

Postby Giovanni » Fri Apr 23, 2021 6:09 am

Fixing the AWD problem requires a change in the HLD, errors need to become a mask instead of an enumeration, I will change the definitions later today, all ADC drivers need to be updated after this.

Giovanni

User avatar
alex31
Posts: 334
Joined: Fri May 25, 2012 10:23 am
Location: toulouse, france
Has thanked: 33 times
Been thanked: 46 times
Contact:

Re: STM32H7 ADCv4 patches

Postby alex31 » Fri Apr 23, 2021 7:40 am

Hi,

Yes, I told that to Bob about the mask instead of enum in a PM, but I was thinking that the changes in all drivers would be a show stopper.

I will wait for your changes, and after that will propose different patches for fixes and features next week.

About AWD, is it right to manage limit detections as errors and stop the driver ? In some case, limit detection can be perfectly fine and cyclic, if one want to implement a shmitt trigger by example.

Alexandre


Return to “Bug Reports”

Who is online

Users browsing this forum: No registered users and 3 guests