add a current_standby() method when the pin implements StatefulOututPin

[ripytide]

Let me know if there is a better name than current_standby

[rursprung]

thanks a lot for your PR!

please leave out the get_max_duty_cycle => max_duty_cycle renaming, this is related to the e-h 1.0.0-rc.1 => 1.0.0-rc.2 update. i realised that i didn't properly pin the version in Cargo.toml, thus you could pull in the newer RC version which wasn't compatible yet. i've just fixed this in #27, the update to rc.2 will follow once stm32f4xx-hal has a matching release (so that the example still works).

what is the reason for having current_standby? i'd have expected that the standby setting is driven by a business need (for lack of a better word) and the application knows that and thus always knows the state the standby pin should be in?

as a general suggestion:

• please make atomic commits
• please follow these recommendations for commit messages (also listed in the git book)

[rursprung]

see previous comment

[ripytide]

I've updated the commits to reflect on the changes to master.

This is more about consistency, the Motor has a current_drive_command() method yet their is no similar method for the current_standby().

[rursprung]

sorry for not having replied yet. i'm still contemplating whether it actually makes sense to have these methods (both current_standby as suggested by you as well as current_drive_command) - based on my previous argument the consumer should know the state, right? i added it here because i had the need to know the state when i built it and just decided on putting it in here rather than keeping it in the consumer.

what was the reason why you contributed this method? do you have a use-case which is being simplified by having this method?

[ripytide]

I used it as the owner of the current state when printing it back to the user. Alternatively as you say, if you are storing the current state somewhere else which you are using to control the Driver then I could just print that to the user instead and expect the Driver to always match it.

In fact, I now use the second method and no longer need this pull request. Perhaps you could also remove the current_drive_command() method which would also restore consistency.

[rursprung]

In fact, I now use the second method and no longer need this pull request. Perhaps you could also remove the current_drive_command() method which would also restore consistency.

i certainly want to have it consistent, i'm just unsure which option is better. but i guess it's not a big deal to have this method to make it easier for consumers.

i just realised an inconsistency: when i create the Motor i implicitly assume that the current state is that it's stopped, but i don't issue this as an actual command, so current_drive_command might differ from this depending on what was previously done with the pins. i think i should explicitly stop the motors on new (and possibly add a second new_with_drive_command so that it can be initialised with another setting if it shouldn't start in a safe state but e.g. continue from something else - though i think that can be added when somebody has that use-case), then it'd be guaranteed to match.
i found this when thinking about current_standby: initially we might not know the state if it has never been set. should Tb6612fng::new set the standby pin (and if so: enable or disable it?)? how does StatefulOutputPin::is_set_high respond if it hasn't been set so far? since the e-h docs don't mention it i presume that this might be implementation-specific?

[rursprung]

@ripytide: it'd be great to get your feedback on my previous comment!
i also saw that you pushed an update for e-h 1.0 RC.3 to this branch. 1.0 (no RC!) has just been released, so we can soon update to that, once there are matching releases from both embedded-hal-mock (open PR: dbrgn/embedded-hal-mock#105) and stm32f4xx-hal (open PR: stm32-rs/stm32f4xx-hal#723; needed for the example). i'd also prefer doing that update in a separate PR to keep the discussions separate, i hope that's ok for you?

[ripytide]

I didn't mean to add the version commit to this PR I was using my GitHub fork as a cargo git dependency while debugging some embedded_hal version issues.

As for your previous comment, I think it's more flexible for the library consumers to keep the internal state, including for standby, since even if they don't use it it doesn't hurt to have the functionality available. I would also agree that new should probably set outputs to some sane and documented defaults.

[rursprung]

great! then i'd propose to do the following in three separate PRs:

• update to e-h 1.0 (once the prerequisites have been merged & released)
• update new to set sensible defaults
• add current_standby (i only feel comfortable doing this after new has been modified to ensure that we return something sensible)

feel free to contribute any of the other two PRs besides this one! otherwise i'll try to get around to them at some point (though i'm a bit busy right now with other things, might take a week or so)

do you know the answer to this one?

how does StatefulOutputPin::is_set_high respond if it hasn't been set so far? since the e-h docs don't mention it i presume that this might be implementation-specific?

[ripytide]

I'll have a go at the pr's tomorrow.

StatefulOutputPin::is_set_high() in my use use of the esp32_hal the implementation reads the gpio registers to see if it is set high or low rather than keeping the state in a "rust" variable, though other implementations might do that. I think by default when you create an output pin it is created in the low position.

[ripytide]

@rursprung This PR and #35 should now be ready for review.

[rursprung]

in general, this looks good to me. as agreed, we'll first get #37 in and then merge this. when you rebase this (and adapt it to propagate the error) please also squash the commits

[ripytide]

@rursprung I've rebased and squashed this PR now that #37 is merged so this should be ready for review

[rursprung]

thanks @ripytide!