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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement full embedded_hal::Pwm #176
Implement full embedded_hal::Pwm #176
Changes from all commits
ed4f6f0
5050dda
f37b17c
8d0012c
5a2f9f6
bb0113d
4dfe267
a275dc1
69235c7
8d26086
4a0a15f
7175ebe
0a555c9
89ef755
a6bbac1
06f5a47
6a6b08a
0cbc67f
a77eea3
b22ff3b
a6f21b4
ca3b1a9
87179b1
cf5a152
62ec7bd
b2b3a07
aa3c331
4c4c2d1
1098ab2
a1c54ff
cedb98f
bca666d
16431d3
013478a
7904998
abb1a17
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not a fan of this panic, and I'm not sure if check_used is even nessecary anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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:
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, panic is better than doing nothing
Panics happen at runtime, if possible I would like to move this to compile time but that is a difficult task.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ofassert
which fails in compile when it is possible and fail in runtime in other cases.