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
MCPWM 5.0 #132
base: master
Are you sure you want to change the base?
MCPWM 5.0 #132
Changes from 55 commits
e47f8af
5a86430
bef1ea4
208bde0
ad91ea1
4e3b09c
8a1b0fd
17a3e63
a428ed6
8f2f559
d64529a
729cb85
15610a2
cc8589d
8ca34ba
4ed2696
490a84a
c874e78
9ebbeee
d05ae44
f5a13ca
6bc129a
1d779db
8384b06
5f01835
5acf5fc
a10f4f3
12e34db
0caf311
451dcd9
952a36f
9ec6458
ca5685e
dc05884
d932af4
8d80760
1907eef
bb8d957
e7e755e
d20ae62
7b2a929
6e8d31e
d3e44c1
c15c69f
f053be1
326299f
44cd1d8
5c4738b
7ab9357
aa19703
55745e6
1c3dae4
8bca808
7063f05
1bd2dfd
d786f05
4acbca7
6913374
cc76cb5
bdcb5e4
316f685
66d0dde
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.
The implementation uses lot's of traits which results in a heavy utilization of generics, making the code difficult to comprehend and probably difficult to use.
I don't mind generics and traits, if they cannot be avoided, but I'm questioning the utility of almost all traits in this module. Most of them can be replaced with alternatives: enums or simple options.
I'll comment on each trait below.
For
ComparatorChannel
specifically: why not justThere 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 am struggling to understand what
OptionalCmp
is giving me that a simpleOption<&'a mut Comparator>
won't?Struct
Comparator
can then just have afn counting_direction(&self) -> CountingDirection
method.Would you elaborate?
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.
Again, I am not quite happy with the huge amount of traits either :/
The motivation behind
OptionalCmp
is to only provide aoperator.set_compare_value_x
iffCMPX
is aComparator
.esp-idf-hal/src/mcpwm/operator.rs
Line 143 in 4acbca7
I personally do not quite like a method doing that check at runtime. I will hower change it if you would.
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 do not have time the time to check right now but I believe, if I am not mistaken, there was also something similar when building the generator config. The generator config should not depend on events from comparators that are not provided.
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.
On second thought, perhaps both comparators should be obligatory since they can not be used for anything else outside their operator anyways...
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.
Just remembered espressif/esp-idf#9904 . Do you think
tep
should also be set to1
to avoid that? I have not tested this PR for that problem yetThere 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.
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.
Ditto here. Why are we hiding simple
Option
s behind traits with just two implementations: none and an actual one?