Conversation
EvanBottango
left a comment
EvanBottango
left a comment
There was a problem hiding this comment.
any idea on license of pwm servo? Also see comments on the us to deg conversion
| #ifdef ESP32 | ||
| #include <ESP32Servo.h> | ||
| #elif defined(CORE_TEENSY) | ||
| #include <PWMServo.h> |
There was a problem hiding this comment.
I can't figure out the license on this library? It doesn't have a license file at https://github.com/PaulStoffregen/PWMServo
There was a problem hiding this comment.
Its LGPL. Look at the comment header in the PWMServo.h file.
| if (currentSignal != targetSignal) | ||
| { | ||
| #ifdef CORE_TEENSY | ||
| servo.write(180 * (targetSignal - 544) / 1856); |
There was a problem hiding this comment.
Some things give me misgivings here:
-
544 and 1856 are pretty magic number-y? Maybe a define?
-
I get what you're doing with the lerp to convert us to angle. But I'm sad that this is 10x less resolution in us pulse length than directly writing us. Especially since this will just eventually go back to us pulse length on the backend. I guess this is more of a critique of the library than you, but it makes me have reservation of suggesting it as the default. By my math with the defaults here, 1 deg == about 10us.
A possible mitigation would be to scale the values at attach and then resultantly the lerp as well?
There was a problem hiding this comment.
Never mind to scaling, realized that was a dumb idea 2 mins later. I'm not sure there's a way to get back the lost resolution when the only call is to write between 0 and 180 deg.
There was a problem hiding this comment.
I agree, it's frustrating that PWMTeensy doesn't have a writeMicroseconds call available. Should this be something that requires the user to change BottangoArduinoConfig.h, rather than a default for the teensy family of devices?
There was a problem hiding this comment.
Sorry for delay, ya this should be an option to enable (off by default) in the config file, rather than just on.
Also, looks like "TEENSYDUINO" is the more appropriate guard than CORE_TEENSY?
There was a problem hiding this comment.
I'll work on moving it to a configurable option!
As for the guards, CORE_TEENSY is the more common one used within Teensy internal libraries. The difference is TEENSYDUINO is provided from compiler flags, and CORE_TEENSY is provided from Arduino.h. But in the context of the Bottango driver, either would work.
|
Dang, I really did just forget about this PR, huh? That's my bad and I apologize. I have added the guard to BottangoArduinoConfig.h like we talked about :) |
|
@EvanBottango any update on this? |
|
@natekfl Hi, sorry for the slowness here. I am still working on a major update and have been for a while, and will work to merge these changes into that branch once it's close to done. I'll also take a look at your changes asap. |
2 participants
No description provided.