New pca9685 backend #57
Conversation
…no longer working. this is the start of a rewrite
Hermanoid
left a comment
Hermanoid
left a comment
There was a problem hiding this comment.
Some cleaning up but I'm not about to forge into the datasheet (willingly, anyway) so I'm accepting the rest :)
src/rov/rov_pca9685/src/PCA9685.cpp
Outdated
| } | ||
|
|
||
| void PCA9685::setDuty(uint8_t channel, float duty, float _delay) { | ||
| // uint16_t time_on = static_cast<uint16_t>(std::round(duty * 4096)); |
There was a problem hiding this comment.
comments are the "intended" way of doing this. could not get it to work with 0% duty cycle though since the math for the time_off register underflows at this condition...
This is actually a really hacky way of using the 9685. When counts <= time_off, the output is inverted. When only time_off is utilized, its actually the total number of counts that the pwm signal is asserted (leading edge at 0 counts, trailing edge at time_off counts).
If you could figure out how the math is supposed to work at this condition we would have a more functional 9685 driver (capable of offsetting the pulses wrt each other)
There was a problem hiding this comment.
After poking through the datasheet more, I'm noticing a couple of things
- the first example subtracts one from the time_on for some reason, but the later examples don't and it doesn't make sense to me - if time_on is 42 counts and your duty cycle is zero, you should stop (at least in theory, this seems like an edge case, see the next bullet) at 42 counts, not 42-1 = 41. Similarly, if time_on is 42 and your duty cycle corresponds to an on time of 1, you should stop at 43; stopping at 43-1=42 would be wrong.
- Examples 3 and 4 on page 18 of the datasheet detail what seem to be special cases for all-on and all-off. The datasheet also doesn't specify anything else about these cases - like, if the on and off times are the same, which one wins? It sounds like this is an undefined-behavior edge case, so we should use the special all-on and all-off cases (setting bit 12) instead
There was a problem hiding this comment.
That same argument applies to time_on as well, actually - if you want zero delay, why would you want to turn on at count=-1?
There was a problem hiding this comment.
Another note: The special case for fully-off doesn't actually involve bit 12 like I assumed. Full-on involves setting bit 12 on LED_ON, but full-off involves setting the two counts equal - which answers the question of who wins when time_on and time_off are the same
There was a problem hiding this comment.
This is fun
There's also a special case for fully off using bit 12 on page 20, bit 12 on LEDn_OFF overrides bit 12 for LEDn_ON.
…ome commented code is left behind and why certain bitmasks are the way they are
…the special ON/OFF cases
src/rov/rov_pca9685/src/PCA9685.cpp
Outdated
| } | ||
|
|
||
| void PCA9685::setDuty(uint8_t channel, float duty, float _delay) { | ||
| // uint16_t time_on = static_cast<uint16_t>(std::round(duty * 4096)); |
There was a problem hiding this comment.
After poking through the datasheet more, I'm noticing a couple of things
- the first example subtracts one from the time_on for some reason, but the later examples don't and it doesn't make sense to me - if time_on is 42 counts and your duty cycle is zero, you should stop (at least in theory, this seems like an edge case, see the next bullet) at 42 counts, not 42-1 = 41. Similarly, if time_on is 42 and your duty cycle corresponds to an on time of 1, you should stop at 43; stopping at 43-1=42 would be wrong.
- Examples 3 and 4 on page 18 of the datasheet detail what seem to be special cases for all-on and all-off. The datasheet also doesn't specify anything else about these cases - like, if the on and off times are the same, which one wins? It sounds like this is an undefined-behavior edge case, so we should use the special all-on and all-off cases (setting bit 12) instead
src/rov/rov_pca9685/src/PCA9685.cpp
Outdated
| } | ||
|
|
||
| void PCA9685::setDuty(uint8_t channel, float duty, float _delay) { | ||
| // uint16_t time_on = static_cast<uint16_t>(std::round(duty * 4096)); |
There was a problem hiding this comment.
That same argument applies to time_on as well, actually - if you want zero delay, why would you want to turn on at count=-1?
src/rov/rov_pca9685/src/PCA9685.cpp
Outdated
| } | ||
|
|
||
| void PCA9685::setDuty(uint8_t channel, float duty, float _delay) { | ||
| // uint16_t time_on = static_cast<uint16_t>(std::round(duty * 4096)); |
There was a problem hiding this comment.
Another note: The special case for fully-off doesn't actually involve bit 12 like I assumed. Full-on involves setting bit 12 on LED_ON, but full-off involves setting the two counts equal - which answers the question of who wins when time_on and time_off are the same
src/rov/rov_pca9685/src/PCA9685.cpp
Outdated
| } | ||
|
|
||
| void PCA9685::setDuty(uint8_t channel, float duty, float _delay) { | ||
| // uint16_t time_on = static_cast<uint16_t>(std::round(duty * 4096)); |
There was a problem hiding this comment.
This is fun
There's also a special case for fully off using bit 12 on page 20, bit 12 on LEDn_OFF overrides bit 12 for LEDn_ON.
src/rov/rov_pca9685/src/PCA9685.cpp
Outdated
| } | ||
|
|
||
| void PCA9685::setDuty(uint8_t channel, float duty, float _delay) { | ||
| // uint16_t time_on = static_cast<uint16_t>(std::round(duty * 4096)); |
|
@yameatmeyourdead please give this new implementation a test on the hardware :) |
your implementation works for all off and all on, but not any other duty. Performs a quick flash then turns off, attempting to set the same pwm does nothing |
JH backend was really obtuse and didn't have some nicities I wanted.
This fixes all of the gripes I had with JH's backend. Implements support for external clocks (need to uncomment and test obv).
Tested locally using continuous servo (esc w/ motor). Works just fine. All pca channels work
This does not touch the ROS-interfacing API so no other nodes should have to be touched for this to work. Yay compartmentalization.
I rebased on main so this doesn't have the same 30 commits that the last 12 PRs had holy crap this is ridiculous.