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

Fix: ESP32 espShow() doesn't de-init RMT #424

Merged
merged 6 commits into from
Mar 6, 2025
Merged

Conversation

brentru
Copy link
Member

@brentru brentru commented Mar 5, 2025

@ladyada The issue fixed by this PR is allowing a new NeoPixel object on ESP32 to be destroyed and re-created. The core issue is within esp.c's espShow() implementation for IDF 5+ which looks like it was modified/written by teknynja.

What should work:

Why this does not work:

Addresses:
adafruit/Adafruit_Wippersnapper_Arduino#691
adafruit/Adafruit_Wippersnapper_Arduino#684

@tyeth
Copy link
Contributor

tyeth commented Mar 5, 2025

Does this change mean the updateLength(0) call then show() might be ineffective on other platforms? There doesn't seem to be the same advice about cleanup for other platforms.
Also just now reading the docs it says update length is deprecated and to use the new syntax and length argument.
I don't think many people destroy and then re-setup NeoPixel objects, nor set length to zero and then alter to many pixels, so maybe just untested

Copy link
Contributor

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .update_length(0); .show() to free resources seems too subtle to me. Either the resources (RMT, in this case) should just be freed in the destructor, and that's the only way to free resources, or there might be a .end() routine, which doesn't do anything for most platforms, except for Espressif.

@brentru
Copy link
Member Author

brentru commented Mar 6, 2025

@dhalbert

The .update_length(0); .show() to free resources seems too subtle to me.

Since it wasn't correctly implemented, I was implementing the broken length/show code.

Either the resources (RMT, in this case) should just be freed in the destructor,

I agree; we should go in this direction instead of freeing the RMT via 2 functions. calls may be good for two reasons:

  1. update_length() is marked as a deprecated function to begin with
  2. this issue deals with new and delete and the best spot for handling that would be in the ctor/dtor, respectively.

I'll go ahead and implement this, ping both you and @tyeth when

@brentru brentru requested a review from dhalbert March 6, 2025 21:23
@brentru
Copy link
Member Author

brentru commented Mar 6, 2025

@tyeth Can I re-request a review for this?

Comment on lines +125 to +131
#ifdef ARDUINO_ARCH_ESP32
// Release RMT resources (RMT channels and led_data)
// by indirectly calling into espShow()
memset(pixels, 0, numBytes);
numLEDs = numBytes = 0;
show();
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems OK to me. It still seems a little round-about, but I assume there are use cases for adjusting the number of LEDs on the fly and we support that.

@tyeth tyeth merged commit fd74287 into adafruit:master Mar 6, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants