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

Configure different kinds of busses at compile #4107

Merged
merged 16 commits into from
Sep 14, 2024

Conversation

PaoloTK
Copy link

@PaoloTK PaoloTK commented Aug 18, 2024

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:

Test Data Type Data Pins Counts Configuration Configured outputs Notes
Default settings TYPE_WS2812_RGB 16 30 1x 1ch type, 1x pin, 1x count 1
Default + Types TYPE_SK6812_RGBW DEFAULT DEFAULT 1x 1ch type, 1x pin, 1x count 1 Same as default but another type has been specified
Default + Counts DEFAULT DEFAULT 1,30 1x 1ch type, 1x pin, 2x counts 1 More counts than possible outputs with supplied pins, only one output configured
Default + Pins DEFAULT 2,4,12,32,33,5 DEFAULT 1x 1ch type, 6x pins, 1x count 6 Everything aligned, verified last type (and count) is used if user doesn't specify enough
Everything aligned with different bus types TYPE_ANALOG_5CH,TYPE_SK6812_RGBW 2,4,12,32,33,5 1,30 1x 5ch type + 1x 1ch type, 6x pins, 2x counts 2 Everything aligned with busses with different pins requirements and counts
Default + Multipin Type TYPE_ANALOG_2CH DEFAULT DEFAULT 1x 2ch type, 1x pin, 1x count 0 Default data pins is only 1, not enough to configure analog 2ch, therefore no outputs are configured
Default + 2 Types, first Multipin TYPE_ANALOG_5CH,TYPE_SK6812_RGBW DEFAULT DEFAULT 1x 5ch type + 1x 1ch type, 1x pin, 1x count 0 Same as previous test but because of that, second type is also not configured even if there are enough free pins in theory
Misaligned pins and type TYPE_ANALOG_2CH 2,4,12,32,33 30 1x 2ch type, 5x pins, 1x count 2 3x 2ch would require 6 pins, but since only 5 are supplied 2x outputs are configured and the last pin is not used
Misaligned counts and type TYPE_APA102 2,4,12,32,33 5,20,100 1x 2ch type, 5x pins, 3x count 2 Same as previous test but also third count provided (and not used)
Misaligned types and pins TYPE_ANALOG_2CH, TYPE_ANALOG_3CH,TYPE_SK6812_RGBW 2,4,12,32,33 5,20,100 1x 2ch type + 1x 3ch type + 1x 1ch type, 5x pins, 3x count 2 First 2 types align, therefore 3rd type (and count) is not used
More pins than types TYPE_ANALOG_5CH,TYPE_SK6812_RGBW 2,4,12,32,33,5,16 5,20,100 1x 5ch type + 1x 1ch type, 7x pins, 3x count 3 Keep configuring last type if there are more pins left. Counts are aligned so all are used
More pins than types + misaligned pins and type TYPE_WS2812_RGB,TYPE_ANALOG_2CH 2,4,12,32,33,5 5,20 1x 1ch type + 1x 2ch type, 6x pins, 2x count 3 Same as last test but last output not configured because pins are misaligned
More outputs than board allows (C3) TYPE_ANALOG_1CH 3,6,1,4,5,7 1 1x 1ch type, 5x pins, 1x count 4 Enough pins for 5x outputs, but C3 only allows 4x analog outputs

@PaoloTK
Copy link
Author

PaoloTK commented Aug 18, 2024

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.

@blazoncek
Copy link
Collaborator

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 if (IS_PWM(dataType)) count = 1; similar as you did for busPins.
You can also modify that to read: unsigned busPins = IS_PWM(dataType) ? NUM_PWM_PINS(dataType) : IS_2PIN(dataType) + 1;

@blazoncek
Copy link
Collaborator

| Default + Multipin Type | TYPE_ANALOG_2CH | DEFAULT | DEFAULT | 1x 2ch type, 1x pin, 1x count | 0 | Default data pins is only 1, not enough to configure analog 2ch, therefore no outputs are configured

There always have to be at least one bus created. If invalid configuration is specified either #error during compile or create WS2812 bus.

@PaoloTK
Copy link
Author

PaoloTK commented Aug 19, 2024

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 if (IS_PWM(dataType)) count = 1; similar as you did for busPins.

Done.

You can also modify that to read: unsigned busPins = IS_PWM(dataType) ? NUM_PWM_PINS(dataType) : IS_2PIN(dataType) + 1;

Clever, done.

| Default + Multipin Type | TYPE_ANALOG_2CH | DEFAULT | DEFAULT | 1x 2ch type, 1x pin, 1x count | 0 | Default data pins is only 1, not enough to configure analog 2ch, therefore no outputs are configured

There always have to be at least one bus created. If invalid configuration is specified either #error during compile or create WS2812 bus.

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:

  1. Hardcoded led type (can't reference DEFAULT_LED_TYPE since it's gonna be changed by the user)
  2. While the type is overridden, the pin and count isn't. If correct default pin for board should be used instead, I'd have to re-implement the preprocessor logic here, which is easy but doesn't feel like a clean solution.
  3. I have maxed out the iterator so that only one output gets configured. This is done to avoid the specified types beyond the first getting misaligned. As an example, if one configures 1x 5ch analog + 1x sk6812 but only provides 4 pins, the result will be one output on WS2812B and 3 on sk6812.

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.

@blazoncek
Copy link
Collaborator

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.

This is where static_assert() comes in.

  1. You've introduced LED_TYPES, DEFAULT_LED_TYPE is the one to use, even if user overrides it. You can use static_assert() to force compilation errors if parameters mismatch.
  2. Similar as above.
  3. I do not like the current solution. It is actually incorrect. Only one bus of type WS2812 should be created instead (or compiler error raised).

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.

@PaoloTK
Copy link
Author

PaoloTK commented Aug 19, 2024

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.

This is where static_assert() comes in.

  1. You've introduced LED_TYPES, DEFAULT_LED_TYPE is the one to use, even if user overrides it. You can use static_assert() to force compilation errors if parameters mismatch.
  2. Similar as above.
  3. I do not like the current solution. It is actually incorrect. Only one bus of type WS2812 should be created instead (or compiler error raised).

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 have implemented a first try on the static_assert, although I think it's quite ugly. I'm thinking I could move the pin counter logic to a separate function if I have to use it twice. but regardless I pushed this to know if you think I'm on the right track.
The error is thrown correctly. I tried placing the logic in the #IFNDEF block for LED_TYPES but that required additional defs since I don't have access to defNumTypes there.

@blazoncek
Copy link
Collaborator

I think it is ok but we can also ask C++ experts like @willmmiles or @robertlipe or @TripleWhy to verify.
It would also benefit if @softhack007 has a say.

@PaoloTK
Copy link
Author

PaoloTK commented Aug 20, 2024

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.

@willmmiles
Copy link
Collaborator

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");
}

@blazoncek
Copy link
Collaborator

You don't know how much I like learning this way. Thank you!

@PaoloTK
Copy link
Author

PaoloTK commented Aug 21, 2024

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.

@blazoncek
Copy link
Collaborator

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!

Copy link

@robertlipe robertlipe left a comment

Choose a reason for hiding this comment

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

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.

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]));

Choose a reason for hiding this comment

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

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.

const unsigned defNumBusses = defNumPins > defNumCounts && defNumPins%defNumCounts == 0 ? defNumCounts : defNumPins;
const unsigned pinsPerBus = defNumPins / defNumBusses;

static_assert(Bus::getRequiredPins(defDataTypes[0]) <= defNumPins,

Choose a reason for hiding this comment

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

Nice!

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];

Choose a reason for hiding this comment

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

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.

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])) {

Choose a reason for hiding this comment

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

Do curlies not change the indention level in your style? That's uncommon.

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];

Choose a reason for hiding this comment

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

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.

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; }

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@blazoncek blazoncek left a comment

Choose a reason for hiding this comment

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

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.

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; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO this should be asserted as well as build time misconfiguration should not be allowed.

Copy link
Collaborator

@blazoncek blazoncek left a comment

Choose a reason for hiding this comment

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

I do understand the desire for a constant but not for moving a method out of class.
Please look at the bus-config branch for the direction this should go in.

@@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could have used forward declaration and still use class member method.

@PaoloTK
Copy link
Author

PaoloTK commented Aug 28, 2024

I do understand the desire for a constant but not for moving a method out of class. Please look at the bus-config branch for the direction this should go in.

Yes will rebase to bus-config, I just quickly pushed code I had already worked on.

@PaoloTK
Copy link
Author

PaoloTK commented Sep 9, 2024

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.

wled00/bus_manager.cpp Outdated Show resolved Hide resolved
- 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.
@robertlipe
Copy link

robertlipe commented Sep 11, 2024 via email

@PaoloTK PaoloTK changed the base branch from 0_15 to bus-config September 11, 2024 19:53
@PaoloTK
Copy link
Author

PaoloTK commented Sep 11, 2024

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:

  • Position of sumPinsRequired() and validatePinsAndTypes(). They're helper functions for finalizeInit() and I can't think of a reason why they'd ever be used anywhere else. At the same time there might be a better place for them.
  • validaPinsAndTypes() returns TRUE if excess pins modulo last type required pins == 0. This is because finalizeInit() will keep using the last provided type if the user hasn't provided enough. This behavior is logical and I see little reason why it would ever change, but I'm not sure how I feel about having the logic of a separate function be based on that.
  • READ_ONLY_PINS macro added due to no built in function in the HAL to report ONLY r/o pins. The ones available also return invalid pins (e.g. pins not exposed on ESP32).
  • I think the way to disallow the user to define something through pio.ini is to remove #ifndef for the given macro, is that correct? This is in the context of disallowing the users from overriding DEFAULT_LED_TYPE and similar (they should now use LED_TYPES instead as mentioned above).

blazoncek referenced this pull request Sep 12, 2024
impure void remove
optimisations: hot
Copy link
Collaborator

@blazoncek blazoncek left a comment

Choose a reason for hiding this comment

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

Looks good to me.
There may be issues with user's build environments if they have used DEFAULT_LED_TYPE (or PIN) overrides but IMO that is acceptable as there is now a different, more flexible way of specifying LED types and pins at compile time.

@PaoloTK
Copy link
Author

PaoloTK commented Sep 13, 2024

Looks good to me. There may be issues with user's build environments if they have used DEFAULT_LED_TYPE (or PIN) overrides but IMO that is acceptable as there is now a different, more flexible way of specifying LED types and pins at compile time.

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.

@blazoncek
Copy link
Collaborator

s it worth also implementing the same logic for color order?

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.

@blazoncek blazoncek changed the base branch from bus-config to 0_15 September 13, 2024 21:32
- rely on HAL for RO pins and max pins
- remove isPinDefined() and do clash check inline
- JS fix to use HAL max pins
Copy link
Collaborator

@blazoncek blazoncek left a comment

Choose a reason for hiding this comment

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

I went thoroughly through the code and provided some comments on (IMO) pitfalls.
I've already implemented (what I consider) fixes and will push them for review and test.

Otherwise I think code is well made.


// 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]));
Copy link
Collaborator

Choose a reason for hiding this comment

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

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()?

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;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't we rely on HAL? Do we really need #define? It may make the code larger but then requires no maintenance.

@@ -267,6 +267,29 @@ bool PinManagerClass::isPinOk(byte gpio, bool output) const
return false;
}

bool PinManagerClass::isReadOnlyPin(byte gpio)
{
#ifdef READ_ONLY_PINS
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer to query HAL here.
Like:

#ifdef ARDUINO_ARCH_ESP32
  if (gpio < WLED_NUM_PINS) return digitalPinCanOutput(gpio);
#endif

Copy link
Author

@PaoloTK PaoloTK Sep 14, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't forget that GUI also has reserved pins, those are not populated using read-only information.

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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If not using HAL this could be simplified (as I've learned recently) as:

for (const auto pin : pins) if (gpio == pin) return true;

// 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)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

@robertlipe
Copy link

robertlipe commented Sep 14, 2024 via email

@blazoncek
Copy link
Collaborator

@robertlipe I like your humorous and poetic writing style. 😄
Thank you for teachings! I may not study/learn everything (as I don't do coding for living) but will surely take a peek.

Use PinManager to determine reserved pins
bool PinManagerClass::isReadOnlyPin(byte gpio)
{
#ifdef ARDUINO_ARCH_ESP32
if (gpio < WLED_NUM_PINS) return digitalPinCanOutput(gpio);
Copy link
Author

@PaoloTK PaoloTK Sep 14, 2024

Choose a reason for hiding this comment

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

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:

  1. Hardcode the read only pins (which I know you're not a fan of).
  2. 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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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:

if (output) return digitalPinCanOutput(gpio);

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().

Copy link
Author

Choose a reason for hiding this comment

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

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:

if (output) return digitalPinCanOutput(gpio);

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().

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)

@blazoncek blazoncek merged commit 28cb3f9 into Aircoookie:0_15 Sep 14, 2024
3 of 20 checks 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.

4 participants