search

Blinkinlabs / ch554_sdcc

CH554 software development kit for SDCC
306 stars 71 forks source link

Misused `&=` operator in method `ADCInit()` #46

Closed limingjie closed 2 years ago

limingjie commented 2 years ago

I am writing an email to tech@wch.cn for this issue. Also submit an issue here, in case anyone meets the same problem.

The &= operator is misused in the ADCInit() method in adc.c. The expression on the right side of the operator &= will be calculated first, it may lead to an unexpected value in a certain case.

ADC_CFG &= ~bADC_CLK | speed;

// It implies
ADC_CFG = ADC_CFG & (~bADC_CLK | speed);

The 3rd case failed to set the bit.

- 0x00 & (0xfe | 0) = 0x0
- 0x01 & (0xfe | 0) = 0x0
- 0x00 & (0xfe | 1) = 0x0 // Failed to set the bit
- 0x01 & (0xfe | 1) = 0x1

It should have been like this.

ADC_CFG = ADC_CFG & ~bADC_CLK | speed;

All cases work as expected.

- 0x00 & 0xfe | 0 = 0x0
- 0x01 & 0xfe | 0 = 0x0
- 0x00 & 0xfe | 1 = 0x1
- 0x01 & 0xfe | 1 = 0x1
limingjie commented 2 years ago

The comment of the code is also the opposite of the document.

ADC reference clock frequency selection bit, if the bit is 0, select the slow clock, and 384 Fosc cycles are required for each ADC. If the bit is 1, select the fast clock, and 96 Fosc cycles are required for each ADC

* Input          : uint8_t speed clock setting
1 Slow 384 Fosc                                                  
0 Fast 96 Fosc
cibomahto commented 2 years ago

Good catch- I'd write it like this to make it even more clear: ADC_CFG = (ADC_CFG & ~bADC_CLK) | speed;

If you send a pull request, we can merge it.