STM32H7 ADCv4 patches Topic is solved

[alex31]

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

[FXCoder]

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

[alex31]

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

[Giovanni]

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

[alex31]

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.

[Giovanni]

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

[alex31]

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

[Giovanni]

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

[Giovanni]

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

[alex31]

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