LowPowerLab / RFM69

RFM69 library for RFM69W, RFM69HW, RFM69CW, RFM69HCW (semtech SX1231, SX1231H)
GNU General Public License v3.0
780 stars 380 forks source link

Do not redefine RF69_SPI_CS #160

Closed mcspr closed 3 years ago

mcspr commented 3 years ago

Noticed here: https://github.com/xoseperez/espurna/runs/1757526551?check_suite_focus=true#step:9:290

Avoids `RFM69.h:141:0: warning: "RF69_SPI_CS" redefined [enabled by default]' Remove esp8266 & esp32 flags, always infer the value through SS Move definition down to stand out a bit more

My understanding is that we always have SS through the Arduino.h 'variant' constants (but I guess originally this was not the case?)

LowPowerLab commented 3 years ago

"My understanding is that we always have SS through the Arduino.h 'variant' constants" @mcspr Thanks, That assumes the SS is defined in all ESP based boards packages. If that is not true, others will start opening issues to fix this and go backwards. Any suggestions on that? Can you verify all main/popular ESP8266/32 packages have the SS defined?

mcspr commented 3 years ago

See: https://gist.github.com/mcspr/f1392a748f494278985afd322bf3f4c9 Both using latest framework-arduino packages via pio platform install. Every $variant/pins_arduino.h include is able to build printf("%hhu\n", SS);

The only weird one espressif32's fm-devkit where SS is NOT_A_PIN aka -1

LowPowerLab commented 3 years ago

Well wait, only a fraction of Arduino users use platformio, is that the same on Arduino? I think there's a risk of introducing a breakage here. I'd rather not do that and have people live with a warning.

mcspr commented 3 years ago

variants/ is in both, it is something used via {build.variant} in the board.txt definition for the Arduino IDE

LowPowerLab commented 3 years ago

Is there a better way to suppress the warning or check if that SS is defined instead?

mcspr commented 3 years ago

Wait, no. Maybe static_assert(SS != NOT_A_PIN, "");? Sry, phone writing atm

mcspr commented 3 years ago

It does seem pretty standard though, both IDE and PIO always select some variant so we always get the SS as either define or the static int8_t / static uint8_t

LowPowerLab commented 3 years ago

So the only detriment of not merging this is a WARNING? Other than being an eye sore does that affect anything?

mcspr commented 3 years ago

It is an error in the configuration flow. ESP8266 stays un-affected, SPI SS=15 is always the case (wifio is an outlier, but not as common). ESP32 correctly detects available SS pin now, instead of using the hard-coded default this library chooses

LowPowerLab commented 3 years ago

OK how about leaving the #define RF69_SPI_CS where it is, I don't see any reason why this has to be moved. I can merge if you revert that change and just remove the specific defines for ESP.

mcspr commented 3 years ago

Thanks!

I think the idea was to make the 'future' warning refer to the SS line, in case those re-definitions somehow come back into device-specific block. Does not matter though, really, just a style preference :)