makers-for-life / makair

🫁 The world's first open-source ventilator tested on human patients. Mass-producible at a low cost (~2000€).
Other
818 stars 78 forks source link

Buzzer code review #165

Closed ABOSTM closed 4 years ago

ABOSTM commented 4 years ago

After reviewing several PR that have already been merged, I have few remarks. I made a global review on SHA1: 5505822acfad62cdd9b0ff6daf7c5f1c7fdef8dc:

1) Buzzer_Init() BuzzerTim->setPrescaleFactor((BuzzerTim->getTimerClkFreq() / (TIMER_TICK_PER_MS * 1000)) - 1); It is true that hardware register should be assigned a value like (prescaler - 1) but this is already taken into account in HardwareTimer class. So '- 1' must be removed'

2) BuzzerControl_On() In HARDWARE_VERSION == 2 Buzzer_Hw_Timer->setMode() , Buzzer_Hw_Timer->setOverflow() and Buzzer_Hw_Timer->setCaptureCompare() could be moved to BuzzerControl_Init() . Doing this only once is enough and thus we avoid useless call specially in interrupt callback.

3) Buzzer_Mute() Useless to call BuzzerTim->attachInterrupt() because it is attached in Buzzer_Start() and never detached.

4) Update_IT_callback() BuzzerTim->pause() In V1 (as well as in V2) there is a chance to pause timer while buzzer pin is High (or PWM active). It is needed to set pin low (or PWM disabled) BuzzerControl_Off(); Also, in the else state, it looks like BuzzerTim->resume(); is useless, timer is already running isn't it ? It was formerly necessary to due to former setMode().

5) Buzzer_Resume() I would suggest that resume restarts from the beginning of buzzer pattern: Active_Buzzer_Index = 0; Also BuzzerControl_Off(); is useless: setOverflow can be called while timer is running. And useless to call BuzzerTim->attachInterrupt() because it is attached in Buzzer_Start()and never detached.