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

Automatically calculate proper read timeout for instructions. #12

Merged
merged 2 commits into from
Jun 17, 2024

Conversation

de-vri-es
Copy link
Member

This PR removes the manually provided read_timeout parameter, and instead uses the baud rate of the serial port and the message length to compute the expected response time.

The timing is based on the fact that each byte going over the serial line takes 1 start bit, 8 data bits and 1 stop bit.

The downside is that we now need to query the serial port for the baud rate in Bus::new() and Bus::with_buffers(), so they can return an error.

@omelia-iliffe: As part of this PR I removed serial_port_mut() again. It would allow to change the baud rate of the port, which makes the timeout computation inaccurate. Do you think there is an important enough use case for mutable access to the serial port that we should support it? Note that you don't need mutable access to read/write, only to configure the port.

@de-vri-es de-vri-es requested review from hgaiser and wddler May 20, 2024 12:33
@omelia-iliffe
Copy link
Contributor

I haven't been through this properly but the only reason I wanted serial_port_mut() is to change the underlying read_timeout. If the timeout is calculated automatically then I don't have any need to it and I can't think of anything else rn.
Looks pretty nice :)

@de-vri-es
Copy link
Member Author

I haven't been through this properly but the only reason I wanted serial_port_mut() is to change the underlying read_timeout. If the timeout is calculated automatically then I don't have any need to it and I can't think of anything else rn.
Looks pretty nice :)

Good to hear 👍

@hgaiser, @wddler: do you think either of you can review this any time soon? No need to go totally in depth, but some sanity checks and another pair of eyes at least :)

@de-vri-es de-vri-es force-pushed the automatic-timeout-calculation branch from 39feb43 to 21177be Compare June 3, 2024 11:33
Copy link
Contributor

@hgaiser hgaiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not super familiar with this code, but you have my 2 cents :).

dynamixel2-cli/src/bin/dynamixel2/main.rs Show resolved Hide resolved
dynamixel2-cli/src/bin/dynamixel2/main.rs Show resolved Hide resolved
dynamixel2-cli/src/bin/dynamixel2/main.rs Show resolved Hide resolved
dynamixel2-cli/src/bin/dynamixel2/options.rs Show resolved Hide resolved
src/bus.rs Show resolved Hide resolved
@de-vri-es de-vri-es merged commit c69967c into main Jun 17, 2024
1 check passed
@de-vri-es de-vri-es deleted the automatic-timeout-calculation branch June 17, 2024 18:45
@de-vri-es
Copy link
Member Author

Released as v0.9.0 together with the previous changes 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants