search

betaflight / betaflight-esc

Open source ESC firmware.
GNU General Public License v3.0
103 stars 40 forks source link

Move stm32f0xx_ll_* leftovers to include.h #16

Closed brycedjohnson closed 6 years ago

brycedjohnson commented 6 years ago

a little cleanup

mikeller commented 6 years ago

Ok, not sure I like the use of include.h as a catch-all. This seems to me to be pretty much an antipattern, and a slippery slope to drift into poor layering of code.

What do others think about this?

brycedjohnson commented 6 years ago

I did that because it looked like it was already headed that way for the LL drivers. I was guessing it was to separate STM32F0 from other future ones, but I'm sure that could all be done on the C files themselves with some #ifdefs. Doesn't matter to me, just don't really like the mix of the two.

As for the the non-LL stuff. like 

#include "drivers/drv_system.h"
#include "drivers/drv_uart.h"
#include "drivers/drv_led.h"
#include "drivers/drv_watchdog.h"
#include "drivers/drv_ws2812.h"

I would rather see those included correct C files when needed.

mikeller commented 6 years ago

Sorry, this wasn't meant as criticism on your pull request. It's just the first time that I noticed include.h, and I wanted to bring this up for discussion. I agree with improving consistency, but at the same time it's important to be careful about the use of antipatterns.

brycedjohnson commented 6 years ago

No offense taken. I didn't even really see that all/most h files were in there until you mentioned it... Yeah I'm not a big fan of a catch all include either.

blckmn commented 6 years ago

@mikeller I agree, we need to move them to the relevant C files where they are used.

Linjieqiang commented 6 years ago

Okay.That's my fault.Maybe need to move them to the relevant C files.