PWM: Add wrapper around RIOTs PWM-interface #38

Remmirad · 2023-02-08T12:52:53Z

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 · 2023-02-11T10:13:06Z

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 #37 in parallel.

chrysn

Thanks for adding this. It looks good over-all; I have several details and questions annotated across the source.

Quite a few of them might easily be explained by inspiration from existing code; I'd still hope to not continue on the bad examples I've set in the past.

src/pwm.rs

chrysn · 2023-02-14T17:32:42Z

src/pwm.rs

@@ -0,0 +1,186 @@
+use crate::println;


There's no good convention, but I think that the errors that now println should panic instead. Better crash than leave peripherals in a state they're not intended for.

(Also, should RIOT consider adding support for these?)

That certainly would be nice. Do you think we should suggest this and then build it into this PR or save it for later? Not sure how long it takes to make changes to the peripheral driver Interface since that would affect all boards?

Changed this to panic in 8d770fd. Hopefully it can be resolved but until that we have the panic.

I wouldn't wait for RIOT changes, the panics are fine now.

If it's something that can't easily be done for many boards, it may be that there'll be a peripheral feature this would be limited to ... or maybe it turns out it's just something impractical anyway and the embedded-hal API does something bad, don't know :-)

(Leaving this thread as unresolved not because it'll need addressing here, but because there's a potential follow-up issue to be created later on).

src/pwm.rs

chrysn · 2023-02-14T17:43:51Z

src/pwm.rs

+    frequency: u32,                       //Needed because of embedded_hal implementation
+}
+
+pub type Hz = u32;


Have you looked into whether embedded_time::Hertz could be a suitable type here?

Should work since it seems just to be an wrapper around u32. This would introduce a new dependency to the wrapper crate if I am not mistaken? But if thats ok i would use this type.

Adding dependencies is generally OK if they're good ones.

I've had a a closer look at embedded-time now, and it seem like fugit::rate::Hz would be the better candidate even.

(Neither has reviews at https://web.crev.dev/; maybe I'll manage a short one).

I've build something using fugit::HertzU32 in c94fc52 . I hope that suffices as a temporary solution until embedded_hal changes and all of this needs to be reviewed again.

If we think that this type is the right choice we might also add a pub use fugit in lib.rs

Using all of fugit would open up quite a bit of API surface (esp. if fugit people decide to split and keep compatibility re-exports in place), but a pub use fugit::HertzU32; in the PWM module would certainly be convenient.

src/pwm.rs

chrysn · 2023-02-14T19:34:35Z

src/pwm.rs

+        // Ignore if entry does not exists because
+        // only embedded_hal interface cares about those values
+        // and the implementation already checks for this
+        if channel < (CHANNELS) {


Over at .channels() we treat the PWMDevice as clipped to CHANNELS channels. For consistency, we should do this here as well, and err out without setting anything ... maybe even panic when using a channel that doesn't actually exist?

(Not sure on the best direction here).

Oh i just realized that there is a bit of a Problem. In the comments for PWMDevice i say its o.k. to just set CHANNELS=0 if embedded_hal is not used but in channels() I act as if CHANNELS would always be the count of available channels, which might not be the case!

I would try to change the api a bit:

Remove CHANNELS

Add cache_duty: bool to new/new_without_init

Setup the duty cache with riot_sys::pwm_channels if cache_duty

Change set_duty to check against channels

In addition I could insert a check using channels to set as you suggested to instead of RIOT failing at pwm_set give a "clean" Rust panic.

I don't quite get the proposal: How would, with CHANNELS removed, the values be stored for the getters?

The idea is to determine the length for the duty cache with riot_sys::pwm_channels in the new method instead of taking this length in via CHANNELS. We still have the cache array but determine its size in a different way.

This wastes a little bit of memory if not all of the available channels are used but might be worth the simpler API.

If a big array gets allocated all the time I think we should use it.

But how about we take the same approach as with the panicing functions, and offer CHANNELS as a way out?

Channel size report etc. would all work on the actual (RIOT-reported) number of channels, but getting the set PWM value would panic if CHANNELS is not big enough.

Thus, a user can decide whether to just take the zero-cost abstraction and forfeit the use of the accessor, or or get the fuller trait implementation at the cost of having [u16; CHANNELS] stored. (And thanks to monomorphization, all the "if channel > CHANNELS" checks, eg. for writing to the array in the setter, would be optimized out).

While making those changes i was wondering if we want to make a check on channels() in set_duty. The original check against CHANNELS has to go since, we want CHANNELS to be possibly 0. I am not sure whats best practice there, RIOT will probably fail in riot_sys::pwm_set in some way but a Rust panic might be easier to debug but then that will introduce checks to every set_duty call.

A PWM with different instances is nothing I think we should worry about. RIOT has no ownership concept and is free-for-all. When creating a Rust instance for a peripheral, the user states that they intend to use this exclusively. We can't allow any memory-unsafe effects when this is violated (for otherwise the constructors would need to be unsafe), but effects like getters getting confused sounds OK to me.

What's the problem with set_duty? It'd forward the write to RIOT, and only write to the cache if CHANNELS is large enough. (In performance critical situations that won't be a check at all, because there the CHANNELS would be monomorphized to 0, and every number is >= 0).

What bothered me was that in set_duty riot_sys::pwm_set gets called directly with the provided channel: u8. So if this is not a valid channel the underlying implementation of pwm might crash, since no error is returned. I was wondering if it might make sense to catch that case beforehand in set_duty (as seen here a8a9ea0) to allow for a possibly more understandable, Rust based error message.

If we decide we don't like the check after all I'll just remove it.

I've sampled three PWM implementations, and indeed they show the worst possible variation for nonexisting channel access:

cpu/nrf52/periph/pwm.c silently ignores such writes.

cpu/lpc23xx/periph/pwm.c places an assert, but C assertions are disabled, it silently ignores writes.

cpu/esp8266/periph/pwm.c places an assert, and if C assertions are disabled, it happily accesses the register array out-of-bounds, which may be a write to a completely unrelated register. Kaboom.

The (unstated?) goal of riot-wrappers is to be safe, zero-cost, and ergonomic / idiomatic. We can't reach this all the time, but we can try. The current solution is safe (as safe as we can be relying on RIOT's guarantees) but does a bounds check of unknown cost on each set.

An alternative that is a bit less easy-to-use but more performant would be to check the channel number against the const channel count at construction time [edit: a smaller channel number should be fine] -- we wouldn't get the full Rust typestate, but at least we could panic (or no-op, IMO either is fine) more cheaply.

I'd say that the current solution is good enough, especially given that it'll be up for some redesign with embedded-hal 1.1. If you want to explore alternatives, be my guest, but I'm fine with how it is right now.

In f89082d I tried an idea to use a separate PWMChannel struct which only holds a u8. set/set_duty now take this struct. Which can only be created from the PWMDevice::get_channel where we check against channels(). This would trade the check costs for a bit of memory.

What are your thoughts on that approach?

src/pwm.rs

chrysn · 2023-03-04T19:16:16Z

src/pwm.rs

+        // Ignore if entry does not exists because
+        // only embedded_hal interface cares about those values
+        // and the implementation already checks for this
+        if channel < (CHANNELS) {


I've sampled three PWM implementations, and indeed they show the worst possible variation for nonexisting channel access:

cpu/nrf52/periph/pwm.c silently ignores such writes.

cpu/lpc23xx/periph/pwm.c places an assert, but C assertions are disabled, it silently ignores writes.

cpu/esp8266/periph/pwm.c places an assert, and if C assertions are disabled, it happily accesses the register array out-of-bounds, which may be a write to a completely unrelated register. Kaboom.

The (unstated?) goal of riot-wrappers is to be safe, zero-cost, and ergonomic / idiomatic. We can't reach this all the time, but we can try. The current solution is safe (as safe as we can be relying on RIOT's guarantees) but does a bounds check of unknown cost on each set.

An alternative that is a bit less easy-to-use but more performant would be to check the channel number against the const channel count at construction time [edit: a smaller channel number should be fine] -- we wouldn't get the full Rust typestate, but at least we could panic (or no-op, IMO either is fine) more cheaply.

I'd say that the current solution is good enough, especially given that it'll be up for some redesign with embedded-hal 1.1. If you want to explore alternatives, be my guest, but I'm fine with how it is right now.

chrysn · 2023-03-04T19:21:45Z

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 c9cf3f2, otherwise I'll do it as part of the final review.

Remmirad · 2023-03-06T09:24:49Z

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

chrysn

We're down to nits, please go ahead with the test :-)

chrysn · 2023-03-16T19:14:18Z

src/pwm.rs

+    Unavailable,
+}
+
+/// A descriptor for a pwm channel


Suggested change

/// A descriptor for a pwm channel

/// A descriptor for a PWM channel

///

/// Note that these channels merely offer "soft" type safety, as they are not bound to a specific

/// channel. Using a channel on a different PWM device than the one it was created for may result in

/// panics, ignored PWM settings or nonsensical values, but it will still be safe.

I briefly considered whether we could make things any better, but I think not: We could store the PWM device and compare (but that'd be runtime overhead), and we could encode the PWM device in the type (but then we'd need to make the PWM device number an associated constant rather than a runtime parameter), so I think this is good.

src/pwm.rs

chrysn

Looks good to me, with just one more pending fix above. As this had some back and forth in the history, please squash it down (eg. one to add PWM, and one to add the test).

Doing a local test still, but then I'd merge that.

tests/pwm/Cargo.toml

tests/pwm/src/lib.rs

chrysn

Drilling down to why the test did not work (with which board did you test it?), I found an inconsistency here: resolution is passed in from the user (in the example, to 10), but then the setting interface (and also the example) assumes it's the full u16 range.

This is particularly problematic because there is an explicit get_max_duty that reports u16::max.

I see two possible ways forward:

Store the resolution (like the frequency is stored), and return it in get_max_duty.
Make the resolution a const generic parameter.

Given that with embedded-hal 1.0 we can do away with the other "needless" run time state, I have a slight preference for the former, but it's up to you.

Remmirad · 2023-03-17T11:03:21Z

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 · 2023-03-17T12:16:37Z

My tests were also disturbed by RIOT-OS/RIOT#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.

19402: sys/net/gnrc/netif: fixing no global address wait r=benpicco a=jan-mo ### Contribution description The function `gnrc_netif_ipv6_wait_global_address()` will always return true, even if no global address is attached to the interface. Currently the function only waits for any message and does not check if it was from the bus or not. So in `msg.content.ptr` is no valid address and therefore it returns true. I added just the check, if the message is from the bus of any interface and then checking the address. We could also first check if the address in `msg.content.ptr` is valid, but this will just hide the bug. Also the timeout was never checked. It was just assuming that no other message will be received during the wait. ### Testing procedure Use two devices, one works as a border router and supports the global address, the other will wait for the global address. You can call the function `gnrc_netif_ipv6_wait_global_address()` on the waiting node and see whether it returns true and finds the global address in the given time-range. 19404: sys/trickle: cleanup deps r=benpicco a=MrKevinWeiss ### Contribution description Cleans the dependencies of the `trickle` module. This removes the deprecated xtimer and models kconfig. ### Testing procedure Green murdock ### Issues/PRs references 19405: cpu/efm32: pwm_init errors are zeros r=benpicco a=chrysn ### Contribution description pwm_init is documented to return 0 on errors, and has an unsigned return value. EFM32's initialization function returned negative values, which through implicit casting become 0xffffffff or 0xfffffffe, which are successful by the documentation. This makes all the EFM32 error paths return 0 as they should. Also, it fixes a variable name and the corresponding error message that used to talk of "initiation" (which would be the start of a process) rather than "initialization" (which is a process that is completed before something else can happen). ### Testing procedure * on stk3700, tests/periph_pwm, run `init 0 0 10 1000` / `set 0 0 500` * The init used to respond with "The pwm frequency is set to 4294967294", and the set does nothing. * The init now responds with "Error: device is not <del>initiated</del><ins>initialized</ins>". The set still does nothing, but then one doesn't expect it to any more. (But really, looking at the patch and the docs should suffice). ### Issues/PRs references By-catch from testing the Rust wrappers provided by `@remmirad` at RIOT-OS/rust-riot-wrappers#38 Co-authored-by: Jan Mohr <jan.mohr@ml-pa.com> Co-authored-by: MrKevinWeiss <weiss.kevin604@gmail.com> Co-authored-by: chrysn <chrysn@fsfe.org>

Remmirad · 2023-03-20T09:52:03Z

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.

chrysn

Looks all as good as it gets with the current embedded-hal version, please squash!

Remmirad · 2023-03-21T07:46:53Z

Squashed.

chrysn reviewed Feb 14, 2023

View reviewed changes

Remmirad force-pushed the pwm-wrapper branch 2 times, most recently from 471ad72 to 49fb4d7 Compare February 23, 2023 09:06

chrysn reviewed Mar 4, 2023

View reviewed changes

chrysn reviewed Mar 16, 2023

View reviewed changes

chrysn approved these changes Mar 17, 2023

View reviewed changes

tests/pwm/Cargo.toml Outdated Show resolved Hide resolved

chrysn reviewed Mar 17, 2023

View reviewed changes

tests/pwm/src/lib.rs Outdated Show resolved Hide resolved

chrysn requested changes Mar 17, 2023

View reviewed changes

chrysn mentioned this pull request Mar 17, 2023

cpu/efm32: pwm_init errors are zeros RIOT-OS/RIOT#19405

Merged

chrysn reviewed Mar 20, 2023

View reviewed changes

Joel Aschmann added 2 commits March 21, 2023 08:42

PWM: Add wrapper around RIOTS PWM interface

ac6558d

PWM: Add test

5536eaf

Remmirad force-pushed the pwm-wrapper branch from dcd3218 to 5536eaf Compare March 21, 2023 07:45

-/// A descriptor for a pwm channel
+/// A descriptor for a PWM channel
+///
+/// Note that these channels merely offer "soft" type safety, as they are not bound to a specific
+/// channel. Using a channel on a different PWM device than the one it was created for may result in
+/// panics, ignored PWM settings or nonsensical values, but it will still be safe.

PWM: Add wrapper around RIOTs PWM-interface #38

Are you sure you want to change the base?

PWM: Add wrapper around RIOTs PWM-interface #38

Conversation

Remmirad commented Feb 8, 2023

chrysn commented Feb 11, 2023

chrysn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Remmirad Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Remmirad Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Remmirad Feb 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrysn Mar 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrysn Mar 4, 2023 • edited Loading

Choose a reason for hiding this comment

chrysn commented Mar 4, 2023

Remmirad commented Mar 6, 2023

chrysn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrysn left a comment

Choose a reason for hiding this comment

chrysn left a comment

Choose a reason for hiding this comment

Remmirad commented Mar 17, 2023 • edited Loading

chrysn commented Mar 17, 2023

Remmirad commented Mar 20, 2023

chrysn left a comment

Choose a reason for hiding this comment

Remmirad commented Mar 21, 2023

Remmirad Feb 23, 2023 •

edited

Loading

Remmirad Feb 23, 2023 •

edited

Loading

Remmirad Feb 18, 2023 •

edited

Loading

chrysn Mar 4, 2023 •

edited

Loading

chrysn Mar 4, 2023 •

edited

Loading

Remmirad commented Mar 17, 2023 •

edited

Loading