adc: adc_read() not check for error (#1575)

[ruehlchris]

Implementation of adc_read() not check for errors in the ADC_CS register and might hang forever in a while() loop.

• change the return type from uint16_t to int32_t

• clean start with ADC_CS_START_ONCE_RESET before call ADC_CS_START_ONCE_BITS.

• check for ADC_CS_READY_BITS and error ADC_CS_ERR_BITS or ADC_CS_ERR_STICKY_BITS

• return the ADC value (12bits) or the negative value of ADC_CS_ERR_BITS or ADC_CS_ERR_STICKY_BITS depending on the error

[Reneg973]

Are the chances that errors occur during adc_read() very high? During all my tests I never had any error. I would prevent error checking if circumstances are unlikely to happen or if high frequency sampling is required.

[ruehlchris]

Are the chances that errors occur during adc_read() very high? During all my tests I never had any error. I would prevent error checking if circumstances are unlikely to happen or if high frequency sampling is required.

What will be the impact on high frequency sampling, the system will be in the tideloop anyway waiting for the ADC to be set the ready bit. Testing the register bits will not have a huge impact. The alternative to use the fifo style implementation which fits much better for what you mentioned.

My opinion, if the hardware give you a hand and signal a bad ADC then do not use the result.

[Reneg973]

What will be the impact on high frequency sampling, the system will be in the tideloop anyway waiting for the ADC to be set the ready bit. Testing the register bits will not have a huge impact. The alternative to use the fifo style implementation which fits much better for what you mentioned.

My opinion, if the hardware give you a hand and signal a bad ADC then do not use the result.

Yes, nevertheless setting the ADC_CS_START_ONCE_RESET seems not to be required, as the ADC_CS_START_ONCE_BITS is an auto reset flag. And after compilation the (my) compiler produces an unnecessary jump for the most likely code path. Also the last condition can be removed as it is the only remaining condition:

static inline int32_t adc_read(void) {
     hw_set_bits(&adc_hw->cs, ADC_CS_START_ONCE_BITS);

     while ((adc_hw->cs & (ADC_CS_READY_BITS|ADC_CS_ERR_BITS|ADC_CS_ERR_STICKY_BITS))==0) {
         tight_loop_contents();
     }

     if ((adc_hw->cs & ADC_CS_READY_BITS)>0) {
         return adc_hw->result & 0xfff;
     }
     if ((adc_hw->cs & ADC_CS_ERR_BITS)>0) {
         return -ADC_CS_ERR_BITS;
     }
     /* clear sticky bit */
     hw_set_bits(&adc_hw->cs, ADC_CS_ERR_STICKY_BITS);
     return -ADC_CS_ERR_STICKY_BITS;
 }

[ruehlchris]

What will be the impact on high frequency sampling, the system will be in the tideloop anyway waiting for the ADC to be set the ready bit. Testing the register bits will not have a huge impact. The alternative to use the fifo style implementation which fits much better for what you mentioned. My opinion, if the hardware give you a hand and signal a bad ADC then do not use the result.

Yes, nevertheless setting the ADC_CS_START_ONCE_RESET seems not to be required, as the ADC_CS_START_ONCE_BITS is an auto reset flag. And after compilation the (my) compiler produces an unnecessary jump for the most likely code path. Also the last condition can be removed as it is the only remaining condition:

static inline int32_t adc_read(void) {
     hw_set_bits(&adc_hw->cs, ADC_CS_START_ONCE_BITS);

     while ((adc_hw->cs & (ADC_CS_READY_BITS|ADC_CS_ERR_BITS|ADC_CS_ERR_STICKY_BITS))==0) {
         tight_loop_contents();
     }

     if ((adc_hw->cs & ADC_CS_READY_BITS)>0) {
         return adc_hw->result & 0xfff;
     }
     if ((adc_hw->cs & ADC_CS_ERR_BITS)>0) {
         return -ADC_CS_ERR_BITS;
     }
     /* clear sticky bit */
     hw_set_bits(&adc_hw->cs, ADC_CS_ERR_STICKY_BITS);
     return -ADC_CS_ERR_STICKY_BITS;
 }

Happy New Year,

Yes, I think you nailed it and optimized it out. I will modify the patch accordant to your succession and create a new pull request.