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

Add support for easy compilation of define-only custom targets. #12335

Closed
wants to merge 1 commit into from
Closed

Add support for easy compilation of define-only custom targets. #12335

wants to merge 1 commit into from

Conversation

hydra
Copy link
Contributor

@hydra hydra commented Feb 8, 2023

e.g.

make TARGET=STM32H730 CUSTOM_TARGET=SPRO-SPRACINGH7EXTREME

Thus, no need to create temporary build scripts anymore.

To test this, ensure you have BF and BF unified targets repo as siblings in your filesystem.

$ git clone <betaflight-repo-url> betaflight
$ git clone <betaflight-unified-targets-repo-url> betaflight-unified-targets
$ tree -L 1
.
├── betaflight
└── betaflight-unified-targets

then setup BF for development as usual, usually just:

cd betaflight
make arm_sdk_install

then finally just run:

make TARGET=STM32H730 CUSTOM_TARGET=SPRO-SPRACINGH7EXTREME

this will result in a log similar to this:

$ make TARGET=STM32H750 CUSTOM_TARGET=SPRO-SPRACINGH7EXTREME
make -j ./obj/betaflight_4.5.0_STM32H750.hex
make[1]: Entering directory '/cygdrive/d/Users/Hydra/Documents/dev/projects/betaflight/betaflight'
Extracting #defines from custom target file ./../betaflight-unified-targets/configs/default/SPRO-SPRACINGH7EXTREME.config to ./obj/main/STM32H750/custom_target.h
rm -f ./obj/main/STM32H750/.efhash_*
EF HASH -> ./obj/main/STM32H750/.efhash_d41d8cd98f00b204e9800998ecf8427e
%% startup_stm32h743xx.s
%% (size optimised) ./src/main/drivers/bus_i2c_timing.c
...
%% (size optimised) lib/main/google/olc/olc.c
Linking STM32H750
Memory region         Used Size  Region Size  %age Used
        ITCM_RAM:        9848 B        64 KB     15.03%
        DTCM_RAM:       82156 B       128 KB     62.68%
             RAM:       27924 B        64 KB     42.61%
        CODE_RAM:      384653 B     450496 B     85.38%
 CUSTOM_DEFAULTS:           8 B         8 KB      0.10%
       EXST_HASH:          64 B         64 B    100.00%
          D2_RAM:          0 GB       256 KB      0.00%
       MEMORY_B1:          0 GB         0 GB
         QUADSPI:          0 GB         0 GB
   text    data     bss     dec     hex filename
 376769    7956  102196  486921   76e09 ./obj/main/betaflight_STM32H750.elf
Creating BIN (without checksum) ./obj/main/betaflight_STM32H750_UNPATCHED.bin
Creating EXST ./obj/betaflight_4.5.0_STM32H750.bin
448+0 records in
896+0 records out
458752 bytes (459 kB, 448 KiB) copied, 0.0041625 s, 110 MB/s
896+0 records in
896+0 records out
458752 bytes (459 kB, 448 KiB) copied, 0.0048189 s, 95.2 MB/s
Generating MD5 hash of binary
Patching MD5 hash into binary
0006fff0: 39fc fc26 b8b6 2e49 1e48 c229 1860 9963  9..&...I.H.).`.c
Extracting HASH section from unpatched EXST elf ./obj/main/betaflight_STM32H750.elf
./tools/gcc-arm-none-eabi-10.3-2021.10/bin/arm-none-eabi-objcopy ./obj/main/betaflight_STM32H750.elf ./obj/main/betaflight_STM32H750_EXST.elf.tmp --dump-section .exst_hash=./obj/main/STM32H750/exst_hash_section.bin -j .exst_hash
D:\Users\Hydra\Documents\dev\projects\betaflight\betaflight\tools\gcc-arm-none-eabi-10.3-2021.10\bin\arm-none-eabi-objcopy.exe: ./obj/main/betaflight_STM32H750.elf: warning: empty loadable segment detected at vaddr=0x4beeb320010000, is this intentional?
D:\Users\Hydra\Documents\dev\projects\betaflight\betaflight\tools\gcc-arm-none-eabi-10.3-2021.10\bin\arm-none-eabi-objcopy.exe: ./obj/mainrm ./obj/main/betaflight_STM32H750_EXST.elf.tmp
Patching MD5 hash into HASH section
Patching updated HASH section into ./obj/main/betaflight_STM32H750_EXST.elf
./tools/gcc-arm-none-eabi-10.3-2021.10/bin/arm-none-eabi-objcopy ./obj/main/betaflight_STM32H750.elf ./obj/main/betaflight_STM32H750_EXST.elf --update-section .exst_hash=./obj/main/STM32H750/exst_hash_section.bin
There are 27 section headers, starting at offset 0xd7bc4:

Section Headers:
  [Nr] Name              Type            Addr     Off    Size   ES Flg Lk Inf Al
  [ 0]                   NULL            00000000 000000 000000 00      0   0  0
  [ 1] .isr_vector       PROGBITS        24010000 010000 000298 00   A  0   0  1
  [ 2] .text             PROGBITS        24010298 010298 058ae8 00  AX  0   0  8
  [ 3] .tcm_code         PROGBITS        00000000 070000 002678 00  AX  0   0  8
  [ 4] .ARM              ARM_EXIDX       2406b3f8 07b3f8 000008 00  AL  2   0  4
  [ 5] .pg_registry      PROGBITS        2406b400 07b400 0009bc 00   A  0   0  4
  [ 6] .pg_resetdata     PROGBITS        2406bdbc 07bdbc 000205 00   A  0   0  4
  [ 7] .custom_defaults  PROGBITS        2407dfc0 09dfc0 000008 00  WA  0   0  4
  [ 8] .data             PROGBITS        20000000 080000 001b08 00  WA  0   0  4
  [ 9] .bss              NOBITS          20001b08 081b08 00ca9c 00  WA  0   0  8
  [10] .sram2            PROGBITS        24000000 0a0000 000000 00   W  0   0  1
  [11] .fastram_data     PROGBITS        2000e5a4 08e5a4 000004 00  WA  0   0  4
  [12] .fastram_bss      NOBITS          2000e5a8 08e5a8 005344 00  WA  0   0  4
  [13] .dmaram_data      PROGBITS        24000000 090000 0003c0 00  WA  0   0 32
  [14] .dmaram_bss       NOBITS          240003c0 0903c0 001680 00  WA  0   0 32
  [15] .DMA_RAM          NOBITS          24001a40 091a40 005060 00  WA  0   0 32
  [16] .DMA_RW_D2        PROGBITS        30000000 0a0000 000000 00   W  0   0  1
  [17] .DMA_RW_AXI       NOBITS          24006aa0 006aa0 000274 00  WA  0   0 32
  [18] .persistent_data  PROGBITS        24006d14 0a0000 000000 00   W  0   0  1
  [19] ._user_heap_stack NOBITS          200138ec 0038ec 000800 00  WA  0   0  1
  [20] .ARM.attributes   ARM_ATTRIBUTES  00000000 0a0000 000035 00      0   0  1
  [21] .exst_hash        PROGBITS        2407ffc0 09ffc0 000040 00  WA  0   0  1
  [22] .comment          PROGBITS        00000000 0a0035 000049 01  MS  0   0  1
  [23] .debug_frame      PROGBITS        00000000 0a0080 00075c 00      0   0  4
  [24] .symtab           SYMTAB          00000000 0a07dc 01f410 10     25 5542  4
  [25] .strtab           STRTAB          00000000 0bfbec 017eb4 00      0   0  1
  [26] .shstrtab         STRTAB          00000000 0d7aa0 000122 00      0   0  1
Key to Flags:
  W (write), A (alloc), X (execute), M (merge), S (strings), I (info),
  L (link order), O (extra OS processing required), G (group), T (TLS),
  C (compressed), x (unknown), o (OS specific), E (exclude),
  y (purecode), p (processor specific)

Elf file type is EXEC (Executable file)
Entry point 0x24057081
There are 11 program headers, starting at offset 52

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  LOAD           0x000000 0x20010000 0x20010000 0x00194 0x040ec RW  0x10000
  LOAD           0x006aa0 0x24006aa0 0x24006aa0 0x00000 0x00274 RW  0x10000
  LOAD           0x010000 0x24010000 0x24010000 0x58d80 0x58d80 R E 0x10000
  LOAD           0x070000 0x00000000 0x24068d80 0x02678 0x02678 R E 0x10000
  LOAD           0x07b3f8 0x2406b3f8 0x2406b3f8 0x00bc9 0x00bc9 R   0x10000
  LOAD           0x080000 0x20000000 0x2406bfc1 0x01b08 0x0e5a4 RW  0x10000
  LOAD           0x08e5a4 0x2000e5a4 0x2406dac9 0x00004 0x05348 RW  0x10000
  LOAD           0x090000 0x24000000 0x2406dacd 0x003c0 0x01a40 RW  0x10000
  LOAD           0x001a40 0x24001a40 0x2406de8d 0x00000 0x05060 RW  0x10000
  LOAD           0x09dfc0 0x2407dfc0 0x2407dfc0 0x00008 0x00008 RW  0x10000
  LOAD           0x09ffc0 0x2407ffc0 0x2407ffc0 0x00040 0x00040 RW  0x10000

 Section to Segment mapping:
  Segment Sections...
   00     ._user_heap_stack
   01     .DMA_RW_AXI
   02     .isr_vector .text
   03     .tcm_code
   04     .ARM .pg_registry .pg_resetdata
   05     .data .bss
   06     .fastram_data .fastram_bss
   07     .dmaram_data .dmaram_bss
   08     .DMA_RAM
   09     .custom_defaults
   10     .exst_hash
Creating EXST HEX from patched EXST BIN ./obj/betaflight_4.5.0_STM32H750.bin, VMA Adjust 0x97CE0000
make[1]: Leaving directory '/cygdrive/d/Users/Hydra/Documents/dev/projects/betaflight/betaflight'

e.g.

```
make TARGET=STM32H730 CUSTOM_TARGET=SPRO-SPRACINGH7EXTREME
```

Thus, no need to create temporary build scripts anymore.
@github-actions
Copy link

github-actions bot commented Feb 8, 2023

Do you want to test this code? Here you have an automated build:
Assets
WARNING: It may be unstable. Use only for testing! See: https://www.youtube.com/watch?v=I1uN9CN30gw for instructions for unified targets!

@hydra
Copy link
Contributor Author

hydra commented Feb 8, 2023

Maintainers: this PR comes about because of the plan to go 'define-only' for the unified targets, see betaflight/betaflight-configurator#3316 (comment)
I will raise a corresponding PR for 4.4-maintenance once this is reviewed/merged.

@hydra hydra mentioned this pull request Feb 8, 2023
@blckmn
Copy link
Member

blckmn commented Feb 8, 2023

AUTOMERGE: (FAIL)

  • github identifies PR as mergeable -> FAIL
  • assigned to a milestone -> PASS
  • cooling off period lapsed -> PASS
  • commit count less or equal to three -> PASS
  • Don't merge label NOT found -> FAIL
  • at least one RN: label found -> FAIL
  • Tested label found -> FAIL
  • assigned to an approver -> FAIL
  • approver count at least three -> FAIL

@blckmn
Copy link
Member

blckmn commented Feb 8, 2023

Aside from the use of unified targets how is this different from the board.h scratch pad for developers?

Wouldn't this be better to say make the scratch pad from the unified config.

Eg make board_details CONFIG=X

You only need board.h, and board.mk (which only needs one line. TARGET:=XXX).

Why reinvent something that already exists? Why not simply improve on it?

Also, please use wget or curl and download the content, don't force a dependency on child repos.

We could also move the logic for config file parsing to the build api then and remove the repo dependency.

@hydra
Copy link
Contributor Author

hydra commented Feb 8, 2023

Aside from the use of unified targets how is this different from the board.h scratch pad for developers?

board.h and board.c, and let's remind ourselves that we don't want any target specific code in the code-base, are for the purposes of developing new targets where devs can temporarily hack things in and the files are SPECIFICALLY excluded from commits by .gitignore.

Wouldn't this be better to say make the scratch pad from the unified config.

No, as board.c/.h exist for a different purpose.

Why reinvent something that already exists? Why not simply improve on it?

See above, I did consider changing it, but after evaluating things it didn't make any sense.

Also, please use wget or curl and download the content, don't force a dependency on child repos.

There dependency on the child repo is not forced for normal builds without CUSTOM_TARGET. Probably the cloud build API already includes the #defines from the .config by some other means that I'm not privy to as my request for the source to the API was denied as it's closed source, I cannot be any help there.

However, there is no reason why curl couldn't be used as a secondary source for the config, using it as a primary source however is BAD for a number of reasons:

  1. you need curl.
  2. you need internet access.
  3. your development environment might change without your realizing it if someone uploads a new file to the repo.
  4. you can't test your unified target config until it's published and merged.

Using a local repo allows fully offline compilation and allows devs to work on their target configuration before publishing it, typical workflow is:
a) make firmware changes.
b) make target config changes.
c) test.
d) commit firmware changes.
e) push firmware changes and make a PR to the firmware repo.
f) commit target config changes.
g) push target changes and make a PR to the targets repo.

by integrating curl you would break the above workflow.

Furthermore, it also lets you use a SPECIFIC version of a target config and/or SPECIFIC version of a target config, which is extremely helpful when trying to git-bisect, another very important workflow that should not be broken. And is extremely helpful when developing new targets on branches of the two repos.

We could also move the logic for config file parsing to the build api then and remove the repo dependency.

No, see above. Config parsing should be available offline from the firmware repo.

The solution in this PR works offline, addresses all dev workflows that I'm aware of and doesn't introduce any new requirements for non-custom targets, it's only when you build a custom target that you need the other repo.

I'm not against adding curl to allow CI/cloud build systems that want to build targets, but a CI/cloud build system could make the file for the target available locally and still use the CUSTOM_TARGET option in here and could also specify the path to a fake repo that just contains the 1 file for the target being built. Using curl is beyond the scope of this PR and can be added later, as-required. this works, now. time is short.

@blckmn
Copy link
Member

blckmn commented Feb 8, 2023

What specifically is your use case here? To make it easy for developers to build locally - with the odd few flyers (who don't consider themselves developers) to be able to build a local hex.

This is EXACTLY what board.h is for. It is explicitly excluded from git for that very reason also.

You are just introducing yet another layer of complexity for a problem that does not exist on the auspices of keeping things separate under some guise of "clean".

It is of my opinion that this approach is entirely unnecessary.

In addition I am also not a fan of adding in curl, but it is the lesser of two evils in regards to linking in a repo that is about to be archived!

My view is if you are going to all that trouble, you can simply download the defines, and create your own board.h.

For EVERYONE else there is an entire make command to run at the start of every CLOUD BUILD log that allows someone to repeat it locally!

@hydra
Copy link
Contributor Author

hydra commented Feb 9, 2023

Reducing the barrier-to-entry for contributions and to make developers life EASY is another goal, in addition to the offline, bisect suitable, always-consistant, and workflow requirements above. All of which are my use-cases, are all very important, and all have been known goals for the codebase for 10 years or so now.

At the moment if someone downloads the codebase and wants to build the /exact/ same code that is already on their FC they're going to have a much harder time doing it than with the approach that this PR brings.

This PR brings a return to the /simple/ way of building a target that previously existing for all the legacy targets and that still exists for the base targets.

With the approach outlined above the knowledge of how to build a target is hidden in the code that generates the commands to build the target for their board.

@hydra
Copy link
Contributor Author

hydra commented Feb 9, 2023

There's another use-case I forgot to mention, and that is IDE support.

In Eclipse, CLion/Idea and most other IDEs you have launch configurations and make targets so you can store frequently used launch configurations. For example:

image

image

Easy!

@hydra
Copy link
Contributor Author

hydra commented Feb 9, 2023

This is EXACTLY what board.h is for. It is explicitly excluded from git for that very reason also.

this requires much faffing with moving files around locally when you're trying to test code on multiple targets, as is often the case during normal development. you have to locate some code, copy/paste code into a file, build, test, locate some different code, copy paste into board.c/h build, test, etc.

you can't bisect easily, etc, it's just a terrible workflow.

You are just introducing yet another layer of complexity for a problem that does not exist on the auspices of keeping things separate under some guise of "clean".

that is not the case, there is no 'guise of clean' here, it's about developer workflow, simplicity, understandable and also being time-efficient.

the board.c/.h is /more/ complex as it requires more effort to use.

I don't see how you can argue about this not-complex.

The current world of building two different targets, one after the other.

$ git clone <betaflight-repo-url> betaflight
$ git clone <betaflight-unified-targets-repo-url> betaflight-unified-targets
$ cd betaflight
$ make arm_sdk_install
* use the configurator to build a target A, at a specific git revision using a specific revision of the target (probably not even possible)
* get a log
* find the command used to build the target
* copy it to clipboard
* paste into command prompt
* run configurator, load the exact same target config using the flash firmware button
* use the configurator to build a target B, at a specific git revision using a specific revision of the target (probably not even possible)
* get a log
* find the command used to build the target
* copy it to clipboard
* paste into command prompt

or

$ git clone <betaflight-repo-url> betaflight
$ git clone <betaflight-unified-targets-repo-url> betaflight-unified-targets
$ cd betaflight
$ make arm_sdk_install
$ grep -P '^#define` ../betaflight-unified-targets/configs/defaults/SPRO-SPRACINGH7EXTREME > src/board/board.h
$ make TARGET=STM32H750
$ rm src/board/board.h # <-- you have to remember to do this too!
$ grep -P '^#define` ../betaflight-unified-targets/configs/defaults/SPRO-SPRACINGH7RF > src/board/board.h
$ make TARGET=STM32H730
$ rm src/board/board.h # <-- you have to remember to do this too!

vs this (this PR)

git clone <betaflight-repo-url> betaflight
git clone <betaflight-unified-targets-repo-url> betaflight-unified-targets
cd betaflight
make arm_sdk_install
make TARGET=STM32H750 CUSTOM_TARGET=SPRO-SPRACINGH7EXTREME
make TARGET=STM32H730 CUSTOM_TARGET=SPRO-SPRACINGH7RF

Simple, easy, repeatable, bisect-capable, efficient, understandable, not-error-prone, always-consistent, maintainable, workflow friendly, ide-friendly.

@howels
Copy link
Contributor

howels commented Feb 9, 2023

Can we merge this as a stop-gap change until the new targets repo is ready? Seems like the arguments are that we can resist local build automation cos a manual approach exists, which doesn't track.
If we want to make a bunch of local builds to test code before making a PR, and test that PR on a few different quads, then this script will help as well as with SPracing board targets that aren't yet available online.

@hydra
Copy link
Contributor Author

hydra commented Feb 9, 2023

From discussions in discord:

another reason against using the existing board.h file is that src/main/board is a consisent location, and not a build-artifact location. This PR extracts the #defines into a file that is managed by make in the obj directory which is where all the build artifacts belong so that they can all be removed by deleting the entire obj directory if needed, this has always been the case with the codebase.

@blckmn
Copy link
Member

blckmn commented Feb 10, 2023

Following the discussion, the PR #12341 makes this approach redundant. Whilst the config.h files will be periodically updated from the unified targets repo, this is only transitory - as we adjust the cloud build processes and documentation.

Closing the PR.

@blckmn blckmn closed this Feb 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: COMPLETED
Development

Successfully merging this pull request may close these issues.

None yet

4 participants