Configure different kinds of busses at compile #4107
Conversation
|
A bug that's currently present and that I haven't fixed with this PR is that it's possible to configure analog outputs with an LED count > 1, which is not possible through the GUI. Fixing it should be trivial, but wasn't sure if it was considered intended behavior for some cases I am not aware of. |
It should not be possible to create a PWM output with length > 1. The easiest would be to set |
There always have to be at least one bus created. If invalid configuration is specified either |
Done.
Clever, done.
I would prefer to raise the error, but can't think of a way to do so since I don't think we can count the provided pins with the preprocessor. I have implemented a fix but I'm not too happy with it:
To be honest I don't see the issue with not having a bus created if the user provides an incorrect configuration. The same is also true if the user chooses a pin that cannot be used for LED output on the controller (e.g. pin 37 on ESP32) at compile. |
This is where
If the configuration (compile-time) is incorrect there are only 2 options available: a) raise compiler error (preferred) or b) create a single WS2812 output on default pin. If you choose b) take into account ESP8266 as well. |
Thank you. |
|
I think it is ok but we can also ask C++ experts like @willmmiles or @robertlipe or @TripleWhy to verify. |
|
I have moved the pins counter logic to a separate function Bus::getRequiredPins(). I have replaced the error messages with better ones and updated the default variables in the example at line 46. |
You beat me to it, this is exactly what I was going to suggest! That said: I also think that the full check of 'does the defined pin count match the sum of required pins for the defined buses' probably should be a static_assert instead of a runtime error, too. It is a little more awkward to construct in C++11, alas - it's too bad we don't have global access to newer features. :( // constexpr recursive left fold
// C++>23, consider std::ranges::fold_left instead
static constexpr unsigned sumPinsRequired(const unsigned* current, size_t count) {
return (count > 0) ? (Bus::getRequiredPins(*current) + sumPinsRequired(current+1,count-1)) : 0;
}
void WS2812FX::finalizeInit(void) {
...
constexpr unsigned defDataTypes[] = {LED_TYPES};
constexpr unsigned defDataPins[] = {DATA_PINS};
constexpr unsigned defCounts[] = {PIXEL_COUNTS};
// in C++20 we'd use std::size() instead. sizeof(array)/sizeof(element) is vulnerable to alignment/packing rules
constexpr unsigned defNumTypes = std::extent<decltype(defDataTypes)>::value;
constexpr unsigned defNumPins = std::extent<decltype(defDataPins)>::value;
constexpr unsigned defNumCounts = std::extent<decltype(defCounts)>::value;
static_assert(sumPinsRequired(defDataTypes, defNumTypes) == defNumPins,
"The default pin list defined in DATA_PINS does not match the pin requirements for the default buses defined in LED_TYPES");
} |
|
You don't know how much I like learning this way. Thank you! |
|
Thank you. In the same spirit, should an error be thrown if the defined pin cannot get used for led output? For example the read only pins on ESP32. |
That would make it even better! |
There was a problem hiding this comment.
No real expert-level advice here; my view is mostly fresh eyes.
I come from the land of serial terminals at 300baud where we'd set lines = 5 in vi so the screen redraws were faster, but much past that, I didn't find the need to conserve vertical whitespace the way this code does. Use those linefeeds to express that something is inside a loop or an if or whatever.
But I also get that if you have a huge project that doesn't do that, this code would look weird if you did, so "all local laws apply". :-)
I really like the compile-time check against misconfiguration. I've done code somewhat like this (like DMA descriptors that change in later models of the chip) and it's SUPER handy to have tables self-asserting the things they expect.
contracts were going to be a hyper-assert, but I can't remember if they made the last train or not. static_assert here is perfect.
wled00/FX_fcn.cpp
Outdated
| const unsigned defDataPins[] = {DATA_PINS}; | ||
| const unsigned defCounts[] = {PIXEL_COUNTS}; | ||
| const unsigned defNumPins = ((sizeof defDataPins) / (sizeof defDataPins[0])); | ||
| const unsigned defNumTypes = ((sizeof defDataTypes) / (sizeof defDataTypes[0])); |
There was a problem hiding this comment.
This is fine, and how we've done this since K&R, but hipsters will use std::size
numTypes = std::size(defDataTypes);
or
... ypes = defDataTypes.size();
https://en.cppreference.com/w/cpp/iterator/size
Says that might be C++17, so try it. I know it works in "my" ESP32/platformio project, but I have language set to "--std=c++-give-me-all-the-things"
To be clear: this is just a suggestion to try. Your way is fine.
wled00/FX_fcn.cpp
Outdated
| const unsigned defNumBusses = defNumPins > defNumCounts && defNumPins%defNumCounts == 0 ? defNumCounts : defNumPins; | ||
| const unsigned pinsPerBus = defNumPins / defNumBusses; | ||
|
|
||
| static_assert(Bus::getRequiredPins(defDataTypes[0]) <= defNumPins, |
wled00/FX_fcn.cpp
Outdated
| DEBUG_PRINTLN(F("LED outputs misaligned with defined pins. Some pins will remain unused.")); | ||
| break; | ||
| } | ||
| for (unsigned j = 0; j < busPins; j++) defPin[j] = defDataPins[pinsIndex + j]; |
There was a problem hiding this comment.
Does your style guide allow this on one line? I'd put it on the next (and I'd probably still use braces) just for visual clarity. I missed that there was only one statement in the loop.
I'll leave whitespace haggles to your local reviewer.
wled00/FX_fcn.cpp
Outdated
| break; | ||
| } | ||
| for (unsigned j = 0; j < busPins; j++) defPin[j] = defDataPins[pinsIndex + j]; | ||
| pinsIndex += busPins; | ||
| // when booting without config (1st boot) we need to make sure GPIOs defined for LED output don't clash with hardware | ||
| // i.e. DEBUG (GPIO1), DMX (2), SPI RAM/FLASH (16&17 on ESP32-WROVER/PICO), etc | ||
| if (pinManager.isPinAllocated(defPin[0])) { |
There was a problem hiding this comment.
Do curlies not change the indention level in your style? That's uncommon.
wled00/FX_fcn.cpp
Outdated
| DEBUG_PRINTLN(F("LED outputs misaligned with defined pins. Some pins will remain unused.")); | ||
| break; | ||
| } | ||
| for (unsigned j = 0; j < busPins; j++) defPin[j] = defDataPins[pinsIndex + j]; |
There was a problem hiding this comment.
Since the use of defPin kind of got away from the size that's hard-coded in the decl of defPin, consider an assert in the loop that j < std::size(defPin) (or just plain "5" since this code reallly likes splashing around that magic number...) or at least an assert that busPins < 5 or the sizeof thing. Whatever it takes to be sure you're not walking off the end of defPin and clobbering whatever's adjacent on the stack.
wled00/bus_manager.h
Outdated
| inline uint8_t getAutoWhiteMode() { return _autoWhiteMode; } | ||
| inline static void setGlobalAWMode(uint8_t m) { if (m < 5) _gAWM = m; else _gAWM = AW_GLOBAL_DISABLED; } | ||
| inline static uint8_t getGlobalAWMode() { return _gAWM; } | ||
| inline void setAutoWhiteMode(uint8_t m) { if (m < 5) _autoWhiteMode = m; } |
There was a problem hiding this comment.
I don't know why this code really likes the number 5, but consider the readabiity boos from making it a constexpr kNumWhatevers = 5;
Now it has some documentation for Whatever it is (I'm just a guest reviewer, I don't know this code. Consider me an intern, :-) ) and it'll be easier to change if the world changes and you have to adapted to 6 Whatevers in your domain.
There was a problem hiding this comment.
Thinking of existing old build environments people may have (using DEFAULT_LED_TYPE and DEFAULT_LED_COUNT) or newer ones (using PIXEL_COUNTS and DATA_PINS) it may be time to forego overriding DEFAULT_ constants and stick with newer ones only (including LED_TYPES.
Why? IMO having the ability to set compile time configuration should only be done in one place.
Otherwise you might be tempted to use DEFAULT_XXX_XXX as a placeholder if certain configuration is missing.
For example: -D DATA_PINS=3,4,5 could/should automatically expand to (or cause the same effect as) -D DATA_PINS=3,4,5 -D PIXEL_COUNTS=DEFAULT_LED_COUNT,DEFAULT_LED_COUNT,DEFAULT_LED_COUNT -D LED_TYPES=DEFAULT_LED_TYPE,DEFAULT_LED_TYPE,DEFAULT_LED_TYPE
I know this may be hard to set up but it would be logical and space/time saving.
You would only need to set up DATA_PINS and DEFAULT_LED_TYPE for example.
Another POV is to always have all and correct build time options otherwise throw (descriptive) error.
TBH I am torn between space/time savings and clarity of the set up process. I would like to have both.
wled00/bus_manager.h
Outdated
| inline static void setGlobalAWMode(uint8_t m) { if (m < 5) _gAWM = m; else _gAWM = AW_GLOBAL_DISABLED; } | ||
| inline static uint8_t getGlobalAWMode() { return _gAWM; } | ||
| // 1 pin per channel on analog, 2 pins on clocked digital strips, 1 pin for the rest | ||
| inline static constexpr uint8_t getRequiredPins(uint8_t type) { return IS_PWM(type) ? NUM_PWM_PINS(type) : IS_2PIN(type) + 1; } |
There was a problem hiding this comment.
I know this may be odd for build time configuration but virtual buses use 4 "pins" for IP address.
IMO this should be reflected here as well.
wled00/FX_fcn.cpp
Outdated
| unsigned dataType = defDataTypes[(i < defNumTypes) ? i : defNumTypes -1]; | ||
| unsigned busPins = Bus::getRequiredPins(dataType); | ||
| // check if we have enough pins left to configure an output of this type | ||
| if (pinsIndex + busPins > defNumPins) { |
There was a problem hiding this comment.
IMO this should be asserted as well as build time misconfiguration should not be allowed.
wled00/bus_manager.h
Outdated
| @@ -10,6 +10,14 @@ | |||
| //colors.cpp | |||
| uint16_t approximateKelvinFromRGB(uint32_t rgb); | |||
|
|
|||
| // 1 pin per channel on analog, 4 "pins" (IPv4 fields) on virtual, 2 pins on clocked digital strips, 1 pin for the rest | |||
| static constexpr uint8_t getRequiredPins(uint8_t const type) { | |||
There was a problem hiding this comment.
You could have used forward declaration and still use class member method.
Yes will rebase to bus-config, I just quickly pushed code I had already worked on. |
PaoloTK
force-pushed
the
compile_different_busses
branch
from
33e1671
to
c51ce2e
Compare
|
Thanks everyone for the great feedback and the opportunity to learn. I think I have implemented everything that was requested and some more on top. I will put the code through the tests in the first post again after all the changes, and will also test pin conflict more deeply since I have touched the logic for it. |
- Renamed LEDPIN to DEFAULT_LED_PIN. - Removed ability to override DEFAULT_LED_PIN, DEFAULT_LED_TYPE and DEFAULT_LED_COUNT. Use DATA_PINS, LED_TYPES and PIXEL_COUNTS instead.
|
On Tue, Sep 10, 2024 at 12:11 PM Blaž Kristan ***@***.***> wrote:
-void BusDigital::show() {
+void BusDigital::show(void) {
Is there a difference? I never used (void) before but seen it in some
places so I thought...
TL;DR: most C++ style guides discourage it. It's dead typing/reading.
Please don't let me break inertia on PR this with a style flame. This
actually came up in an early code submission of mine (then, primarily a C
programmer that did some C++) when I started at at C++ house that all but
banished C. You'd have thought I was Sinbad O'Connor on SNL from the
reaction. They didn't like my "C accent". :-) It was officially
discouraged in the style guide. C++ nerds feel strongly about it, even
though they're identical *in c++*.
They are different in C. show(void) is a function explicitly defined to
have NO parameters and show() is a function that may have zero or
more parameters, but only if it's a definition (int show();) and not a
declaration. int show() int arg1; {return 42;}. actually has two - and is
less 'greppable' (or ack or whatever other code browser you like to use.)
Remember that you may have one or more defn before seeing a decl and the
common use case is to take advantage of an additional quirk that that
defns may repeat if they differ in their defined scope, notably internal or
external linkage. In C,
extern int open();
[ a hundred lines of jibber-jabber;
int open();
[ a hundred more lines of jibber-jabber; ]
open(const char* path; int oflag; ...);
is not a style particularly UNcommon to see in (preprocessed) C.
When thinking this through, you also have to stake a stand on function
pointers:
void (*blinkPtr)() = doBlink;
vs
void (*blinkPtr)(void) = doBlink;
Some older C++ compilers - which mostly support interop with C via extern
"C" {} and leave the fine details implementation defined or simply
undefined - used to generate stupid call frames if it saw both the pre-ansi
C version (show();) and the ISO C89 version (show(void);) because it
couldn't figure out how many registers to mark dirty and how large of a
stack to reserve (the correct answer being "none", but it'd lose its mind
and generate a bunch of code to do that "nothing") but I think the language
has been tightened up enough to make that surely unnecessary by now.
My point really was that there's no reason to ADD them, particularly if you
then potentially had a mismatched C++ and .h version of the same. The
majority of this code already uses the 'none' form, e.g. just a few lines
later:
BusManager::show();
unsigned long showNow = millis();
... and ...
bool WS2812FX::isUpdating() const {
return !BusManager::canAllShow();
so I guess my nudge was rooted in you being firmly into the C++ camp at
this point (no turning back!) and having many, many instances of bare
parens now. If you started adding "void" here and there, you'd spend an
afternoon writing the perl/sed macros to get them all building sensibly and
tested everywhere and that's just burning fuel to go nowhere.
So I'd encourage you to not do that. But if you want to, I can't/won't stop
you.
Signed
Miss Code Manners :-)
|
|
I have now gone through all the tests in the OP again and confirmed everything works as expected. Now every time there is a misalignment compile fails. Will update when I have tested pin conflicts. If anyone has feedback on the following I'm all ears:
|
Agreed. I was thinking, since it is now possible to configure multiple led types, is it worth also implementing the same logic for color order? Should be pretty straight forward. |
IMO no. As you may not know what kind of actual LEDs will be connected. I.e. some are RGB while others of the same type may be GRB. |
- rely on HAL for RO pins and max pins - remove isPinDefined() and do clash check inline - JS fix to use HAL max pins
wled00/pin_manager.cpp
Outdated
|
|
||
| // Given an array of pins, check if a given pin is defined except at given index | ||
| bool PinManagerClass::isPinDefined(const byte gpio, const uint8_t *pins, const unsigned index) { | ||
| unsigned numPins = ((sizeof pins) / (sizeof pins[0])); |
There was a problem hiding this comment.
This will not work correctly if I am not mistaken.
sizeof pins is equal to sizeof(uint8_t*), which is 4 (a pointer), and not 5 as defined in finalizeInit().
I'm also wondering why do you really need to query defined pins here? Why not in the place where this actually matters - in finalizeInit()?
wled00/FX_fcn.cpp
Outdated
| DEBUG_PRINTLN(F("Some of the provided pins cannot be used to configure this LED output.")); | ||
| defPin[j] = 1; // start with GPIO1 and work upwards | ||
| validPin = false; | ||
| } |
There was a problem hiding this comment.
I think this needs continue; here as otherwise defPin[j] will be incremented in next if.
Or use else.
| @@ -787,7 +787,7 @@ void BusNetwork::cleanup(void) { | |||
|
|
|||
| //utility to get the approx. memory usage of a given BusConfig | |||
| uint32_t BusManager::memUsage(BusConfig &bc) { | |||
| if (Bus::isOnOff(bc.type) || Bus::isPWM(bc.type)) return 5; | |||
| if (Bus::isOnOff(bc.type) || Bus::isPWM(bc.type)) return OUTPUT_MAX_PINS; | |||
There was a problem hiding this comment.
Reviewing original code, BusOnOff uses only 1 byte of data (bug in original code). Does not hurt to report 5, though.
| unsigned start = prevLen; | ||
| // if we have less counts than pins and they do not align, use last known count to set current count | ||
| unsigned count = defCounts[(i < defNumCounts) ? i : defNumCounts -1]; | ||
| // analog always has length 1 | ||
| if (Bus::isPWM(dataType)) count = 1; |
There was a problem hiding this comment.
BusOnOff also has a length of 1.
wled00/const.h
Outdated
| #ifndef LEDPIN | ||
| #if defined(ESP8266) || defined(CONFIG_IDF_TARGET_ESP32C3) //|| (defined(ARDUINO_ARCH_ESP32) && defined(BOARD_HAS_PSRAM)) || defined(ARDUINO_ESP32_PICO) | ||
| #define LEDPIN 2 // GPIO2 (D4) on Wemos D1 mini compatible boards, safe to use on any board | ||
| // List of read only pins. Cannot be used for LED outputs. |
There was a problem hiding this comment.
Can't we rely on HAL? Do we really need #define? It may make the code larger but then requires no maintenance.
wled00/pin_manager.cpp
Outdated
| @@ -267,6 +267,29 @@ bool PinManagerClass::isPinOk(byte gpio, bool output) const | |||
| return false; | |||
| } | |||
|
|
|||
| bool PinManagerClass::isReadOnlyPin(byte gpio) | |||
| { | |||
| #ifdef READ_ONLY_PINS | |||
There was a problem hiding this comment.
I would prefer to query HAL here.
Like:
#ifdef ARDUINO_ARCH_ESP32
if (gpio < WLED_NUM_PINS) return digitalPinCanOutput(gpio);
#endifThere was a problem hiding this comment.
I have tested digitalPinCanOutput but unfortunately it does not return FALSE only for read/only pins but also for other types of pins (e.g. ones that don't have pads). While this doesn't matter for finalizeInit(), the GUI displays read/only pins in orange, which cannot be achieved from what I can see without hardcoding the pins currently. We could of course just leave the pins hardcoded there, but since they might be useful in other places I figured centralizing them in const.h was a better option.
EDIT: I guess a possible solution in the GUI is to first color the pins orange if they're not a valid output, and then color them red if they're not a valid pin. That way read/only pins will stay orange while the other invalid pins will stay red.
There was a problem hiding this comment.
Don't forget that GUI also has reserved pins, those are not populated using read-only information.
wled00/pin_manager.cpp
Outdated
| const unsigned numPins = ((sizeof pins) / (sizeof pins[0])); | ||
|
|
||
| if (gpio <= WLED_NUM_PINS) { | ||
| for (unsigned i = 0; i < numPins; i++) if (gpio == pins[i]) return true; |
There was a problem hiding this comment.
If not using HAL this could be simplified (as I've learned recently) as:
for (const auto pin : pins) if (gpio == pin) return true;
wled00/FX_fcn.cpp
Outdated
| // When booting without config (1st boot) we need to make sure GPIOs defined for LED output don't clash with hardware | ||
| // i.e. DEBUG (GPIO1), DMX (2), SPI RAM/FLASH (16&17 on ESP32-WROVER/PICO), read/only pins, etc. | ||
| // Pin should not be already allocated, read/only or defined for current bus | ||
| while (pinManager.isPinAllocated(defPin[j]) || pinManager.isReadOnlyPin(defPin[j]) || pinManager.isPinDefined(defPin[j], defPin, j)) { |
There was a problem hiding this comment.
Why not use pinManager.isPinOk(defPin[j],true) instead of pinManager.isReadOnlyPin(defPin[j])? It serves the same purpose.
Use of pinManager.isPinDefined() is IMO incorrect (see also other comment).
I would rather add
// is the newly assigned pin already defined? try next in line until there are no clashes
bool clash;
do {
clash = false;
for (const auto pin : defDataPins) {
if (pin == defPin[j]) {
defPin[j]++;
if (defPin[j] < WLED_NUM_PINS) clash = true;
}
}
} while (clash);after incrementing defPin[j] to check if newly assigned pin clashes with one of the defined ones.
|
Otherwise I think code is well made.
Likewise.
Again, this is not to detract or derail. Just random comments from the
peanut gallery.
+ unsigned numPins = ((sizeof pins) / (sizeof pins[0]));
This will not work correctly if I am not mistaken.
sizeof pins is equal to sizeof(uint8_t*), which is 4 (a pointer), and not
5 as defined in finalizeInit().
If pins is something like an STL container (including a std::array),
pins.size() or std::size(pins) should work. I think those were C++17.
They're computed at compile-time, so there's no overhead.
+ if (gpio <= WLED_NUM_PINS) {
+ for (unsigned i = 0; i < numPins; i++) if (gpio == pins[i]) return true;
If not using HAL this could be simplified (as I've learned recently) as:
for (const auto pin : pins) if (gpio == pin) return true;
Nice! Someone's listening and learning. :-)
For a POD type, you dont' have to worry about the cost of a ctor/copy, but
generically, get in the habit of writing
for (const auto& pin : pins)
That way pin is THE element in pins[] and it doesn't make/destroy a copy of
it. Since it's const, it's cheaper to operate on the reference in place
instead of call the constructor for pin, the copy operator, and then the
destructor. The difference in generated code (if there's a non-trivial
create/copy cost) make it worth remembering the & ref operator.
I won't hold class for it as it's a bit silly for such a simple test here,
but this is a good place to forward-reference reading up on <algorithms>.
std::find, std::find_if, std::search, and std::for_each can do magical
things on CPUs that are smarter than ESP32's. (Honestly, a good branch
predictor and good data structures that are cache friendly make a linear
scan of a table in cache freakishly fast on even fairly humble processors
these days.)
The current versions are a bit boilerplate-heavy, but there are papers
pending to lighten that up. But watching the code this stuff can generate
on an AVX-512-class of machine is just jaw-dropping. It can unroll and
speculate the heck out of those loops! It can even splatter BIG data across
multiple cores automatically. Clearly, we don't want it running around
spawning threads for us on an ESP32, though!
The syntax (and I'm free-forming this at a crazy hour) is something like:
return std::find_if(pin.begin(), pin.end(), gpio) != std::end(pin)
This is is a pretty silly thing to do here, but once you get into segments
(which map very naturally into ranges) and want to do complicated things
over larger data structures, it might be worth brushing up.
https://en.cppreference.com/w/cpp/algorithm
has some amazingly handy, compact forms of little loops and things we write
all the time, like generating a sum from a table, copying elements in a
range backward (I'd imagine that mirroring a segment does this...), flls,
accumulations, generate patterns, merges, min/max elements in an array, and
so on. Learning the presence of one a day - even if you don't use it
immediately - will make your code generally safer and shorter. I'd bet a
lot of the effects would benefit from rotate(), shift_left(), and
shift_right(), for example.
The whole idea is that instead of telling the compiler HOW to do something,
you describe WHAT you're doing, and let IT figure out the HOW. It really
can let the optimizer eliminate read dependency stalls on loop iterators
because you've described the task you're trying to solve (e.g. "add these
array elements up") instead of how to do it.
There's a proposal that does type inference on the array and lets you do
the equivalent of
return std::find_if(pin, gpio) ... for most of those helpers in
<algorithms> just to vaporize the iterator-heavy boilerplate of .begin/.end
but that's not reality yet - and certainly not reality in a platformio
world where the compilers are held back seven years.
TL;DR:
Consider foo.size() over sizeof(foo) / sizeof(foo[0]).
Default to taking a const auto ref (yes, I wish that were the default) in
"for (const auto& cat : cats)"
Bonus: Read up on <algorithm> and the next time you find yourself writing
things we've written a billion times, see if there's an <algorithm> for
that.
...but remember that not everything is available on Platformio with G++ 8.x
… Message ID: ***@***.***>
|
|
@robertlipe I like your humorous and poetic writing style. 😄 |
Use PinManager to determine reserved pins
wled00/pin_manager.cpp
Outdated
| bool PinManagerClass::isReadOnlyPin(byte gpio) | ||
| { | ||
| #ifdef ARDUINO_ARCH_ESP32 | ||
| if (gpio < WLED_NUM_PINS) return digitalPinCanOutput(gpio); |
There was a problem hiding this comment.
This should be !digitalPinCanOutput().
Unfortunately even with that fixed it still doesn't work because this function also returns FALSE on other pins than R/O (for example 28 to 31 on ESP32).
While as far as the finalizeInit() code this doesn't matter I think we should not return TRUE for pins that are not read/only on a function called isReadOnlyPin(). This could lead to bugs in the future (e.g. usermod wanting to use this function to check if a pin can be used to read data).
There are two ways I can think of to get around this problem:
- Hardcode the read only pins (which I know you're not a fan of).
- Remove the orange coloring in the GUI for read/only pins. We don't need to check if a pin is r/o in finalizeInit() and if the r/o pins are red (since they fail the digitalPinCanOutput() check) in the GUI instead of orange I don't think it's a big deal. As far as I can tell there's no other place in the codebase that does a r/o check, but please correct me if I'm wrong.
EDIT: I'll try implement what I suggested above: first color digitalPinCanOutput() pins orange, then color digitalPinIsValid() red.
There was a problem hiding this comment.
Unfortunately even with that fixed it still doesn't work because this function also returns FALSE on other pins than R/O (for example 28 to 31 on ESP32).
This is used in pin manager since ever:
Line 259 in df24fd7
If you do not trust HAL (you may have reasons) then use isPinOk(gpio) != isPinOk(gpio,false) to determine RO pins. This way there will only be one place to configure GPIOs and that is isPinOk().
There was a problem hiding this comment.
Unfortunately even with that fixed it still doesn't work because this function also returns FALSE on other pins than R/O (for example 28 to 31 on ESP32).
This is used in pin manager since ever:
Line 259 in df24fd7
If you do not trust HAL (you may have reasons) then use
isPinOk(gpio) != isPinOk(gpio,false)to determine RO pins. This way there will only be one place to configure GPIOs and that isisPinOk().
Sorry, I had already implemented the logic fix before reading your message. Both options work the same in my testing so your call on which you prefer:
pinManager.isPinOk(i) != pinManager.isPinOk(i,false)
digitalPinIsValid(gpio) && !digitalPinCanOutput(gpio)
Ability to configure different kind of busses at compile time has been added.
The logic to assign pins to busses has been changed. It now iterates up to the max allowed busses and stops if there are not enough pins left to configure the next bus.
I changed the macro name to better reflect the ability to configure multiple types, like it was done for the other macros that were changed to accept lists in the past. I have also implemented a DEFAULT_LED_TYPE macro in const.h. While this macro is only used here and could be therefore placed in FC_fcn.cpp, the same is true of LEDPIN so I preferred to keep them together.
I have tested the code quite a bit and haven't found any issues with it. These are the scenarios I have tested: