-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
esp32: Add Sigma-Delta peripheral #5452
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
Conversation
Would be nice to get this merged! (I guess you have some conflicts to resolve, though) |
There's also one for the ESP8266: #4167 |
Fixed docs conflict with a rebase |
Oh my, the ESP8266 PR has been languishing for months :-( |
ports/esp32/esp32_sigmadelta.c
Outdated
mp_raise_ValueError("duty must be between -128 and 127"); | ||
} | ||
|
||
if (prescale < 0 || prescale > 255) { |
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.
prescale
is an mp_uint_t
(typedef to unsigned long
); any point testing for <0?
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.
duty
should be a int8_t
(-128 ~ 127). Am I casting it to a mp_int_t
correctly (line 65)?
prescale
should be uint8_t
(0 - 255). If it's being cast as unsigned
, then no, as it's not possible to be < 0, so that could be removed.
I thought mp_uint_t
was an int
, not a long
?
typedef unsigned int mp_uint_t;
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.
Ah, good point, mp_uint_t
is port-dependent. Haven't quite tracked down what that pops out to for ESP32 but it seems to usually be an unsigned type...
Looks good @mcauser! |
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.
LGTM!
Only thing I'm wondering is that most drivers seem to use a model where there is a constructor and an init function, both of which take essentially the same args. Here there is only a constructor.
Can this be merged @dpgeorge? |
f6521d8
to
b405ea0
Compare
Rebased (a few times) |
Use `mp_arg_validate_type` for keyword args
Rebased |
This is an ESP32-specific peripheral so lives in the esp32 module Signed-off-by: Mike Causer <mcauser@gmail.com>
Signed-off-by: Mike Causer <mcauser@gmail.com>
Warning: The legacy sigma-delta driver is deprecated, please use |
This is an automated heads-up that we've just merged a Pull Request See #13763 A search suggests this PR might apply the STATIC macro to some C code. If it Although this is an automated message, feel free to @-reply to me directly if |
https://docs.espressif.com/projects/esp-idf/en/stable/api-reference/peripherals/sigmadelta.html