cleanflight / cleanflight

Clean-code version of the baseflight flight controller firmware
http://cleanflight.com
GNU General Public License v3.0
2.59k stars 1.39k forks source link

Preprocessor defaulting for IO #2780

Closed ledvinap closed 7 years ago

ledvinap commented 7 years ago

With quite simple preprocessor magic, it may be possible to implement macro 'IO_TAG_DEFAULT(define, default)' that returns default when define is not defined (and behaves as IO_TAG otherwise). Only limitation is that each supported define must be registered somewhere (ISIO_x must be #defined)

for example:

#ifdef INVERTER_PIN_UART2
             serialPinConfig->ioTagInverter[SERIAL_PORT_IDENTIFIER_TO_RESOURCE_INDEX(SERIAL_PORT_USART2)] = IO_TAG(INVERTER_PIN_UART2);
#endif

can be replaced with:

// possibly in some header:
# define ISIO_INVERTER_PIN_UART2

serialPinConfig->ioTagInverter[SERIAL_PORT_IDENTIFIER_TO_RESOURCE_INDEX(SERIAL_PORT_USART2)] = IO_TAG_DEFAULT(INVERTER_PIN_UART2, NONE);

Another possibility is `IO_TAG_DEFAULT(define)' and define macros with default value:

#define DEFAULT_INVERTER_PIN_UART1 PA2
#define DEFAULT_INVERTER_PIN_UART2 NONE

... = IO_TAG_DEFAULT(INVERTER_PIN_UART1);  // expands to tag for PA2 when INVERTER_PIN_UART1 is not defined
... = IO_TAG_DEFAULT(INVERTER_PIN_UART2);  // expands to 0 when INVERTER_PIN_UART2 is not defined

Any opinions? Is it worth the effort?

For example https://github.com/cleanflight/cleanflight/blob/master/src/main/drivers/serial_uart_stm32f30x.c#L42 can loose quite some lines ...

martinbudden commented 7 years ago

In my opinion, too much preprocessor magic can obscure the code and make it harder to read/maintain. With the current code it is easy to see what is going on, this change would obscure that.

ledvinap commented 7 years ago

@martinbudden : Yes, that is why I'm asking. Preprocessor metaprogramming is fun, but it's difficult for me to find right balance between magic hidden inside macros and ease of use.

martinbudden commented 7 years ago

I understand. It's easy to get carried away...

hydra commented 7 years ago

let's leave this for now, I'd rather we focussed on reviewing and resolving existing PR's first. We can come back to it in the future. @ledvinap if you have any code snippets can you attach them to this issue?

ledvinap commented 7 years ago

@hydra: Sorry, no code yet ... basically it would use detection mechanism from https://github.com/pfultz2/Cloak/wiki/C-Preprocessor-tricks,-tips,-and-idioms, possibly after adding test test values into id_def_generated.

Then just test if INVERTER_PIN_UART1 expands into IO tag (PA1), if not, then try DEFAULT_INVERTER_PIN_UART1


#define IS_IO(tag) <something> // return 1 if tag expands to io tag, 0 othrewise
#define IO_TAG_DEFAULT_GET(tag) PP_IIF(IS_IO(tag), tag, PP_IIF(IS_IO(CAT(DEFAULT_, tag), CAT(DEFAULT_, tag), tag))