Perchance We May need Input?#122
Conversation
…d allow for a general PWM_INPUT for f3
…, need to figure out the pins
… remaining pins to f3xx
…e, added all the correct pins to PWM_INPUTF3xx.cpp. Should be good for testing
ActuallyTaylor
left a comment
ActuallyTaylor
left a comment
There was a problem hiding this comment.
This looks great! Very very solid code.
I only have one suggestion for one of the samples (seems to be a testing change left over).
I also want to note that pieces of this class will need to change with #125, but that can be handled in that PR.
Left over testing change Co-authored-by: Taylor Lineman <taylor.lineman@gmail.com>
Removed extra empty line Co-authored-by: Taylor Lineman <taylor.lineman@gmail.com>
mjh9585
left a comment
mjh9585
left a comment
There was a problem hiding this comment.
Very nice, mainly just a bunch of nit picks. Some of this comments may not apply once the new timers are in, but still things to be aware of. Good work!
|
|
||
| namespace core::io { | ||
|
|
||
| PWM_INPUT::PWM_INPUT(Pin pin) { // core::io::Pin pin |
There was a problem hiding this comment.
Convert this to an initializer list.
| TIM_HandleTypeDef* PWM_INPUTf3xx::getHandle() { | ||
| return &halTIM; | ||
| } |
There was a problem hiding this comment.
Instead of making this publicly available, store a pointer to your timer in a file global variable like your activePwmInput var that you could use in your interrupt.
| if (activePwmInput) { | ||
| activePwmInput->handleCapture(htim); | ||
| } |
There was a problem hiding this comment.
To support multiple inputs, you will need a way to look up the class instance from the timer instance. Look at some of the other drivers for an example.
| } | ||
| } | ||
|
|
||
| void PWM_INPUTf3xx::handleCapture(TIM_HandleTypeDef* htim) { |
There was a problem hiding this comment.
Place your IRQ handlers at the top of your file and keep your class member definition together for better readability.
|
Looks great :) Approved, pending Matts requested changes! |
…xed order of #include directives.
print duty cycle w/ min 2 digits Co-authored-by: Matthew Heller <69865851+mjh9585@users.noreply.github.com>
…-EVT/EVT-core into feature/xmb4293/pwm_input_v2
ActuallyTaylor
left a comment
ActuallyTaylor
left a comment
There was a problem hiding this comment.
I went back over @mjh9585's suggestions and almost everything seems to be fixed. The only thing I see right now is the multiple input instances problem he brought up.
3 participants
Implemented Pulse Width Modulation Input for f302. Tested all pins. I have a file that lists all correct pins and alternate functions specifically for PWM_INPUT, and some reasons why certain ones aren't used but I don't have permission to edit the drive.