betaflight / betaflight

Open Source Flight Controller Firmware
GNU General Public License v3.0
8.42k stars 2.98k forks source link

Proposal: Move drivers from `src/main/drivers` into `src/drivers`. #8500

Open mikeller opened 5 years ago

mikeller commented 5 years ago

Idea for discussion, coming out of #8497: How about we move the drivers in src/main/drivers into src/drivers, and adjust the build pathing so that compilation units in src/main can include headers from src/drivers, but ones in src/drivers cannot include headers from src/main? This would move the enforcing of the separation of the driver layer into the build system. (We would also have to move src/main/pg to src/pg or similar, as the data structures in parameter groups are shared.)

etracer65 commented 5 years ago

I understand the goal of preventing drivers from depending on anything in the app layer, but I'm also not thrilled with the idea of moving part of the source out from under main. If it was just /drivers I guess it wouldn't be "too" bad, but having to move /pg out as well seems unpleasant.

Is there anyway to change the pathing so that anything in main/drivers can only access it's current directory and subdirectories? Like a chroot? I guess it would also need to be able to access main/pg.

Can we do symlinks?

Docteh commented 5 years ago

chroot and fakechroot sounds troublesome for most users. I'm hoping I'm the only one dumb enough to be working as root for everything :)

I think we should look at the paths given to GCC

Lets pick on adc.h and adc_impl.h

      4 "adc.h"
      4 "drivers/adc.h"
      2 "adc_impl.h"
      4 "drivers/adc_impl.h"

Looks like its being included in two different ways. If we make it so that asking for "drivers/adc.h" doesn't work, then any attempt to use something else from where we don't want will fail.

Although maybe that'll lead to a desire to do something silly looking like move pg includes into eg src/main/pg/pg/pg.h so that src/main/pg is given on the command line, and still the file can ask for #include "pg/beeper.h"

I can see how symlinks would help. not sure on how well they work on all platf... (okay I just mean windows)

There is always my favorite solution to everything: really long terminal commands

[/Web/betaflight.github] # grep -e '#include "blackbox' -e '#include "cli' -e '#include "cms' -e '#include "fc' -e '#include "flight' -e '#include "io/' -e '#include "msp' -e '#include "osd' -e '#include "scheduler' -e '#include "sensors' -e '#include "rx/' -e '#include "target/' -e '#include "telemetry/' src/main/drivers/* -r
src/main/drivers/barometer/barometer_qmp6988.c:#include "io/serial.h"
src/main/drivers/camera_control.c:#include "osd/osd.h" <--- worth a look
src/main/drivers/compass/compass_ak8963.c:#include "scheduler/scheduler.h" <-- might be worth a look
src/main/drivers/flash_w25n01g.c:#include "io/serial.h"
src/main/drivers/inverter.c:#include "io/serial.h" // For SERIAL_PORT_IDENTIFIER_TO_INDEX
src/main/drivers/rangefinder/rangefinder_hcsr04.h:#include "sensors/battery.h"
src/main/drivers/rangefinder/rangefinder_lidartf.c:#include "io/serial.h"
src/main/drivers/rx/rx_cc2500.h:#include "rx/rx_spi.h"
src/main/drivers/rx/rx_nrf24l01.h:#include "rx/rx_spi.h"
src/main/drivers/serial_escserial.c:#include "flight/mixer.h" <--- heyo
src/main/drivers/serial_escserial.c:#include "io/serial.h"
src/main/drivers/serial_pinconfig.c:#include "io/serial.h"
src/main/drivers/serial_softserial.c:#include "fc/config.h" //!!TODO remove this dependency
src/main/drivers/serial_tcp.c:#include "io/serial.h"
src/main/drivers/usb_msc_f4xx.c:#include "blackbox/blackbox.h" <-- I guess this is what we're looking for
src/main/drivers/usb_msc_f7xx.c:#include "blackbox/blackbox.h"
[/Web/betaflight.github] #
mikeller commented 5 years ago

@etracer65: Agreed. The fact that pg/ needs to be 'middle ground' is unpleasant - we could probably get around this by leaving pg/ where it is, and adding an explicit search path of src/main/pg/ to the drivers build.

hydra commented 5 years ago

likely things in /build/ and /common/ need to be used by both /pg and /drivers.

As the person that created the src/main folder the reason it was done was to seperate the test code from the main code, a pattern which is common in JVM languages.

I don't think it's a good idea to mix something that's not MAIN and not TEST along side the two respective directories at all. an alternate solution would be to begin the modularisation process which is long overdue, thus:

modules\math\src\main modules\math\src\test

modules\rx-sbus\src\main modules\rx-sbus\src\test

modules\rx-sbus\src\main modules\rx-sbus\src\test

drivers\gyro-x\src\main drivers\gyro-x\fc\src\test

drivers\motor-dshot\src\main drivers\motor-dshot\src\test

core\fc\src\main core\fc\src\test

I dunno, just throwing it out there.

mikeller commented 5 years ago

Agreed that having drivers/ at the same level as test/ and 'utils/isn't right at a logical level - if we move on this we should probably go forsrc/main/driversandsrc/main/firmware, to have the build units ('libraries') for the firmware all belowsrc/main. Not sure about splitting things that are the same build unit up, and mixing in the tests - I always find that structures of this kind have a negative effect when searching code withgrep` or 'Search in Files', as the developer is then forced to sort through test / non test results. Also, if this is done, it should be done in a way that dynamically generates include paths in the Makefile, as having to maintain a complex include path for the firmware to avoid building tests is a step backwards.

hydra commented 5 years ago

drivers and firmware seems OK, but I'm not sure how it would immediately improve the separation between drivers and firmware code. Is the idea to adjust the include paths for the compiler so that drivers cannot include anything in /firmware ?

mikeller commented 5 years ago

@hydra: Yes, exactly, with the only exception being headers in pg/.

stale[bot] commented 5 years ago

This issue / pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs within a week.

tavdog commented 2 years ago

Close bump?