I suspect you could simplify your error definitions by defining a single embedded_hal::digital::ErrorType for all of the pins and assume they all have the same error type. It would be exceptionally weird if they didn't. And even in the edge case where i.e. some pins are on a GPIO expander and others are on the MCU, a user could make their own error enum to wrap them up into a single type expression. This might aid in usability of the driver.
Same for the above with PWM errors

I'm not sure I understand this suggestion, are you suggesting we create a catch-all error type? Such as:

struct Error;
impl Tb6612fng {
    fn drive(..) -> Result<(), Error>;
}

This has already been discussed as being less flexible due to the scenario you described as having pins with different error types. How would a user create their own wrapping error types though? Would that not require a generic return type? Such as:

impl Tb6612fng {
    fn drive<E: embedded_hal::digital::ErrorType>(..) -> Result<(), E>;
}

This seems harder to use since it adds trait bounds to the drive() function which can cause hard to diagnose trait bound compile errors.

DriveCommand specifies a speed in percentage, but the stored value is a u8. Are they supposed to use 0 for 0% and u8::MAX for 100%?

This is since we pass the u8 to embedded_hal::SetDutyCycle::set_duty_cycle_percent(speed: u8), this could be better documented on our side though about 0-100 being required. Possibly, we could also switch to a DrivePercent and DriveFraction enum so a user could pick which method they wanted to use (SetDutyCycle::set_duty_cycle_percent() being the other possible method).

Beware exposing pub fields in Tb6612fng. Read through https://rust-lang.github.io/api-guidelines/future-proofing.html#structs-have-private-fields-c-struct-private for an analysis of why. It recommends exposing the fields as getter methods.

Switching to getting mutable motor references via getters also has some drawbacks such as preventing multiple mutable references otherwise allowed:

let driver = Tbb6612fng::new(...);

//no borrow checker issue
takes_mutable_reference(&mut driver.motor_a);
takes_mutable_reference(&mut driver.motor_b);

//borrow checker issue
takes_mutable_reference(driver.motor_a_mut());
takes_mutable_reference(driver.motor_b_mut());

As it stands, your constructor just takes a whole bunch of pins. This could make it easy to get them mixed up. Maybe a builder pattern would be better to ensure the pins are going to their proper definitions? Or have the Tb6612fng take in two Motors` instead.

Good point, Tb6612fng::new(motor_a: Motor, motor_b: Motor, stby: STBY) sounds much nicer to use.

thanks a lot for your review! sorry for not reacting on it earlier, but i was away!

I would suggest having a look at the checklist here.

i had already done that, thanks!

One thing I noticed is that the dependency to embedded-hal-mock can be customized so that this does not depend on embedded-hal 0.2 with:
`embedded-hal-mock = {version = "0.11", default-features=false, features=["eh1"]}

thanks! i've changed it now and also raised dbrgn/embedded-hal-mock#122

• I suspect you could simplify your error definitions by defining a single embedded_hal::digital::ErrorType for all of the pins and assume they all have the same error type. It would be exceptionally weird if they didn't. And even in the edge case where i.e. some pins are on a GPIO expander and others are on the MCU, a user could make their own error enum to wrap them up into a single type expression. This might aid in usability of the driver.
• Same for the above with PWM errors

as per @ripytide's comment i don't thinkt hat this simplification would actually work (or simplify anything)?

• DriveCommand specifies a speed in percentage, but the stored value is a u8. Are they supposed to use 0 for 0% and u8::MAX for 100%?

the comment on the enum says that it's "in percentage" - isn't that enough to show that it's in the 0..100 range? (since it's u8 it can't be 0..1, so that confusion at least is avoided). pulling in something like bounded-integer (or hand-rolling it) seems like an overkill here?

• Beware exposing pub fields in Tb6612fng. Read through https://rust-lang.github.io/api-guidelines/future-proofing.html#structs-have-private-fields-c-struct-private for an analysis of why. It recommends exposing the fields as getter methods.

i am aware that it's not recommended to have pub fields, but as pointed out by @ripytide it makes it so much more convenient to use it since you need mut access to the Motor to use it.

• As it stands, your constructor just takes a whole bunch of pins. This could make it easy to get them mixed up. Maybe a builder pattern would be better to ensure the pins are going to their proper definitions? Or have the Tb6612fng take in two Motors` instead.

thanks, you're right! i've changed this now