hannobraun / stepper

Universal Stepper Motor Interface
Other
107 stars 17 forks source link

replace embedded-time with fugit #142

Closed ahdinosaur closed 2 years ago

ahdinosaur commented 2 years ago

here's a pull request to close https://github.com/braun-embedded/stepper/issues/141 :smile_cat:

def needs a review, as i'm not sure if this is the best approach. :shrug:

notable changes:

:sunny:

ahdinosaur commented 2 years ago
1. That commit you mentioned as potentially problematic, the one that adds a `TIMER_HZ` parameter to `EnableMotionControl`, why does it need to be there in the first place? I don't see the parameter used anywhere.

if we remove TIMER_HZ from EnableMotionControl here: https://github.com/braun-embedded/stepper/blob/4a481e06370d092555e6c7d57029ca6ddb9eec6d/src/motion_control/mod.rs#L411-L431

then the compiler errors with:

error[E0207]: the const parameter `TIMER_HZ` is not constrained by the impl trait, self type, or predicates
   --> src/motion_control/mod.rs:413:45
    |
413 | impl<Driver, Timer, Profile, Convert, const TIMER_HZ: u32>
    |                                             ^^^^^^^^ unconstrained const parameter
    |
    = note: expressions using a const parameter must map each value to a distinct output value
    = note: proving the result of expressions other than the parameter are unique is not supported

and we can't remove TIMER_HZ altogether as we need it in the where clause.

but maybe there's a better way. :shrug:

2. I don't have a test setup at the ready, so while I can say that this change looks good to me, I can't confirm using a real use case. I you're okay with this change from a user's perspective, I'm happy to defer to your judgement.

all i can say is it works for me. :smiley_cat:

3. The CI build is failing. Looks like you missed an example in a doc comment.

yes, i also need to update the docs to reflect these changes.

hannobraun commented 2 years ago

if we remove TIMER_HZ from EnableMotionControl here:

...

but maybe there's a better way. :shrug:

Ah, got it. I can't think of a better way either. Maybe we'll figure out something better at some point, but this is good enough for now.

all i can say is it works for me. :smiley_cat:

Good enough :+1:

yes, i also need to update the docs to reflect these changes.

Sounds good! Happy to merge once the CI build is green.

valorekhov commented 2 years ago

yes, i also need to update the docs to reflect these changes.

Perhaps, these are the doc updates you are looking for? I've been doing work on a generic driver which can drive various motors by bit-banging 1..4 lines. The base commits for the work were from this PR and I needed to make the following changes to make cargo test to pass. I also updated embedded_hal alpha to the latest version.

I can send a PR with the same to either repo.

hannobraun commented 2 years ago

Thank you, @valorekhov! Looks good!

Both options, you sending a PR to @ahdinosaur to complete this pull request, or you submitting a new PR with all changes to this repository, are fine with me!

hannobraun commented 2 years ago

Oh, so this gets marked as merged automatically. Neat!

(I merged #143, which contained the commits from here.)