Add ctimer based pwm implementation

[david-sawatzke]

No description provided.

[david-sawatzke]

@hannobraun RegProxy really feels like it should have an unsafe new method, since I think you can otherwise create race conditions. Is there any reason it's not currently?

[hannobraun]

Thank you, @david-sawatzke! I intend to take a look later today.

RegProxy really feels like it should have an unsafe new method, since I think you can otherwise create race conditions. Is there any reason it's not currently?

I don't remember my reasoning back then in detail, but I think the key is this bit of documentation:

lpc8xx-hal/src/reg_proxy.rs

Lines 48 to 51 in ba40d07

	/// Please note that all of this isn't really different from the raw API
	/// generated by svd2rust, as multiple shared references to the same
	/// register can exist there, and a shared reference is all that's required
	/// to have full control over a register.

Basically, RegProxy doesn't enable anything you can't already do with raw registers.

The whole thing is a huge hack anyway. I still hope that rust-embedded/svd2rust#213 will get implemented some day.

[hannobraun]

Addendum to my comment earlier: I forgot that RegProxy::new doesn't have an argument. In its current form, it should definitely be unsafe. I think what I said earlier would make sense, if RegProxy took a reference to the register, just to prove the caller has access.

But yeah, all of it is a bit of a mess. I think it's fine, because it's just an internal helper, but I wouldn't mind making its constructor unsafe.

[hannobraun]

Thank you, looking good!

I'm not very familiar with CTIMER and didn't review the register access code in detail, but I tested the example and it works great.

Two notes:

• I think this needs a rebase (as indicated by the SWM workaround I pointed out, which probably shouldn't compile on current master).
• I'm not sure that the naming is really on point. start_pwm seems to be appropriately named, but it returns UnconfiguredPwmPin, which seems already weird (It's not configured? I though I already started it?). The way I understand it, everything is configured and running, it just doesn't output to any pin.

Suggestion: Rename UnconfiguredPwmPin to DetachedPwmPin and configure to attach. I believe that would be clearer. (We could also use UnassignedPwmPin and assign, but that conflicts with the SWM nomenclature and would be confusing for that reason.)

[david-sawatzke]

Suggestion: Rename UnconfiguredPwmPin to DetachedPwmPin and configure to attach. I believe that would be clearer

Thanks for the suggestion, it's more obvious what happens that way

[hannobraun]

Thanks, that's perfect now!

I found some typos, but I believe I'll be able to apply my own suggestions, once I've submitted this review.

[hannobraun]

Suggested change

	/// An detached [`CTimerPwmPin`]
	/// A detached [`CTimerPwmPin`]

[hannobraun]

Suggested change

	/// Use `attach` to assing an output to it
	/// Use `attach` to assign an output to it.

[hannobraun]

Suggested change

	/// Assings a pin to an `DetachedPwmPin`,
	/// Assigns a pin to a `DetachedPwmPin`,

[hannobraun]

Seems I don't have permissions to commit to your branch. You will have to apply the suggestions. Feel free to merge yourself, once you did that.