Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Servo set max pulse width math is incorrect #9

Open
johnn01 opened this issue May 25, 2020 · 7 comments
Open

Servo set max pulse width math is incorrect #9

johnn01 opened this issue May 25, 2020 · 7 comments
Labels
help wanted Extra attention is needed

Comments

@johnn01
Copy link

johnn01 commented May 25, 2020

    /// Set the servo's maximum pulse width
    pub fn set_max_pulse_width(&mut self, value: u64) {
        if value >= self.frame_width {

The default frame_width is set to 20, while the default max_pulse_width is 2000. However, my servo goes to 2100. I'm unable to change this because as seen above, 2100 >= 20.

To work around this, I have to set frame_width to something greater than 2100, increase max_pulse_width to 2100, and then decrease frame_width back to 20.

@rahul-thakoor rahul-thakoor added the help wanted Extra attention is needed label Jun 17, 2020
@rahul-thakoor
Copy link
Owner

I will have to look into this. Thanks

@i-jey
Copy link

i-jey commented Jul 17, 2020

Hi there, just took a look at this and made a PR. @rahul-thakoor

Cheers!

@after-ephemera
Copy link

The particular issue mentioned originally has to do with the way we compare frame width (milliseconds) to max and min pulse width (microseconds). Working on a PR to fix it.

@rahul-thakoor
Copy link
Owner

hey @azjkjensen

so we should keep

                    min_pulse_width: 1000,
                    max_pulse_width: 2000,

? ie #14 shouldn't be merged but #18 merged instead?

@after-ephemera
Copy link

After some more investigation, actually neither. If we go the #14 route and use ms, the struct fields for max_pulse_width, min_pulse_width, and frame_width should all be changed to float types. These values for some servos (such as https://servodatabase.com/servo/towerpro/sg90) require more granularity.

Alternatively, we can use microseconds for these fields instead, diverging from the Python library but potentially remaining more performant and Rust-friendly. Defaulting these values to microseconds would allow us to continue to use unsigned types, avoiding floating-point arithmetic for each operation.

@rahul-thakoor
Copy link
Owner

hey, @AldaronLau any thoughts on this?

@AldaronLau
Copy link
Contributor

@rahul-thakoor @azjkjensen How about instead of deciding between floating-point milliseconds and integer microseconds, we use the Duration type from the standard library? It seems like the perfect fit for this situation (it also avoids floating point, and in my opinion is the more Rust-friendly to do things - and not too divergent from the Python library).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants