8000 ports/esp32/esp32_rmt.c: Fix looping behavior for RMT by IcedRooibos · Pull Request #11254 · micropython/micropython · GitHub
[go: up one dir, main page]
Skip to content

ports/esp32/esp32_rmt.c: Fix looping behavior for RMT #11254

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

ports/esp32/esp32_rmt.c: Fix looping behavior for RMT #11254

wants to merge 1 commit into from

Conversation

IcedRooibos
Copy link
Contributor
@IcedRooibos IcedRooibos commented Apr 13, 2023 •
edited
Loading

Issue link: #11213

Problem:
This code below doesn't loop correctly on the ESP32-S3 (tested). It works fine on the ESP32 (also tested).

import esp32
from machine import Pin

r = esp32.RMT(0, pin=Pin(2), clock_div=255)
r.loop(1)
r.write_pulses((32767, 32767))

To fix this I changed the order of check_esp_err(rmt_write_items(self->channel_id, self->items, num_items, false));

I am not sure about the history behind this module so it may be that this PR has something wrong with it - very open to learning about it. Submitting it for the moment since it fixes the issue for me - tested on ESP32 and ESP32-S3.

Hardware ESP32: ESP32 DEVKIT V1
Hardware ESP32S3: LuatOS ESP32-S3 running MicroPython v1.19.1-995-g0a3600a9a on 2023-03-31; ESP32S3 module (spiram octal) with ESP32S3

Firmware binaries after the change was made
Firmware bin for ESP32: esp32-rmt-loop-fix.zip
Firmware bin for ESP32-S3 OCTAL SPIRAM: firmware-rmt-esp32s3-loop.zip

I almost feel like this section could be simplified but didn't do it since I didn't want to accidentally break something. Let me know if we prefer the simplified version.

Current version where the only difference compared to current code is I changed the order of rmt_write_items

    if (self->loop_en) {
        bool loop_en;
        check_esp_err(rmt_get_tx_loop_mode(self->channel_id, &loop_en));
        if (loop_en) {
            check_esp_err(rmt_set_tx_intr_en(self->channel_id, true));
            check_esp_err(rmt_set_tx_loop_mode(self->channel_id, false));
        }
        check_esp_err(rmt_wait_tx_done(self->channel_id, portMAX_DELAY));
    }

    if (self->loop_en) {
        check_esp_err(rmt_set_tx_intr_en(self->channel_id, false));
        check_esp_err(rmt_set_tx_loop_mode(self->channel_id, true));
    }

    check_esp_err(rmt_write_items(self->channel_id, self->items, num_items, false));

Simplified version

    if (self->loop_en) {
        check_esp_err(rmt_wait_tx_done(self->channel_id, portMAX_DELAY));
    }

    check_esp_err(rmt_write_items(self->channel_id, self->items, num_items, false));

Happy to run more tests if needed, let me know.

@IcedRooibos IcedRooibos changed the title Fix looping behavior for RMT /ports/esp32/esp32_rmt.c: Fix looping behavior for RMT Apr 13, 2023
@IcedRooibos IcedRooibos changed the title /ports/esp32/esp32_rmt.c: Fix looping behavior for RMT ports/esp32/esp32_rmt.c: Fix looping behavior for RMT Apr 13, 2023
@dpgeorge
Copy link
Member

@mattytrentini what do you think about this? It looks OK to me, all the IDF (v4.4.3) examples call rmt_set_tx_loop_mode before rmt_write_items.

@mattytrentini
Copy link
Contributor

@mattytrentini what do you think about this? It looks OK to me, all the IDF (v4.4.3) examples call rmt_set_tx_loop_mode before rmt_write_items.

Yes, I'm happy for the change - though it's very curious why the behaviour is different between the ESP32 and the ESP32-S3! I wonder how many subtle differences there will be when running ULP code on the S3?

@IcedRooibos
Copy link
Contributor Author

Any thoughts on whether this is ready to be accepted? Let me know if you need any more testing from me of course, happy to help.

@IcedRooibos IcedRooibos mentioned this pull request Apr 23, 2023
Closed
@dpgeorge
Copy link
Member

Rebased and merged in 7ea06a3

@dpgeorge dpgeorge closed this Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
port-esp32
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@IcedRooibos @dpgeorge @mattytrentini
0