Implement full embedded_hal::Pwm

[justacec]

This change implements the full embedded_hal::Pwm and embedded_hal::PwmPin API defined at

Trait	embedded_hal Documentation Link
Pwm	https://docs.rs/embedded-hal/0.2.3/embedded_hal/trait.Pwm.html
PwmPin	https://docs.rs/embedded-hal/0.2.3/embedded_hal/trait.PwmPin.html.

Motivation

This PR was prompted by the desire to be able to adjust the frequency of the timer after construction through the set_period() and get_period() Pwm trait methods. The changes were done in such a way as to maintain compatibility with the current API.

Coding Concept

The primary idea was to shift the current stm32f1xx_hal::pwm::Pwm struct to be PwmChannel and then devise a new Pwm struct which held the channels. While this sounds potentially not necessary, it was required in order to have a landing space for the set_period and get_period methods in the embedded_hal::Pwm trait. Implementing this method on an individual channel is not preferred because the operation adjusts the Prescaler (PSC) and Auto Reload Register (ARR) of the entire timer which affects all the channels, not just one. Implementing the desired method on the PwmChannel struct could lead to unintended consequences.

Time Units

The embedded_hal API requires that a time based type be defined for the Pwm trait. This is difficult as the natural timescale for the MCU is in fractional seconds of high order. If one wanted to use milliseconds (the currently defined time unit in the stm32fxx_hal crate) periods greater than 2 seconds could not be specified because of the 32-bit field depth. If one was to try to use microseconds, there could be difficulty getting particular frequencies because of the integer nature of the unit

Because of this, I have defined the time unit in the Pwm trait to be Hertz. I do realize this is not a "time". This unit is just a bit easier to work with. In order to make it so end users can pass time like units, I have implemented Into<Hertz> for both MilliSecond and MicroSecond this allows for adjusting the period like the following:

mypwm.set_period(1.ms());

Compatibility

In order to maintain compatibility with the previous API, I have made Pwm Deref and DerefMut in such a way that it returns a reference to the channels. I have further made PwmChannel Clone and Copy. This is safe as the PwmChannel does not occupy any space as it is a struct holding nothing but PhantomData elements. The struct and its fields carry only type information.

Random Thoughts

1. I have been calling the channel in this PR as PwmChannel, but the embedded_hal API calls it PwmPin. Should we use PwmPin here? I like channel as that is the way that the ST documentation has it. Also, different channels could point to different pins. With that thought I might suggest that the embedded_hal API be updated/adjusted to match this nomenclature.

Potential Improvements

While coding this up I recognized a few potential improvements but decided to not implement them right now in order to get a working base. Below is a list of these ideas which were not implemented:

Array based Channels

Currently, the channels are held in a compile time tuple with length specified by the number of channels in the PINS object. Indexing these channels is then done by their order in the tuple. This results in cases where the timers C3 channel is indexed as 0, not 3. It might be more clear if the PwmChannel objects are stored as Option in a fixed length array of size 5 (leave the first entry as a None so the numbers are more logical when compared with the ST manuals). That way if C3 is the only pin defined, it is accessed by:

mypwm.3

In the event that a user asks for a pin that was not initialized, he/she would get a None and would need to handle that situation.

[burrbull]

LGTM.

cc @geomatsi

[justacec]

@burrbull There is still an existing issue with a compiler warning on the uninit memory. I was not sure how to tell the compiler to disreguard that issue. Do you know how? I know that @TheZoq2 would like to see a solution to that.

[justacec]

I am not seeing that warning in the TravisCI build logs. very strange.

[burrbull]

This is because you've added clk: Hertz to Pwm.
You can just use #[derive(Clone, Copy)] on Pwm instead of implement Clone and Copy manually.

But I don't understand sense Deref.

[TheZoq2]

Thanks for the PR. Nice to see such well documented code and description!

I left some minor nitpicks in the comments, as well as a note about the Copy and Clone implementations probably being unsound.

I have been calling the channel in this PR as PwmChannel, but the embedded_hal API calls it PwmPin. Should we use PwmPin here? I like channel as that is the way that the ST documentation has it. Also, different channels could point to different pins. With that thought I might suggest that the embedded_hal API be updated/adjusted to match this nomenclature.

I think both names are fine so we can probably keep it like this

Array based Channels

I see your point here, but I don't think arrays is the answer as that would add a potential for runtime errors which we are trying to avoid throughout this crate.

A better option might be to always return a tuple of length 4, but with () or struct NoChannel in the unused slots.

[justacec]

@burrbull, The Clone / Copy is needed against the PwmChannel. I did try to put the derive directive above the PwmChannel struct definition, but that did not seem to work and just produced errors. It was not until I manually implemented Clone that everything worked.

What is your specific question concerning Deref?

[TeXitoi]

Great work! I'll use it for playing music on my alarm clock! (doing it manually using unsafe now)

[burrbull]

@justacec See this draft burrbull@42acde3

[justacec]

@burrbull @TheZoq2 @TeXitoi Ok, I believe that I have merged all of the efforts in the other pull request (#177) to this pull request. I think that I might have some concerns about how the channels are dereferenced by the split result. I will expand on this a bit later.

[justacec]

Ok, so, what I was getting to earlier about the split issue is that you could have a custom channel set with a single pin which is, by the ST documentation bound to Channel 2 of the timer. While that channel would be referenced as Channel::C3 it would be dereferenced as split().0 and not split().3. What do you guys think? I think that I am just looking for some consistency.

edit:
I should state that I think that this code seems self sufficient and this issue that I am raising could be corrected later in a separate request.

[justacec]

@burrbull For some reason, I noticed that I cannot specify that I want you to re-review the code. Not sure why. That being said, can you please take a look. Thanks.

[TheZoq2]

Looks good, apart from the panic which I'd like to get rid of, but perhaps we could do that in another PR

[TheZoq2]

Still not a fan of this panic, and I'm not sure if check_used is even nessecary anymore

[justacec]

@TheZoq2 It took me a bit to understand as this was a @burbull construction, but the function seems to make sense to me. Without the function there is no way for the per-channel functions to know if that channel is, in-fact, being used. As an example, lets say that I configure the Timer to use only Channel 3 passing a single pin. But then I call something like:

mypwn.enable(Channel::C1);

Without the check_used method, the function would go ahead and set the bit. Although, I am not sure that are any real negative consequences of this.

This was the real reason behind me using a macro in my original incarnation of this functionality. The macro expanded to only include channels that were active and would default to doing nothing through a default match. In this incantation, this has been shifted to be run-time vice compile-time.

Now, concerning the panic. Although I had originally default to no action, I actually see no issue with this panic as it will aggressively inform the user that he/she has something amis. I kind of feel that this is a better approach than doing nothing. In the do nothing variant, I could mis-type something and work for hours trying to figure out why my channel duty is not acting correctly; only to find out that I had put the wrong channel in there. If there was a panic, the compiler would yell at me (I think). If I am mis-understanding that it would occur at the compiler phase and it would be runtime, I would have a potential backtrace which would point to the exact place that needed to be checked.

@burrbull I am cc'ing you here to see if you have any specific comments on this above what I have put here.

[TheZoq2]

Now, concerning the panic. Although I had originally default to no action, I actually see no issue with this panic as it will aggressively inform the user that he/she has something amis.

Agreed, panic is better than doing nothing

If there was a panic, the compiler would yell at me (I think). If I am mis-understanding that it would occur at the compiler phase and it would be runtime,

Panics happen at runtime, if possible I would like to move this to compile time but that is a difficult task.

If I am mis-understanding that it would occur at the compiler phase and it would be runtime, I would have a potential backtrace which would point to the exact place that needed to be checked.

Yes, but backtraces can be a bit of a pain to get a hold of in embedded contexts

But again, we can probably take care of this in the future in another PR, so unless burbull has additional comments, I think this is good to go

[burrbull]

I don't know how to do this check compile time because channel can be not constant.

In fact we need possibility to write const fn in trait implementation and variant of assert which fails in compile when it is possible and fail in runtime in other cases.

[burrbull]

Ok, so, what I was getting to earlier about the split issue is that you could have a custom channel set with a single pin which is, by the ST documentation bound to Channel 2 of the timer. While that channel would be referenced as Channel::C3 it would be dereferenced as split().0 and not split().3. What do you guys think? I think that I am just looking for some consistency.

Alternative for this is only one: to use 4 pins in predefined order and 4 channels everywhere and to use NoPin (or NoChannel) for absent pins. This even easier to implement.

[justacec]

@burrbull In that case it would always return a size 4 tuple then right.

[justacec]

@burrbull So, I have now been looking into trying to implement this. Right now I am trying to find out how to adjust the existing pins_impl macro such that the Channels type is always a size 5 tuple with the channels in the right place. Right now it looks like that would need to be adjusted to be a procedural macro to have the flexibility to format the Channels type. While I look into learning proc_macro, did you have any other thoughts on how this could be implemented that might steer me in the right direction.

BTW: I am returning a size 5 tuple so that channel 1 will be dereferenced as mychannels.1 and not mychannels.0.

Thanks for any direction.

[TeXitoi]

Please no proc macro and no 5 element tuple. If you want logical fields, better to create a struct with chan1, chan2... fields.

[burrbull]

1. I can suggest to add NoChannel (or NoPin) fictive struct like in SPI/I2C.
   burrbull@1f4b04a

2. Pwm and PwmChannel can own by tuple of 4 real pin structs instead of phantom PINS.
   That allows us to release timer and pins.

3. And yes, 5 element tuple is not good idea.

[justacec]

@burrbull @TheZoq2

Guys,

Sorry I have been absent the past several weeks. My work has really stepped up and I have not been able to dedicate as much time to trying to work on my personal projects, which includes learning Rust. I did attempt to get something working which would return a full object vice the Tuple to get around the issue that I had raised earlier. I think that I was very close but, not quite there. I think that you guys would probably be able to implement a fix for that pretty quickly.

I would like to propose that we go ahead and merge this pull request in it's current state as it maintains the current API and provides a fuller implementation of the embedded_hal API. Once this is in, somebody, possibly even me again (with some coaching :) ) could tackle the tuple return issue.

Thoughts?

[therealprof]

I think despite some minor flaws it's a really solid improvement and it has simmered for long enough so let's land this.

Would you mind:

• adding a CHANGELOG entry
• opening up issues for the comments you couldn't address
• squashing the commits a bit
  ?

[therealprof]

Thanks. Will merge this now.