Move buttons and leds indexes to enum type

[CezaryGapinski]

Moved button and led indexes and total number of buttons and leds to enum type in particular headers. Kconfig files adjusted to this changes.

[FreddieChopin]

After more detailed inspection the changes are not yet complete...

The most important issue is that arrays buttonPins[], ledPins[], buttons[] and leds[] should have their contents also guarded with the same conditions as the values in ButtonIndexes and LedIndexes. Now this is problematic, because currently the contents of buttons[] and leds[] look more-or-less like this:

chip::ChipOutputPin leds[totalLeds]
{
        chip::ChipOutputPin{ledPins[0]},
        chip::ChipOutputPin{ledPins[1]},
        chip::ChipOutputPin{ledPins[2]},
        chip::ChipOutputPin{ledPins[3]},
};

Note that this array assumes that there are 4 initializers, which may not be true if the LEDs are on different GPIO ports and one of them is disabled.

I propose to do it like that:

1. Completely remove buttonPins[] and ledPins[], put actual initializers into buttons[] and leds[] (this can be done in many separate commits - one for each feature for each board).
2. Guard the contents of buttons[] and leds[] with the same guards as you used for the enums (this has to be done in the same commits as the ones you have done currently, so two separate commits).
3. Regenerate configs.

Sorry for not noticing it earlier... If you want, I can fix that for you.

While we're at it, a few stylistic things I would like to keep consistent:

1. For simple conditions, use #ifdef CONFIG_CHIP_STM32_GPIOV2_GPIOD_ENABLE instead of #if defined(CONFIG_CHIP_STM32_GPIOV2_GPIOD_ENABLE). This "rule" is broken in some places, but only because of consistency with the "bigger whole" (like in chip vectors), because other similar places in the same file may have multiple conditions for the similar purpose.
2. Please format #endif comments like this: #endif<tab>// def CONFIG_CHIP_STM32_GPIOV2_GPIOD_ENABLE (tab instead of space)
3. Please update copyright year of the modified files where it is necessary.

Thanks in advance!

[FreddieChopin]

BTW:

For simple conditions, use #ifdef CONFIG_CHIP_STM32_GPIOV2_GPIOD_ENABLE instead of #if defined(CONFIG_CHIP_STM32_GPIOV2_GPIOD_ENABLE). This "rule" is broken in some places, but only because of consistency with the "bigger whole" (like in chip vectors), because other similar places in the same file may have multiple conditions for the similar purpose.

I'm open to changing that so everywhere in the project the longer #if defined(...) form would be used. If you have any opinion about that just let me know. Such change can be done almost automatically, so that's not a big effort.

However:

Please format #endif comments like this: #endif// def CONFIG_CHIP_STM32_GPIOV2_GPIOD_ENABLE (tab instead of space)

I could be open to changing that as well, but while this is simple for one-line elements, there are things like the chip vectors of clocks.hpp, where you have:

#endif  // defined(STM32F401_VECTORS) || defined(STM32F411_VECTORS) || defined(STM32F412_VECTORS) ||
        // defined(STM32F413_STM32F423_VECTORS) || defined(STM32F427_STM32F429_STM32F43_VECTORS) ||
        // defined(STM32F446_VECTORS) || defined(STM32F46_STM32F47_VECTORS)

This would look pretty stupid with space in the first line and tabs in the remaining ones...

[CezaryGapinski]

Oh, I also forgot about that :(.

2. Guard the contents of buttons[] and leds[] with the same guards as you used for the enums (this has to be done in the same commits as the ones you have done currently, so two separate commits).

While we're at it, a few stylistic things I would like to keep consistent:

1. For simple conditions, use #ifdef CONFIG_CHIP_STM32_GPIOV2_GPIOD_ENABLE instead of #if defined(CONFIG_CHIP_STM32_GPIOV2_GPIOD_ENABLE). This "rule" is broken in some places, but only because of consistency with the "bigger whole" (like in chip vectors), because other similar places in the same file may have multiple conditions for the similar purpose.
   2. Please format #endif comments like this: #endif// def CONFIG_CHIP_STM32_GPIOV2_GPIOD_ENABLE (tab instead of space)
   3. Please update copyright year of the modified files where it is necessary.

It is not better to guard this content and fix this stylistic things on actual branch move-buttons-and-leds-indexes-to-enum-type or now is a problem when pull request is being made? Actual commits are inconsistent and just make a mess without a guard #defines in buttons[] and leds[] arrays.

1. Completely remove buttonPins[] and ledPins[], put actual initializers into buttons[] and leds[] (this can be done in many separate commits - one for each feature for each board).
2. Regenerate configs.

Ok, I planned to do this in next step but then the configurations for pins would be "fully explicit" like when I tried something previously with board generation script and here where ledPins[] array is removed?

[FreddieChopin]

It is not better to guard this content and fix this stylistic things on actual branch move-buttons-and-leds-indexes-to-enum-type or now is a problem when pull request is being made? Actual commits are inconsistent and just make a mess without a guard #defines in buttons[] and leds[] arrays.

It is not a problem when you force-push your branch with new commits. But if you are concerned, we can close this PR and you may start another branch. What suits you better.

Ok, I planned to do this in next step but then the configurations for pins would be "fully explicit" like when I tried something previously with board generation script and here where ledPins[] array is removed?

I suggest doing that now (before the changes to enums), as it seems more logical in my opinion. Or maybe I'm too paranoid... Generally the buttonPins[] and ledPins[] serve no purpose now, so instead of fixing them now and leaving things in a little bit inconsistent state, it would be better to just drop them in the first step. But as for all currently supported boards it makes no difference (they always have all buttons and all LEDs on the same GPIO port anyway), we may as well leave that for later. Just move the array "inside" the #ifdef CONFIG_BOARD_LEDS_ENABLE condition, as otherwise it will have zero size when LEDs/buttons are disabled.

[CezaryGapinski]

Hi!

I suggest doing that now (before the changes to enums), as it seems more logical in my opinion. Or maybe I'm too paranoid... Generally the buttonPins[] and ledPins[] serve no purpose now, so instead of fixing them now and leaving things in a little bit inconsistent state, it would be better to just drop them in the first step. But as for all currently supported boards it makes no difference (they always have all buttons and all LEDs on the same GPIO port anyway), we may as well leave that for later. Just move the array "inside" the #ifdef CONFIG_BOARD_LEDS_ENABLE condition, as otherwise it will have zero size when LEDs/buttons are disabled.

I've created two more commits: Explicit configuration of leds for all boards and Explicit configuration of buttons for all boards. Commits are before the changes to enums. I've a hope that looks more logical. To the Move led and button indexes... commits includes the changes with #ifdef conditions in cpp files.

BTW:

For simple conditions, use #ifdef CONFIG_CHIP_STM32_GPIOV2_GPIOD_ENABLE instead of #if defined(CONFIG_CHIP_STM32_GPIOV2_GPIOD_ENABLE). This "rule" is broken in some places, but only because of consistency with the "bigger whole" (like in chip vectors), because other similar places in the same file may have multiple conditions for the similar purpose.

I'm open to changing that so everywhere in the project the longer #if defined(...) form would be used. If you have any opinion about that just let me know. Such change can be done almost automatically, so that's not a big effort.

However:

Please format #endif comments like this: #endif// def CONFIG_CHIP_STM32_GPIOV2_GPIOD_ENABLE (tab instead of space)

I could be open to changing that as well, but while this is simple for one-line elements, there are things like the chip vectors of clocks.hpp, where you have:

endif // defined(STM32F401_VECTORS) || defined(STM32F411_VECTORS) || defined(STM32F412_VECTORS) ||

  // defined(STM32F413_STM32F423_VECTORS) || defined(STM32F427_STM32F429_STM32F43_VECTORS) ||
  // defined(STM32F446_VECTORS) || defined(STM32F46_STM32F47_VECTORS)

I also fixed stylistic problems in the way as you suggest with simple #ifdef instead of defined(). It looks more consistent, but I would be nice to have one style without distinction to simple and complex conditions ;).

I have two more question about style and syntax:

• correct number of tabs on enum type - sometimes are two (ex. STM32-GPIOv2.hpp file), sometimes is one tab (ex. STM32F4-RCC.hpp)
• dividing lines and tabs in case of creating object: chip::ChipOutputPin{chip::Pin::pg13, false, chip::PinOutputSpeed::low, chip::PinPull::none, false, false}, or

  chip::ChipOutputPin{chip::Pin::pg13, false, chip::PinOutputSpeed::low, chip::PinPull::none, false, false},

  
  where alignment is created with tabs and spaces to align to the first bracket or it should be made just with tabs?

Let me know if now my fixes are correct, perhaps I missed something out... :)

[FreddieChopin]

I also fixed stylistic problems in the way as you suggest with simple #ifdef instead of defined(). It looks more consistent, but I would be nice to have one style without distinction to simple and complex conditions ;).

Thanks for your effort! I think we could use #if defined(...) everywhere but I propose to wait with that until all your branches are merged. It will be a massive change to almost every file in the source tree, but at the same time not worth potential conflicts it may cause, so let's delay that a bit (; Until it's changed, I'll have some more time to think about that. But don't hesitate to use the longer form in your commits.

correct number of tabs on enum type - sometimes are two (ex. STM32-GPIOv2.hpp file), sometimes is one tab (ex. STM32F4-RCC.hpp)

Hard question (; I usually let Eclipse decide for me and it seems Eclipse is not consistent in its decisions <: Currently it suggests one tab for enums, two tabs for arrays and two tabs for wrapped lines. But this doesn't matter that much - I have to keep my OCD under control (;

dividing lines and tabs in case of creating object: ... where alignment is created with tabs and spaces to align to the first bracket or it should be made just with tabs?

Just put two tabs if you wrap lines and it would be OK. You don't have to put every element of the initializer in a separate line, but you may do that if it serves some purpose (otherwise it just wastes vertical space). Generally code style is a hard subject. It shouldn't matter that much, but I'm usually tempted to tweak things up, wasting time for such unimportant tasks... Just try to mimic the rest of the code base. If you need to deviate because of some valid reason, I'll probably be fine with that.

In this particular case you presented above, I would just leave that in a single line - it fits within my personal choice of 120 columns, so there's no need to wrap it. If it would go beyond column number 120 - just wrap it, using two tabs. If in doubt - look for inspiration in other parts of project (I do that all the time <: ).

I don't align the wrapped lines with spaces to start at some particular column (below opening parentheses, first argument or whatever).