changed functions to return a response struct,

[omelia-iliffe]

This allows for the reading of the alert bit to check for hardware errors
Key changes:

• Renamed Response struct to StatusPacket
• Added a new generic struct called Response
  • This contains the motor_id, some generically typed data, an alert bit for checking hardware errors, and an optional address used for bulkdata reads
• changed all pub functions to return a Response
• changed sync_read and bulk_read functions to return the Response Struct, instead of SyncData/BulkData

A few things I'm unsure about at the moment:

1. the read function still returns a StatusPacket struct, its the only pub fn to do so.

   • I left it this way to eliminate an extra memory allocation:
   • ie when read_u8 is called, I didn't want to have read creating a Response<Vec<u8>> for it just to be converted into a Response<u8>.
   • This means, however, that if read is called by the user is returns a StatusPacket.
   • I might add a private _read function to get around this issue.

2. The action, clear, factory_reset, and reboot functions return Result<Option<Response<()>>, crate::TransferError>

   • The option is to due to the broadcast function it calls,
   • It might be better to panic! if motor_id == 254 and instead only allow dedicated broadcast functions to be used.

3. The address: Option<u16> field of the Response struct is only used for bulk_read

   • I wanted to simply the number of structs being returned but as the address field is only used to this it makes more sense to create a new struct.

[omelia-iliffe]

Solved Point 1 with a private _read function
Solved point 3 with a struct BulkResponse which wraps Response and adds the needed address field

[omelia-iliffe]

Also flagging that I need to update the CHANGELOG with the changes from this and the previous PR

[de-vri-es]

The PR is looking good! Thanks!

This allows for the reading of the alert bit to check for hardware errors Key changes:

• Renamed Response struct to StatusPacket

Perfect!

• Added a new generic struct called Response

  • This contains the motor_id, some generically typed data, an alert bit for checking hardware errors, and an optional address used for bulkdata reads

  • changed all pub functions to return a Response

  • changed sync_read and bulk_read functions to return the Response Struct, instead of SyncData/BulkData

👍

A few things I'm unsure about at the moment:

1. the read function still returns a StatusPacket struct, its the only pub fn to do so.
   • I left it this way to eliminate an extra memory allocation:
   • ie when read_u8 is called, I didn't want to have read creating a Response<Vec<u8>> for it just to be converted into a Response<u8>.
   • This means, however, that if read is called by the user is returns a StatusPacket.
   • I might add a private _read function to get around this issue.

👍

We could still create a BorrowedBytes struct that internally keeps a StatusPacket alive and derefs to [u8] to allow a public version of read() that doesn't allocate.

However, it would have to have the same generic parameters as the StatusPacket struct, so it wouldn't be the nicest looking type in a public interface.

2. The action, clear, factory_reset, and reboot functions return Result<Option<Response<()>>, crate::TransferError>

   • The option is to due to the broadcast function it calls,
   • It might be better to panic! if motor_id == 254 and instead only allow dedicated broadcast functions to be used.

I like this idea. Although instead of a panic we could also just not check for it. Then the user will get a timeout on the response. I'm on the fence which is better.

We could also add a strongly typed UnicastMotorId struct, but I'm not a huge fan of that either. It's a lot more complicated in use compared to a simple integer.

3. The address: Option<u16> field of the Response struct is only used for bulk_read

   • I wanted to simply the number of structs being returned but as the address field is only used to this it makes more sense to create a new struct.

Solved point 3 with a struct BulkResponse which wraps Response and adds the needed address field

👍

As mentioned with an in-line comment, we could also just remove the address field entirely. It's user input, so the caller doesn't really need it.

[omelia-iliffe]

Re point 2
yea lets just not check and let it timeout. thats how the ping function is implemented. can always change it later :)

[omelia-iliffe]

I think that's everything. Ready to merge once you've had a look over :)

[de-vri-es]

Looks good! There's still the issue of TryFrom vs From and the option of splitting StatusPacket::error() and StatusPacket::alert().

We can merge the PR without these, but I want to resolve them before a new release (since both are the public API).

[omelia-iliffe]

see my inline comments re the TryFrom vs From
I have replaced the use of error() & 0x80 with alert() now :)

[de-vri-es]

see my inline comments re the TryFrom vs From
I have replaced the use of error() & 0x80 with alert() now :)

I don't see any inline comment? Maybe you started a review instead of posting a single comment. Atleast that happens to me sometimes :)

[omelia-iliffe]

Can you see it now? Sorry newish to github's PR system.

Here's what I wrote anyway:

@omelia-wetaworkshop omelia-wetaworkshop Dec 14, 2023

Would this ever actually panic? The responses are only converted into the primitive types inside the read_u8, ... functions. If the StatusPacket has passed the crc check it should have the correct number of bytes to safely convert to the primitive. Or is that not correct?

@omelia-wetaworkshop omelia-wetaworkshop Dec 14, 2023
its also checked in the read_raw function

[de-vri-es]

Can you see it now? Sorry newish to github's PR system.

No problem. I can see them now! I replied in that thread again :)

[de-vri-es]

If the StatusPacket has passed the crc check it should have the correct number of bytes to safely convert to the primitive. Or is that not correct?

Although the CRC check doesn't necessarily mean the message has the right format. If the other side of the communication is misbehaving, it could arrive with a wrong format but valid CRC.

Although that is not a common case, we should still not panic in such situations but allow the user code to deal with it (they may panic if they want).

[de-vri-es]

I hope you don't mind, I pushed a few commits to your branch. I want to release this before going on holiday (which is very soon) :)

I switched to TryFrom, and I actually found one place with a missing check on the parameters count!

I also changed bulk_read_cb(), which now passes the original BulkReadData command to the user callback. For the function that returns data, it's still up to the user to remember their commands.

[de-vri-es]

Released as v0.6.0! \o/

Thanks again for the PR. This was a pretty important fix!

Btw, if you do something cool and not secret with servos using this crate, I would love to hear about it. I would also love to add some example projects to the README.md :D

[omelia-iliffe]

Fantastic, Thanks for finishing all that off!

Certainly, I would love to add some examples to the ReadMe. Ill open a PR when I have some that are cleared from secrecy ;)

[de-vri-es]

Cool! I would love to know how it's being used :D

And thanks again for the PRs! Don't hesitate to send more if you have more improvements!