Clean up CTIMER; implement embedded_hal::Pwm
#291
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I believe that this structure is easier to follow.
We're using names matching the documentation for most other things that directly correspond to hardware resources, so this is consistent with that.
I don't think the name `ctimer` is long enough to warrant being abbreviated. `inner` is even a bit shorter than that, and I think is a good name for fields with wrapped PAC peripherals.
This adds some complication right now, but I believe it will make things easier going forward.
This is an intermediate step. The plan is to replace `CTimerPwmPin` too.
I believe the new name is a bit clearer.
It is no longer unused.
This saves some runtime cost.
I don't believe this makes much sense as-is. I plan to add `Channels` as a field to `CTIMER`, thus making it much more similar to other peripherals in that regard. I think `Channels` will make more sense then.
Again, this doesn't make much sense by itself. It's a step in preparation of making `CTIMER` something that can be enabled an disabled, like other peripherals.
This is another step in prepration of adding `Channels` to `CTIMER`.
This is another one of those changes that make no sense in isolation: - I want to implement `Pwm` on `CTIMER`. If I do this without giving `CTIMER` control over attaching, then trying to do something with a channel that hasn't been attached will be a runtime error. The way I've done it here, we can keep the availability of methods tied to the channel type state. - This API controls the order in which channels can be attached to output functions. Since these are movable function, this shouldn't limit the user in any real way. - I have a concept in mind that deals with detached channels without runtime errors, but that is going to require the channels to be attached sequentially, without any holes.
It is no longer unused.
This follows the convention established in other modules.
Now that `Channels` has been moved out, `channel` fits much better.
This is ugly. Much uglier than I expected it to be. I don't know if this is any better than having to check runtime errors (from a user's perspective it probably is, especially if efficiency is a concern), but since the old embedded-hal traits don't support error checking, I don't know if we have much of a choice right now.
Member
Author
|
Did a bit of force-pushing to fix some CI issues. Nightly still fails for unrelated reasons. Should be good to go now. |
david-sawatzke
approved these changes
Oct 27, 2020
Member
david-sawatzke
left a comment
david-sawatzke
left a comment
There was a problem hiding this comment.
Seems good, but the hal impls really are something else
Member
Author
|
Thanks for the review! I've had this idea that with fully-formed const generics, it might be possible to replace the enums with a number and do some type-level validation depending on how many channels have been attached. At this point this is just science fiction, but who knows! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Lots of commits in this one, but I believe each one should be relatively easy to review.
As noted in the commit, the implementation of
embedded_hal::Pwmis really ugly. I'm not sure what else to do though. Once const generics are fully supported, it might might sense to replace the enum with a number and make it all nice, but for now, I don't really know if that mess is worth the improved reliability and runtime overhead.Happy to hear suggestions.