Closed rdecarreau closed 3 years ago
Bleep bloop!
LabVIEW Diff Robot here with some diffs served up hot for your pull request.
Notice something funny? Help fix me on my GitHub repo.
[x] I'm a bit confused by naming. The existing PWM output mode is called PWM Mode
, and this one is called Counter PWM Input Mode
. Should the existing class be named Counter PWM Output Mode
, or am I missing something?
[x] The nested acronyms in CPI
bug me a bit; I would not immediately assume that CPI
was related to PWM, and it looks like it would conflict with the acronym for period measurements on specialty digital modules. I'm wondering if there's a clearer short-hand we can use.
[x] In Convert CPI Module to Slot Configuration.vi
, I'd prefer if we used the ring types rather than numeric literals. Unfortunately they aren't typedef'd, so you need to copy it, but IMO it makes the code much clearer.
@rtzoeller I named it this way because this is "Counter" mode with "PWM" measurement mode. I don't think we had a convention for this before.
@rtzoeller Updated the naming and acronyms, updated to use typedefs (controls copied from the SD Modules class).
Bleep bloop!
LabVIEW Diff Robot here with some diffs served up hot for your pull request.
Notice something funny? Help fix me on my GitHub repo.
@rdecarreau it looks like CPI
is still used in the icon for Read Counter Input PWM Module.vi
.
Removed CPI, good catch Sorted Get Slot cases (@oscarfonloz take note of this) Added controls.
Bleep bloop!
LabVIEW Diff Robot here with some diffs served up hot for your pull request.
Notice something funny? Help fix me on my GitHub repo.
What does this Pull Request accomplish?
Adds support for Counter PWM input mode.
Why should this Pull Request be merged?
To create modules which support this mode of input
What testing has been done?
Ran the vi tests locally, although those don't cover this new mode yet. I modified the 9422 locally to use the new CPI mode and that passed tests.