Skip to content
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

esp32 RMT: Extend Micropython use of hardware carrier freq feature. #6127

Closed
wants to merge 15 commits into from

Conversation

JonRob812
Copy link

continuing from my premature pull request here: #6118

ports/esp32/esp32_rmt.c Outdated Show resolved Hide resolved
ports/esp32/esp32_rmt.c Outdated Show resolved Hide resolved
ports/esp32/esp32_rmt.c Show resolved Hide resolved
@dpgeorge
Copy link
Member

@mattytrentini do you have any comments on this?

@mattytrentini
Copy link
Sponsor Contributor

I've actually exposed similar functionality which I recently revisited in preparation for PR. Sorry @JonRob812, I should have submitted mine a while ago! But I think we can merge our efforts.

Obvious differences: @JonRob812 has added documentation. Mine adds the carrier level (which I thought was required but perhaps not). The bigger change on my branch is that I've also added different ways to specify the RMT pulse stream and I've exposed the idle level.

I'll take a closer look tonight (and update the variable naming - I also used hz - that you suggested @dpgeorge).

ports/esp32/esp32_rmt.c Outdated Show resolved Hide resolved
ports/esp32/esp32_rmt.c Outdated Show resolved Hide resolved
ports/esp32/esp32_rmt.c Outdated Show resolved Hide resolved
ports/esp32/esp32_rmt.c Outdated Show resolved Hide resolved
@dpgeorge
Copy link
Member

Obvious differences: @JonRob812 has added documentation. Mine adds the carrier level (which I thought was required but perhaps not).

@JonRob812 is there a reason you didn't include carrier_level as an option? Maybe it's not needed after all, since all the IDF examples set it to 1. Anyway, it can be easily added if needed (maybe later on?) by a new kwarg.

The bigger change on my branch is that I've also added different ways to specify the RMT pulse stream and I've exposed the idle level.

That sounds like a change that's better suited to a separate PR.

Jon Rob added 2 commits June 12, 2020 06:10
Merge branch 'rmt' of /media/sf_LinuxDev/mp/ into rmt
@JonRob812
Copy link
Author

JonRob812 commented Jun 12, 2020

Obvious differences: @JonRob812 has added documentation. Mine adds the carrier level (which I thought was required but perhaps not).

@JonRob812 is there a reason you didn't include carrier_level as an option? Maybe it's not needed after all, since all the IDF examples set it to 1. Anyway, it can be easily added if needed (maybe later on?) by a new kwarg.

I have no excuse other than I lacked understanding of how they get used. After researching I still lack the understanding. It seems like carrier_level does same thing that start=1 vs start=0 could do? ( inwrite_pulses() ) I also dont understand idle_output_en or idle_level. What does idle mean? Is it sleep?

@mattytrentini
Copy link
Sponsor Contributor

If I understand correctly, when idle_output_en is enabled, the output of the pin will be idle_level before and after the RMT pulse sequence has completed. It's not clear to me what the output of the pin will be when idle_output_en is not enabled but I would take a guess at it 'floating' - ie remaining at whatever level was drive by the last pulse.

start is not an IDF concept, it's used by the MicroPython RMT driver as an option to define the pulse stream. It could be used instead of carrier_level but I would prefer carrier_level to be exposed because start won't always be used to define the pulse stream (see my branch for alternative ways to configure it). Also, it's in the ESP RMT documentation so users would expect it. That said, it's not a commonly used feature so it's possibly not critical (but why not include it?).

@dpgeorge
Copy link
Member

Thanks for applying the feedback. As it stands I'd be happy to merge this PR.

Re carrier_level: I don't mind either way if this is added or not. @mattytrentini maybe you want to follow up later on adding this and other enhancements?

@mattytrentini
Copy link
Sponsor Contributor

I'm ok if we add carrier_level later; it can wait.

I'm more interested in getting the different pulse stream definitions merged in; that seems to be more useful to folks...

@dpgeorge
Copy link
Member

Squashed and merged in 1678f41, with some typo fixes to the docs.

Thank you for the contribution!

@dpgeorge dpgeorge closed this Jun 16, 2020
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.

None yet

4 participants