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-S2/S3 support #69

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

romkey
Copy link

@romkey romkey commented Sep 23, 2022

This PR adds conditional compilation so that Chroma Pixie doesn't enable FastLED I2S support on ESP32-S2 or ESP32-S3 targets, because FastLED's I2S support doesn't build properly for them.

ESP32-S2 and ESP32-S3 targets also have different available pins from vanilla ESP32s, so this code conditionally compiles to only use the pins available on those targets and to add a few more that don't exist on vanilla ESP32s.

It also changes pin 27 to pin 26 on Quad Mode, since pin 27 isn't available on the S2 or S3. I just randomly picked pin 26 as it's allowed by FastLED.

set up acceptable pin definitions for ESP32-S2 and ESP32-S3 (compatible with FastLED 3.5.0)

change quad mode pin for ESP32-S2 and ESP32-S3 from 27 to 26 as 27 is not usable
@connornishijima
Copy link
Owner

/home/runner/Arduino/libraries/Pixie_Chroma/src/pixie_chroma_internal.cpp: In member function 'int16_t PixieChroma::calc_justification(t_justification, uint8_t)':
/home/runner/Arduino/libraries/Pixie_Chroma/src/pixie_chroma_internal.cpp:3027:26: error: 'x_offset_chars' may be used uninitialized in this function [-Werror=maybe-uninitialized]
return x_offset_chars * display_width;
                      ^~~~~~~~~~~~~
cc1plus: some warnings being treated as errors

Looks like the compilation failed because of line 2971 in pixie_chroma_internal.cpp:

int16_t x_offset_chars;

I'm not sure how CI tests succeeded until now, since you didn't even modify anything related to this. Let me try resolving that and testing again!

@connornishijima
Copy link
Owner

Hmmm. Now CI is complaining about something in the FastLED dependency that isn't even hosted here.

/home/runner/Arduino/libraries/FastLED/src/platforms/esp/32/clockless_rmt_esp32.cpp: In static member function 'static void ESP32RMTController::init(gpio_num_t)':
/home/runner/Arduino/libraries/FastLED/src/platforms/esp/32/clockless_rmt_esp32.cpp:111:15: error: variable 'espErr' set but not used [-Werror=unused-but-set-variable]
 esp_err_t espErr = ESP_OK;
           ^~~~~~
/home/runner/Arduino/libraries/FastLED/src/platforms/esp/32/clockless_rmt_esp32.cpp: In member function 'void ESP32RMTController::startOnChannel(int)':
/home/runner/Arduino/libraries/FastLED/src/platforms/esp/32/clockless_rmt_esp32.cpp:239:15: error: variable 'espErr' set but not used [-Werror=unused-but-set-variable]
 esp_err_t espErr = ESP_OK;
           ^~~~~~

cc1plus: some warnings being treated as errors

I have some S2s on hand to try, let me see if they work with your branch in person. If so, I'll just merge now and fix the CI afterwards. Thank you for your help @romkey!

@romkey
Copy link
Author

romkey commented Sep 23, 2022

I saw the exact same thing. I was actually going to submit a PR for the CI problems and then I stalled here.

I think that the compiler is just being waaaay too strict about this stuff; I took a couple of shots at telling it to not to treat things like unused variables as errors and didn't get that working.

I know this was working for you; it feels like some dependency must have changed, making it crazy strict.

Anyway thanks for taking a look at this so quickly!

@connornishijima
Copy link
Owner

Looks like CI is running just fine now! Sorry, I didn't get a chance yesterday to test on the S2, but the solve you proposed seems to be functioning. I'll have to still do some runtime tests to check, but thanks! (The Docs build failing seems to be whenever you submit a change, which might be a weird permissions issue. No worries there.)

@romkey
Copy link
Author

romkey commented Sep 24, 2022

It's okay, it's not urgent!

I just updated the PR with two changes. I missed a spot checking for an S3 build, so I fixed that.

I think I figured out the CI thing, too - adding:

extra-arduino-cli-args: "--warnings default"

to each platform test makes the errors less strict. It was doing --warnings all which seems to turn it up to 11. This is embedded in the arduino-test-compile code. I tried a couple of other more specific things but the --warnings all seemed to override them. I intentionally checked in an error on another repo and it still caught that so I think this is an okay fix.

BTW, I haven't tested quad mode. The changes compile for it but I haven't confirmed it actually works on an S2 or S3. I need to solder up a few more Pixies to do that. I think it should be okay but the inner workings of FastLED in that area are a bit arcane.

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.

2 participants