Open mlamoore opened 4 years ago
For our SPI implemenation we already have a Config struct that can be passed to the initialization function so I'd imagien having TIM1.pwm but with an additional config parameter might be nice and consistent to the rest of the HAL?
I also see a potential use of type states with your into_complementary_pwm(...)
function (in case you don't know that pattern yet, it's basically what is used in the GPIO implementation, we put 0 sized generic parameters into our structs that are optimized away at compile time but allow us to define that certain functions should only be accesible if the type has a certain set of generics, in this case that'd be ComplementaryOutput and StandardOutput or something like that? You could then write complementary specific methods in a
impl MyChannel<ComplementaryOutput> {
// Complementary functions
fn into_standard(self) -> Self<StandardOutput> // performs the state transition
}
// Same for StandardOutput
That would allow to reduce faulty use of your new, rather complex. feature.
Apart from that, judging from your description (not that deep into PWM until you described here) your general approach seems very reasonable to me, good luck!
I'm definitely planning on using the type state pattern for this. I've read the Embedded Rust book and submitted a PR for the PWM library that uses type states, but I'm definitely still learning here. I see having type states for PWM channels that are standard only, standard but complementary capable, and complementary mode active.
The main problem I see with a config struct is that the return type of the PWM initialization depends on the configuration and which timer this is: normally we return a channel or tuple of channels, but if we configure a fault input, I think we also need to return a fault monitor that can check and clear faults. I would see the return type then being (FaultMonitor, PINS::Channel) instead of PINS::Channel.
Does having a separate fault monitor struct make sense? My reasoning is that the fault is almost like an asynchronous event/process that can disable all outputs of a timer, so it doesn't belong to one channel. Just having globally accessible functions to read and write that register is what I would do in C, but I feel like controlling access is better in the Rust paradigm. I would use type state programming for both the fault pins and fault monitor, similar to the Pins and Pwm for the standard PWM channels.
If the return type is that variable then yes having seperate constructors make sense.
If the FaultMonitor is able to block all outputs of the Timer I feel like it should not co exist with the PWM pins as he can directly influence the pins without the pins knowing. However he can also not really own the pins I think? As you usually want seperate access to them in your drivers etc so that's not really a solution either :/ But having the FaultMonitor have side effects on the pins without the pins being able to know etc definitely sounds bad. A driver that uses PWM for example would assume that such side effects don't exist and it would introduce lots of quirks etc. that are kinda unpredictable, I hope there is a better way to represent this in the Rust type system but nothing comes to mind right now. Maybe you or someone else has an idea?
The peripheral itself can be configured to block all PWM outputs with no software intervention. This is useful because it allows fast response to a short circuit or similar fault; you want to turn off the output in single digit microseconds or less, and the analog current sensor takes that long with the filtering it needs to avoid false trips, so software can't be involved there.
With an external hardware fault circuit which is sometimes used, disabling the PWM happens with logic gates off-chip and software has no choice but to monitor a separate GPIO to learn about faults.
Most control loops I've dealt with, it doesn't matter if it runs a few iterations with no effect after a fault; if a control loop does care, that control loop could require a shared reference to some trait that allows viewing faults without clearing them.
The way I see it, whether fault circuit is present or not should have no impact on a PWM control algorithm. We should have standard PWM channels that implement the embedded-hal PWM trait and have motor control algorithms that work regardless of if you have overcorrect protection or what kind of overcurrent protection you have.
That said, I'm new to designing Rust interfaces. I'm open to other ways to handle it, I just want to make sure this stays compatible with embedded-hal and allows control algorithms to just use the PwmPin trait without worrying about what type of fault detection is being used.
Ah I get it now, in that case yes having the FaultMonitor and the pins in a tuple like that sounds reasonable to me.
So summing things up your design approach one + the type states you planned anyways seems reasonable for me to use, I'd say go for it unless someone else has objections!
As I've been investigating this, I don't think my design approach 1 will scale well enough... I also need to expose trigger outputs TRGO and TRGO2 and not all timers support all these options and the variants seem to be multiplying exponentially. I feel like type state programming and a builder pattern is the better route here.
Now the question is how to handle multiple outputs, many of which aren't always possible. My original approach 2 doesn't work well unless I made freeze() return a huge tuple with lots of options, which seems really ugly.
I'm considering a few ways to get the functionality I need:
let builder = tim.advanced_pwm(pins, freq, prec, clocks).center_aligned(),deadtime(500.ns()); let (builder, fault_monitor) = builder.fault( fault_pin, fault_state, complementary_fault_state); let (builder, trigger_artifact) = builder.trigger_output( trigger_mode ); let (c1, c2, c3) = builder.freeze(); let c1 = c1.into_complementary_output(comp_pin);
Builder would be a generic type with marker structs representing the PWM settings, and actual initialization would take place when freeze() is called.
Alternatively, instead of having individual objects representing the trigger outputs and the fault state, I could try have freeze return a single pwm_state object that implements appropriate traits depending on what state the builder was in:
let (channels, pwm_state) = tim.pwm(pins, freq, prec, clocks).center_aligned(),deadtime(500.ns()).fault( fault_pin, fault_state, complementary_fault_state ).trigger_output(trigger_mode).freeze() let trigger_mode = pwm_state.trigger_output(); pwm_state.clear_fault();
Does using tuples to return an 'extra' value from some builder functions seem ugly? Any other ideas on clean ways to return 'extra' values from a builder or other ways to handle such a wide variety of timer configurations?
I've got this working and proposed it as PR #175
Comments are welcome; is there anything that should be changed about this updated API?
I'm working on a project using the STM32H750 for power electronics applications. I need several features of the chip that are beyond the simple interface of embedded-hal, but I'd like to do this in a way that is still compatible with the embedded-hal traits and is as generally useful as possible.
I'm an experienced embedded C developer but relatively new to Rust, especially trait/generic based libraries and macros. I'm looking for feedback on what features should be included and how they should be exposed to the user.
Here's the features I want to add:
Center-aligned PWM is a different counter mode that can be set up on initialization of timers 1,2,3,4,5,8. It can be easily set up at timer initialization and ignored while running.
PWM fault input pins, complementary outputs, and dead time are all supported only on timers that have the BDTR register: timers 1,8,15,16,17. All channels use the same dead time, so I envision that being configured at timer initialization. I think it would make sense to initialize channels as standard/positive polarity and add a .into_complementary_output(complementary_pin) function that changes the PWM mode from single ended to complementary.
For the break inputs, the hardware is very flexible and configurable. I would like to use the break input functionality to turn off PWM when the break input pin goes low and not turn it back on until a function is called to clear the fault. I would like to do this by returning a separate struct that implements a trait with functions fault_active() -> bool and clear_fault(). The individual PWM channels would be completely unaware of the fault functionality: they would continue to set duty cycle, enable, or disable normally and the output would just stay inactive until a fault was cleared.
I'm thinking about two different APIs for this:
Separate functions, like this: TIM1.pwm(pins, freq, prec, clocks) -> (channels) TIM1.pwm_center(pins, freq, prec, clocks) -> (channels) TIM1.pwm_deadtime(pins, freq, prec, clocks, deadtime) -> (channels) TIM1.pwm_fault_deadtime(pins, freq, prec, clocks, fault_pin, fault_state, deadtime) -> (fault_monitor, channels) TIM1.pwm_center_fault_deadtime(pins, freq, prec, clocks, fault_pin, fault_state, deadtime) -> (fault_monitor, channels)
Then channels could be adjusted like this: channel.set_active_level( ActiveHigh or ActiveLow ) channel.set_fault_level( ActiveHigh or ActiveLow ) let channel = channel.into_complementary_pwm( complementary_pin, complementary_active_state, complementary_fault_state )
Or I could make a builder style API, something like this (with the same methods to adjust an individual channel mode):
TIM1.pwm(pins, freq, prec, clocks).center_aligned(),deadtime(500.ns()).fault( fault_pin, fault_state, complementary_fault_state ).freeze() -> (fault_monitor, channels)
The downside is that breaks existing code using TIM1.pwm(), so I could keep TIM1.pwm() as is and add TIM1.advanced_pwm() for the builder.
Does anybody have an opinion on whether I should implement the first API, the second API, or something else? And are there other PWM related functions people would like to see or other places I should make this more flexible for others?