search

RIOT-OS / rust-riot-wrappers

The `riot-wrappers` crate, which enables high-level access to RIOT from the Rust programming language
13 stars 10 forks source link

PWM: Add wrapper around RIOTs PWM-interface #38

Open Remmirad opened 1 year ago

Remmirad commented 1 year ago

A first attempt at an wrapper around the PWM interface from RIOT. The wrapper tries to implement embedded_hal s interface as far as possible.

chrysn commented 1 year ago

Thanks for the PR. I've cleared it for a first CI run, but haven't yet had time to look at it. Given this needs a pwm_t at construction, it might be good to look at https://github.com/RIOT-OS/rust-riot-wrappers/issues/37 in parallel.

chrysn commented 1 year ago

As a module, I think this is ready -- with the small pub-use nit mentioned above, and depending on whether you want to explore the channel check more or not.

When you're done, please rebase onto a recent main and squash the commits -- I like multi-commit PRs, but only if they build things up, and this is mainly a big implementation with later fixes, so it can just as well be a single commit.

As with the DAC PR, I'll want to have a test around to ensure that the code is actually built during CI. Let me know whether you want to give that a try in the style of c9cf3f2195af4d8c3342ec11078d3d9dc21afa4d, otherwise I'll do it as part of the final review.

Remmirad commented 1 year ago

Once we're finished with the channels stuff I'll try to add a test for this.

Remmirad commented 1 year ago

Oh good point, seems I paid not enough attention to the resolution value. I executed this test only on native. I tested a similar thing before on the esp32c3 but with different values, where this might not have caused an error. The values in the test in its self need to be adapted too since they should never work. I will implement the first approach.

Regarding the squashing: during this PR I synced this branch with main to resolve merge-conflicts in lib.rs (because of dac I think). How should I treat this changes coming from main when squashing? I am not sure if I should just squash them into the Add PWM commit, or put them in a separate commit, ...

chrysn commented 1 year ago

My tests were also disturbed by https://github.com/RIOT-OS/RIOT/pull/19405 -- with that fixed, it's clearer what's going on.

As for the merge conflicts, given that this PR is not moving anything around or building on other code, I think it's easiest to just rebase everything onto current main.

Remmirad commented 1 year ago

I tried to fix the issues, with the resolution and the tests, if this is fine, I'll rebase and squash into two commits one for the wrapper and one for the test.

Remmirad commented 1 year ago

Squashed.