Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add PwmPin mock #52

Merged
merged 1 commit into from
Jan 6, 2023
Merged

add PwmPin mock #52

merged 1 commit into from
Jan 6, 2023

Conversation

rursprung
Copy link
Contributor

@rursprung rursprung commented Dec 4, 2022

this extends the Pin::Mock to also cover the PwmPin trait.

some observations on this commit:

  • i took the liberty of just hardcoding the PwmPin::Duty to be an
    u16 (wrapped in the PwmDuty type because it's used in other
    places here in the module as well). i don't see an easy way to make
    this configurable because i'd have to add it as a generic attribute to
    Pin and then it'd have to be defined by everyone using it
    (unnecessary for all non-PWM use-cases). but i think this should be
    acceptable as u16 probably covers most/all use-cases.
  • the current code features quite some code duplication (essentially the
    method check implementations are all the same for all setters / all
    getters). i've continued this for now but it might be worth a
    refactoring in the future (i haven't touched it because i'm not sure
    if there's some strategic decision behind this?)
  • there's a TransactionKind::is_get API for which i don't really see
    the reason (why not just check it directly?). i've decided to not copy
    that approach for the others as it'd IMHO just bloat the code and i'd
    instead suggest to remove TransactionKind::is_get at a later point.
  • PwmPin only exists on embedded-hal 0.x, it is currently missing
    from the planned 1.0 release. see Tracking issue for PWM traits rust-embedded/embedded-hal#358 for
    the discussion on this. once it has (hopefully) been re-added there a
    corresponding mock can be provided here also for 1.x.

rursprung added a commit to rust-embedded-community/tb6612fng-rs that referenced this pull request Dec 4, 2022
this contains some basic test coverage using `embedded-hal-mock` but
requires dbrgn/embedded-hal-mock#52 to support the `PwmPin` trait. for
the time being the PR branch is being used as a git dependency, this
should be changed back to a normal dependency once the PR has been
merged and a new release of `embedded-hal-mock` has been published.
@dbrgn
Copy link
Owner

dbrgn commented Dec 5, 2022

The PwmPin trait doesn't seem to exist anymore in e-h 1.0. Do you know what the replacement is, if any?

(Note: I'm happy to merge PRs for 0.2 as well, but that mock will become obsolete once e-h 1.0 is out.)

@rursprung
Copy link
Contributor Author

The PwmPin trait doesn't seem to exist anymore in e-h 1.0. Do you know what the replacement is, if any?

hm, i wasn't aware of that (never looked into e-h 1.0 so far). i'll have to dig into that (probably in the next few days).
i also just saw that you're testing against rust 1.31 and that doesn't contain u16::MAX yet (i just used the latest rust stable and didn't bother checking as i didn't expect to be using anything new). should i just use a hard-coded value there or do you want to lift the MSRV instead (which is a bit stupid because i'm only using it in a test and not in the runtime code :/)

@dbrgn
Copy link
Owner

dbrgn commented Dec 5, 2022

I think main is the "legacy branch", and if it's only for testing, I'd use a constant instead for now.

@rursprung
Copy link
Contributor Author

@dbrgn: i've updated the PR a while ago so that it now should build fine with rust 1.31. i've also pushed the e-h 1.0 ticket but so far no updates there. would it be ok for you to take this into 0.x for the time being? based on the e-h discussions it seems that some form of PwmPin should eventually land in e-h 1.x again (though i do hope that the discussion leads to it already landing in 1.0).

@rursprung
Copy link
Contributor Author

rursprung commented Jan 5, 2023

@dbrgn any chance that you could already review this for e-h 0.x while the e-h 1.x discussion is ongoing? as e-h 1.x will anyway look differently it'll require a different implementation here (if i don't forget i'll probably provide it here as well once a solution lands in e-h 1.x).
i'd like to release my motor driver (which would need this) but don't want to have a git-dependency in it (even if it's just for the test) - implicitly i'm also asking for a release of this crate after this PR gets merged 👼.
thanks!

@dbrgn
Copy link
Owner

dbrgn commented Jan 5, 2023

Sorry, I missed your original comment. Thanks for the reminder! I'll take a look tonight.

Copy link
Owner

@dbrgn dbrgn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments. Once these things are fixed, I can tag a new release soon!

src/pin.rs Outdated Show resolved Hide resolved
src/pin.rs Outdated Show resolved Hide resolved
src/pin.rs Outdated Show resolved Hide resolved
src/pin.rs Show resolved Hide resolved
this extends the `Pin::Mock` to also cover the [`PwmPin`][] trait.

some observations on this commit:
* i took the liberty of just hardcoding the `PwmPin::Duty` to be an
  `u16` (wrapped in the `PwmDuty` type because it's used in other
  places here in the module as well). i don't see an easy way to make
  this configurable because i'd have to add it as a generic attribute to
  `Pin` and then it'd have to be defined by everyone using it
  (unnecessary for all non-PWM use-cases). but i think this should be
  acceptable as `u16` probably covers most/all use-cases.
* the current code features quite some code duplication (essentially the
  method check implementations are all the same for all setters / all
  getters). i've continued this for now but it might be worth a
  refactoring in the future (i haven't touched it because i'm not sure
  if there's some strategic decision behind this?)
* there's a `TransactionKind::is_get` API for which i don't really see
  the reason (why not just check it directly?). i've decided to not copy
  that approach for the others as it'd IMHO just bloat the code and i'd
  instead suggest to remove `TransactionKind::is_get` at a later point.
* `PwmPin` only exists on `embedded-hal` 0.x, it is currently missing
  from the planned 1.0 release. see rust-embedded/embedded-hal#358 for
  the discussion on this. once it has (hopefully) been re-added there a
  corresponding mock can be provided here also for 1.x.

[`PwmPin`]: https://docs.rs/embedded-hal/0.2.7/embedded_hal/trait.PwmPin.html
Copy link
Contributor Author

@rursprung rursprung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for your review! i've fixed all your findings

src/pin.rs Outdated Show resolved Hide resolved
src/pin.rs Show resolved Hide resolved
src/pin.rs Outdated Show resolved Hide resolved
src/pin.rs Outdated Show resolved Hide resolved
Copy link
Owner

@dbrgn dbrgn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻 thanks for the updates!

@dbrgn dbrgn merged commit d435fd7 into dbrgn:master Jan 6, 2023
@dbrgn
Copy link
Owner

dbrgn commented Jan 6, 2023

implicitly i'm also asking for a release of this crate after this PR gets merged angel

@rursprung 0.9.0 is released!

@rursprung rursprung deleted the add-pwmpin-mock branch January 7, 2023 07:16
rursprung added a commit to rursprung/embedded-hal-mock that referenced this pull request Jul 2, 2023
this is the 1.0 equivalent of the old `PwmPin` trait (the mock for this
has been added in dbrgn#52).

this fixes dbrgn#73
rursprung added a commit to rursprung/embedded-hal-mock that referenced this pull request Jul 2, 2023
this is the 1.0 equivalent of the old `PwmPin` trait (the mock for this
has been added in dbrgn#52).

note that the test coverage is fully handled by the doc test which is
why there's no additional `mod test` in this file.

this fixes dbrgn#73
@rursprung rursprung mentioned this pull request Jul 2, 2023
tommy-gilligan pushed a commit to tommy-gilligan/embedded-hal-mock that referenced this pull request Jul 3, 2023
this is the 1.0 equivalent of the old `PwmPin` trait (the mock for this
has been added in dbrgn#52).

note that the test coverage is fully handled by the doc test which is
why there's no additional `mod test` in this file.

this fixes dbrgn#73
rursprung added a commit to rursprung/embedded-hal-mock that referenced this pull request Jul 6, 2023
this is the 1.0 equivalent of the old `PwmPin` trait (the mock for this
has been added in dbrgn#52).

note that the test coverage is fully handled by the doc test which is
why there's no additional `mod test` in this file.

this fixes dbrgn#73
rursprung added a commit to rursprung/embedded-hal-mock that referenced this pull request Sep 13, 2023
this is the 1.0 equivalent of the old `PwmPin` trait (the mock for this
has been added in dbrgn#52).

note that the test coverage is fully handled by the doc test which is
why there's no additional `mod test` in this file.

this fixes dbrgn#73
@rursprung rursprung mentioned this pull request Sep 13, 2023
rursprung added a commit to rursprung/embedded-hal-mock that referenced this pull request Sep 13, 2023
this is the 1.0 equivalent of the old `PwmPin` trait (the mock for this
has been added in dbrgn#52).

note that the test coverage is fully handled by the doc test which is
why there's no additional `mod test` in this file.

this fixes dbrgn#73
rursprung added a commit to rursprung/embedded-hal-mock that referenced this pull request Sep 17, 2023
this is the 1.0 equivalent of the old `PwmPin` trait (the mock for this
has been added in dbrgn#52).

note that the test coverage is fully handled by the doc test which is
why there's no additional `mod test` in this file.

this fixes dbrgn#73
cdunster pushed a commit to cdunster/embedded-hal-mock that referenced this pull request Sep 19, 2023
this is the 1.0 equivalent of the old `PwmPin` trait (the mock for this
has been added in dbrgn#52).

note that the test coverage is fully handled by the doc test which is
why there's no additional `mod test` in this file.

this fixes dbrgn#73
cdunster pushed a commit to cdunster/embedded-hal-mock that referenced this pull request Sep 19, 2023
this is the 1.0 equivalent of the old `PwmPin` trait (the mock for this
has been added in dbrgn#52).

note that the test coverage is fully handled by the doc test which is
why there's no additional `mod test` in this file.

this fixes dbrgn#73
rursprung added a commit to rursprung/embedded-hal-mock that referenced this pull request Nov 2, 2023
this is the 1.0 equivalent of the old `PwmPin` trait (the mock for this
has been added in dbrgn#52).

note that the test coverage is fully handled by the doc test which is
why there's no additional `mod test` in this file.

this fixes dbrgn#73
rursprung added a commit to rursprung/embedded-hal-mock that referenced this pull request Nov 5, 2023
this is the 1.0 equivalent of the old `PwmPin` trait (the mock for this
has been added in dbrgn#52).

note that the test coverage is fully handled by the doc test which is
why there's no additional `mod test` in this file.

this fixes dbrgn#73
cdunster pushed a commit to cdunster/embedded-hal-mock that referenced this pull request Nov 6, 2023
this is the 1.0 equivalent of the old `PwmPin` trait (the mock for this
has been added in dbrgn#52).

note that the test coverage is fully handled by the doc test which is
why there's no additional `mod test` in this file.

this fixes dbrgn#73
dbrgn pushed a commit that referenced this pull request Nov 23, 2023
This is the 1.0 equivalent of the old `PwmPin` trait (the mock for this
has been added in #52).

Note that the test coverage is fully handled by the doc test which is
why there's no additional `mod test` in this file.

This fixes #73.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants