sync_read and bulk_read return vec of results

[omelia-iliffe]

The current behavior only captures one Err per call and returns that error even if other Dynamixels respond to the sync/bulk read.

The implementation that I have been using returns the much more verbose Vec of Results, one result for each Dynamixel responding, allowing for proper handling of each unresponsive Dynamixel in the sync/bulk.

Clippy is warning about very complex return types. Depending on what you think I could add some types to make the response types easier to read.

Result<Vec<Result<Response<Vec<u8>>, ReadError<T::Error>>>, crate::TransferError<T::Error>>
// becomes
type ResponseResult = Result<Response<Vec<u8>>, ReadError<T::Error>>
Result<Vec<ResponseResult>, crate::TransferError<T::Error>>

[de-vri-es]

Hey!

I've been thinking a bit about this type of multi-motor response functions. The current approach is not ideal: the simple function hides some error details from you, and the callback version takes away flexibility in how you write your code.

But I also don't really like returning a Result<Vec<Result<...>>. That is also kinda confusing.

We could do something slightly more complicated to implement, but hopefully a lot easier to use: return an object to represent the whole transaction, with functions to get the next response. We can also implement Iterator for that type then:

let mut response = client.sync_read(.....)?;
while let Some((motor_id, response)) = response.next() {
    let response = response?;
    // Do something with the response.
}

// Or with a for loop:
for (motor_id, response) in client.sync_read(...)? {
  let response = response?;
  // Do something with the response.
}

// Or collecting directly into a vector:
let responses: Vec<_> = client.sync_read(...)?.collect();

What do you think? I'm thinking we can remove the other functions then: no callback or Vec version, only this.

Added advantage is that is maps better to async code (having an sync version is a future goal I want to tackle somehow, some time).

[omelia-iliffe]

Yea cool, not something I had thought about but returning the Vec is definitely clunky and even more so if its a Vec of Results.
So are you thinking that the iterator calls would actually perform the calls to read_status_response when called or would you do all the reading in one for and return the iterator afterwards.
If the prior, what happens if the response iterator is never used? The responses to the sync/bulk_read will still be in the buffer and will cause out of sync issues the next time a read is called. Is using #[must_use] enough?

I've also been thinking about an async version and would love to spend some time developing it. I have been given some time over the next few months to contribute to oss projects :)

Also I've implemented a fast sync/bulk read but haven't tested it yet. This is my favorite crate to contribute to btw. I've learnt so much over the past year and can't really thank you enough!

[de-vri-es]

So are you thinking that the iterator calls would actually perform the calls to read_status_response when called or would you do all the reading in one for and return the iterator afterwards. If the prior, what happens if the response iterator is never used? The responses to the sync/bulk_read will still be in the buffer and will cause out of sync issues the next time a read is called. Is using #[must_use] enough?

I'm thinking the iterator will do the actual calls. It can finish reading any remaining responses in the Drop implementation.

This will not be so easy in a future async version (because we have no async drop), but that's a worry for then.

I've also been thinking about an async version and would love to spend some time developing it. I have been given some time over the next few months to contribute to oss projects :)

Right, I would recommend not to start on this without quite some discussion first. I want to find a way to not duplicate the whole codebase. But I think all solutions to that also have serious drawbracks. So it's best not to spend a lot of effort before we have a nice (or good enough) solution for this.

Also I've implemented a fast sync/bulk read but haven't tested it yet.

Sounds good! Always happy to get contributions. Just let me know when it's ready :)

This is my favorite crate to contribute to btw. I've learnt so much over the past year and can't really thank you enough!

That's great to hear :D I'm always happy when other people get involved with projects I maintain. It's nice to know people are actually using it.

[omelia-iliffe]

Right, I would recommend not to start on this without quite some discussion first.

Understood!

I'm thinking the iterator will do the actual calls. It can finish reading any remaining responses in the Drop implementation.

So I have started implementing a method that does this but can't return the type Response<&[u8]> as its borrowed from the StatusPacket owned by the current function.

EDIT: Nevermind I figure out a way, changing the From impl to use the data directly.

impl<'a> From<StatusPacket<'a>> for Response<&'a [u8]> {
	fn from(status_packet: StatusPacket<'a>) -> Self {
		Self {
			motor_id: status_packet.packet_id(),
			alert: status_packet.alert(),
			data: &status_packet.data[STATUS_HEADER_SIZE..],
		}
	}
}

I also discovered we can't impl Iterator<Item=Response<&'a [u8]> due to the lifetimes of 'a

Something related to this that I have been thinking about:
There are almost duplicate functions for u8, u16, u32 for every instruction. These types are also arbitrary as they don't necessary match the data type that is being read. ie I read a u32 for position but then convert it to a i32 as that the type the Dynamixel Ys use for position.

I wonder if you would be open to the idea of letting the user define the type at call time and have traits two traits for Reading and Writing that data type. The trait impl would have a const count and a method like to_bytes(self) -> &[u8] and try_from_bytes(&[u8]) -> Result<Self, _>.
We could add impls for the primitive types and [u8; N] and users could extend it further.

One major draw back is that users have to specify a type when calling (let pos: i32 = bus.read(1)), however we could impl methods to match the existing u8, u16, u32` and more.

[omelia-iliffe]

Hello, I've created a prototype for the SyncRead iterator and the Read / Write traits. Happy for some of this to get chucked out if its not the direction you think we should head. Just did it quickly so its all a bit rough at the moment.

I also moved the StatusPacket, Response, and InstructionPacketinto thepacketmod to reduce the clutter inbus.rsanddevice.rs`.

Somethings i'm not sure about:

• I've added a ParseError as the Read::try_from_bytes method is fallible, but its currently opaque. Doesn't give the actual reason the parse failed,
• To be able to implement iterator for the SyncRead the Data type has to be known when creating it. The only practical way to specify this is with a turbofish: bus.sync_read::<u8>, we can keep the existing methods as well bus.sync_read_u8
• You can still access the raw responses by calling SyncRead::next_raw which will return Response<&'a [u8]>
• I had initially returned (u8, Response<_>) with the u8 being the motor_id as you typed out above, before realising the motor_id is a field of the response

Todo:

• Decide on this approach
• Documentation
• Debug for SyncRead
• Migration of the rest of the methods (bulkread/write, syncwrite, regwrite)
• Do we want scan_cb to follow this iterator pattern as well?
• Name the Read/Write traits (maybe DataRead or Readable)