Bug in os/hal/include/hal_adc.h error handling Topic is solved

[awygle]

Hi,

I believe there is a bug in os/hal/include/hal_adc.h's error handling. I am using the most current version of ChibiOS (HEAD).

Specifically the problem is with _adc_isr_error_code and the way it manipulates driver states. The code looks like this:

Code: Select all

#define _adc_isr_error_code(adcp, err) {                                    \
  adc_lld_stop_conversion(adcp);                                            \
  if ((adcp)->grpp->error_cb != NULL) {                                     \
    (adcp)->state = ADC_ERROR;                                              \
    (adcp)->grpp->error_cb(adcp, err);                                      \
    if ((adcp)->state == ADC_ERROR)                                         \
        (adcp)->state = ADC_READY;                                          \
  }                                                                         \
  (adcp)->grpp = NULL;                                                      \
  _adc_timeout_isr(adcp);                                                   \
}
 

Technically this matches the state machine diagram at http://chibios.sourceforge.net/docs3/ha ... a_d_c.html - the driver only goes into ADC_ERROR for the length of the error_cb call. However, what happens if there's no error_cb - if error_cb is NULL? We skip right past the if statement, the group is set to NULL, and the timeout isr code is called - leaving the driver in state ADC_ACTIVE. That doesn't make sense to me - we're no longer converting, we've called adc_lld_stop_conversion. We should be in ADC_READY, just like we are if error_cb calls a completely empty function.

One way to fix this is the following, but there may be others that are preferable:

Code: Select all

#define _adc_isr_error_code(adcp, err) {                                    \
  adc_lld_stop_conversion(adcp);                                            \
  (adcp)->state = ADC_ERROR;                                                \
  if ((adcp)->grpp->error_cb != NULL) {                                     \
    (adcp)->grpp->error_cb(adcp, err);                                      \
  }                                                                         \
  if ((adcp)->state == ADC_ERROR)                                           \
    (adcp)->state = ADC_READY;                                              \
  (adcp)->grpp = NULL;                                                      \
  _adc_timeout_isr(adcp);                                                   \
}


[Giovanni]

Hi,

Thanks for finding, looking at the code there are two problems there, also "grpp" is not reset to NULL. Probably it is best to add an "else" to the "if" and force the ready state, the _adc_isr_full_code() macro already does it this way:

Code: Select all

#define _adc_isr_error_code(adcp, err) {                                    \
  adc_lld_stop_conversion(adcp);                                            \
  if ((adcp)->grpp->error_cb != NULL) {                                     \
    (adcp)->state = ADC_ERROR;                                              \
    (adcp)->grpp->error_cb(adcp, err);                                      \
    if ((adcp)->state == ADC_ERROR) {                                       \
      (adcp)->state = ADC_READY;                                            \
      (adcp)->grpp = NULL;                                                  \
    }                                                                       \
  }                                                                         \
  else {                                                                    \
    (adcp)->state = ADC_READY;                                              \
    (adcp)->grpp = NULL;                                                    \
  }                                                                         \
  _adc_timeout_isr(adcp);                                                   \
}


Giovanni

[RoccoMarco]

Fixed in repos as bug #751 according to Giovanni's suggestion.
Ciao,
RM

[awygle]

Thanks for the quick turnaround, you two, it works just fine

[RoccoMarco]

Perfect! By the way, in this case the solution has been entirely provided by Giovanni. It has been easy! LOL
Ciao,
RM