Skip to content

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

Closed
wants to merge 2 commits into from

Conversation

mcauser
Copy link
Contributor

@mcauser mcauser commented Dec 24, 2019

@mcauser mcauser changed the title ESP32 Add Sigma-Delta peripheral esp32: Add Sigma-Delta peripheral Dec 24, 2019
@tve
Copy link
Contributor

tve commented Jan 9, 2020

Would be nice to get this merged! (I guess you have some conflicts to resolve, though)

@mcauser
Copy link
Contributor Author

mcauser commented Jan 12, 2020

There's also one for the ESP8266: #4167

@mcauser
Copy link
Contributor Author

mcauser commented Jan 12, 2020

Fixed docs conflict with a rebase

@tve
Copy link
Contributor

tve commented Jan 13, 2020

Oh my, the ESP8266 PR has been languishing for months :-(

mp_raise_ValueError("duty must be between -128 and 127");
}

if (prescale < 0 || prescale > 255) {
Copy link
Contributor

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?

Copy link
Contributor Author

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;

Copy link
Contributor

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...

@mattytrentini
Copy link
Contributor

Looks good @mcauser!

Copy link
Contributor

@tve tve left a 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.

@mattytrentini
Copy link
Contributor

Can this be merged @dpgeorge?

@mcauser mcauser force-pushed the sigma-delta branch 3 times, most recently from f6521d8 to b405ea0 Compare May 14, 2021 06:59
@mcauser
Copy link
Contributor Author

mcauser commented May 14, 2021

Rebased (a few times)

tannewt pushed a commit to tannewt/circuitpython that referenced this pull request Oct 13, 2021
Use `mp_arg_validate_type` for keyword args
@mcauser
Copy link
Contributor Author

mcauser commented Jun 29, 2023

Rebased

mcauser added 2 commits June 29, 2023 11:53
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>
@mcauser
Copy link
Contributor Author

mcauser commented Jun 29, 2023

Warning: The legacy sigma-delta driver is deprecated, please use driver/sdm.h

@projectgus
Copy link
Contributor

This is an automated heads-up that we've just merged a Pull Request
that removes the STATIC macro from MicroPython's C API.

See #13763

A search suggests this PR might apply the STATIC macro to some C code. If it
does, then next time you rebase the PR (or merge from master) then you should
please replace all the STATIC keywords with static.

Although this is an automated message, feel free to @-reply to me directly if
you have any questions about this.

@mcauser mcauser closed this Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants