Force swm to be enabled on lpc845, fix #183

[david-sawatzke]

Also makes the new functions public, to stay in line with gpio and
bypass unused function warnings.

Was there a good reason to implement these on Handle instead of the whole peripheral?

[hannobraun]

Thank you, @david-sawatzke! Overall, this looks good (as always). I've left some comments.

Also makes the new functions public, to stay in line with gpio and
bypass unused function warnings.

I don't like that. If I recall correctly, the GPIO API got public constructors, because that was required with the multi-crate setup. I think I begrudgingly accepted that back then, because there was no other way.

This is no longer required, and I think having non-unsafe constructors is wrong. The user might have done anything with the PAC struct, invalidating our type state and all assumptions we make based on it (there's a lot more type state in there than just enabled/disabled). I think it would be best to only have unsafe constructors to prevent this issue, or no public constructors at all.

(This issue was the main driver behind the Peripherals design. I realize that no other HAL in the ecosystem cares about that problem, but that doesn't mean I'm wrong.)

The unused warnings can be fixed with something like #[cfg_attr(feature = "lpc82x", allow(unused))].

Was there a good reason to implement these on Handle instead of the whole peripheral?

Yes. Answered in review comment. Might be best to include that in the documentation.

[david-sawatzke]

The user might have done anything with the PAC struct, invalidating our type state and all assumptions we make based on it (there's a lot more type state in there than just enabled/disabled).

I'm not sure about it, and it's better to get close to this goal than to never try it, but don't we have issues with IOCON?

With the IOCON, one can set pins to Hi-Z and even worse, on the lpc845 that can happen via the FAIM, so we don't have the option to intercept it.

[hannobraun]

I'm not sure about it, and it's better to get close to this goal than to never try it, but don't we have issues with IOCON?

With the IOCON, one can set pins to Hi-Z and even worse, on the lpc845 that can happen via the FAIM, so we don't have the option to intercept it.

Well, LPC8xx HAL isn't complete yet, and it's not like I've studied all of the user manuals in detail. The API guarantees exist on a best-effort basis. There's not an IOCON API so far, so whatever damage you can do there is out of scope for now. It might be good to add a note to that effect to the GPIO documentation.

Regarding the FAIM, I haven't looked at it in detail, but I think we need to update the affected APIs as follows:

• Add an "unknown" type state.
• Provide a safe method that moves from that unknown state to the user's desired state, doing a runtime check to make sure the hardware settings match that desired state. That would basically return an Option<Pin<ExpectedState>>, or maybe a Result.
• Maybe provide a variant that sets the expected state (would return Pin<ExpectedState> directly).
• Provide an unsafe method that does the same, minus the runtime checking (if the user is sure and doesn't want the overhead). Would return Pin<ExpectedState>.

Just some rough thoughts for now. Details to be determined.

[hannobraun]

Thank you, looking good!

I think making the constructors private, unsafe, or something else, can be left for later.