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

add Device/Peripheral api #17

Merged
merged 8 commits into from
Oct 12, 2024
Merged

Conversation

omelia-iliffe
Copy link
Contributor

Added Device to allow a full dynamixel network to be built.
Moved low level communication to to a shared private struct Messenger

Not attached to any of the names.

Currently the methods like open, new, with_buffers etc are basically duplicated for Bus and Device. I wonder if we should create a builder type or make Messenger public and add methods for building it into a Bus or Device? What are your thoughts? Could also be a macro but that feels clunky.

I also added an integration test with MockSerialPort, I thought it would be useful.

…ructionPacket`.

Moved shared reading/writing to `Messenger`,
added tests `MockSerialPort` for testing
@omelia-iliffe
Copy link
Contributor Author

I had to make the endian module pub to use the fns in the tests. Not sure if this is desired or if we replace it with an external crate like byteorder

}

/// Read a single [`InstructionPacket`].
pub fn read_instruction_packet_timeout(&mut self, timeout: Duration) -> Result<InstructionPacket, ReadError<T::Error>> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only having this function to read isn't very ergonomic. Ideally there would be a few for different use cases:

  • blocking for some time and timing out, current behavour
  • blocking forever until a packet is returned (timeout code be None)
  • returning immeditialy if there is no packet (could use crate nb, common in the embedded world)
    I'm also trying to figure out how this could work with interrupts on an MCU

Copy link
Member

Choose a reason for hiding this comment

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

Only having this function to read isn't very ergonomic. Ideally there would be a few for different use cases:

* blocking for some time and timing out, current behavour
* blocking forever until a packet is returned (timeout code be `None`)

This sounds good to me!

* returning immeditialy if there is no packet (could use crate [nb](https://crates.io/crates/nb), common in the embedded world)
  I'm also trying to figure out how this could work with interrupts on an MCU

I don't want to use the nb crate. I think it just adds an unnecessary layer of complexity.

I prefer the approach taking by std where you can put a socket in non-blocking mode with a member function and functions will start returning Err(WouldBlock).

However, this is something that serial2 doesn't support right now. That needs to be added either way, to support non-blocking reads/writes. Or we need to allow the serial port to return an error when configuring non-blocking mode (I think this is necessary either way, because there will probably be syscalls involved).

@de-vri-es
Copy link
Member

Added Device to allow a full dynamixel network to be built. Moved low level communication to to a shared private struct Messenger

Not attached to any of the names.

Cool!

Currently the methods like open, new, with_buffers etc are basically duplicated for Bus and Device. I wonder if we should create a builder type or make Messenger public and add methods for building it into a Bus or Device? What are your thoughts? Could also be a macro but that feels clunky.

I'm fine with the duplication. In this case I would prioritize ease-of-use over the desire not to duplicate anything. So that means not exposing the Messenger type in my mind.

And I agree macros are too clunky. They make it more difficult for other contributors (and also for me in a year from now :p ) to understand the code base.

I also added an integration test with MockSerialPort, I thought it would be useful.

Cool!

@de-vri-es
Copy link
Member

I had to make the endian module pub to use the fns in the tests. Not sure if this is desired or if we replace it with an external crate like byteorder

Endianness handling is simple enough with std that I don't want to use a dependency for it. I'd rather duplicate read_u16_le than make it public though.

But, even better would be an API that fully parses instruction packets into Rust structs. So instead of having a InstructionId enum we could have a Instruction enum that holds parsed instructions. But if you want this can be something for a follow-up PR, to keep this one more manageable (and quicker to get merged).

@omelia-iliffe
Copy link
Contributor Author

omelia-iliffe commented Sep 24, 2024

Sweet!
I've removed the instruction_id enum and added a Instructions enum, there is also the Instruction struct that wraps the Instructions with an id (instead of putting the id in every varient).
I also extended the InvalidParameterCount to add ExpectedCount::Min.

The Instruction struct is generic over either &[u8] and Vec<u8> which allows you to be returned owned data but I don't think this adds anything so It could be removed. Users could call to_owned on the parameter sliced if they want to own the data.

Otherwise I'm pretty happy with how this feels now.

I left the timeout as is for now. I think if we were to make it an Option<Duration> then we would have to bring that through into the transport trait as well. I briefly looked at unwrapping the Option with unwrap_or(Duration::Max) but of course that overflows and panics when the expected package timeout is added to it.

I saw in the async pull request the github checks were updated to test a matrix of the different features. This is something that needs doing for this pr but I'm not very familiar with github actions. Would you be able to do that?

@de-vri-es
Copy link
Member

Sorry for the long wait. I've been really busy with non-digital things :(

Anyway, looks good! I've started on making some adjustments on the bike-shedding branch. I'll merge this one now, and then I'll open a PR so you can give feedback on my adjustments too.

@de-vri-es de-vri-es merged commit 0a272c5 into robohouse-delft:main Oct 12, 2024
1 check passed
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.

2 participants