connornishijima / Pixie_Chroma

Arduino library and documentation for Pixie Chroma displays!
https://lixielabs.com/chroma/
MIT License
53 stars 9 forks source link

ESP32-S2/S3 support #69

Open romkey opened 1 year ago

romkey commented 1 year ago

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.

connornishijima commented 1 year ago
/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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.