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

Simplified Local Build using Permanent Config #12341

Merged
merged 5 commits into from Feb 11, 2023
Merged

Conversation

blckmn
Copy link
Member

@blckmn blckmn commented Feb 9, 2023

This is the first stage in the decommissioning process for unified targets and custom defaults.

  1. It has purposely been created outside of src/main and targets.
  2. Targets remain as MCU only.
  3. Longer term Cloud Build will move to using the config, instead of passing parameters.
  4. CI build will still default to building the MCU only, and in some cases these will not be bootable.
  5. At the moment they are auto generated, but this is merely transitional.
  6. Config defines should be the bare minimum of defaults, to both boot and operate.
  7. Manufacturer presets can provide even greater configurability.

Usage:

make CONFIG=BETAFLIGHTF4

@blckmn blckmn self-assigned this Feb 9, 2023
@blckmn blckmn added this to the 4.5 milestone Feb 9, 2023
@github-actions

This comment has been minimized.

Copy link
Member

@KarateBrot KarateBrot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add a new line at the end of every file?

You don't need to do it manually, though. @ASDosjani made a little bash script in the unified-targets repo to do this.
https://github.com/betaflight/unified-targets/blob/master/fix-file-endings.sh.

But you need to change it a bit to fit this case.

@blckmn
Copy link
Member Author

blckmn commented Feb 10, 2023

@KarateBrot this is now done :)

@github-actions

This comment has been minimized.

@github-actions
Copy link

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!

@blckmn
Copy link
Member Author

blckmn commented Feb 10, 2023

AUTOMERGE: (FAIL)

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

@hydra
Copy link
Contributor

hydra commented Feb 10, 2023

I'm happy to see you took on-board all of my suggestions and requirements from #12335 and also went the extra step of moving all the target definitions back into the codebase. 🥳

Some questions:

This is the first stage in the decommissioning process for unified targets and custom defaults.

can you outline the remaining stages, as it's not clear what's going to happen with the exsting cli commands that are in the target config in the unified targets repo.

3. Longer term Cloud Build will move to using the config, instead of passing parameters.

long-term, in what form with the config be? .h file only?

4. CI build will still default to building the MCU only, and in some cases these will not be bootable.

Please give an example where any of these targets will not be bootable.

Also:

What is the plan for 4.4.1, are you going to back-port this PR to 4.4.1 so that all the EXST targets can be built again?

Additionally:

The EXST targets need settings that are currently hard-coded in the MCU config, such as VMA address for the flash chip.

I see that this PR extracts a setting from a #define and turns it into a make file parameter. As follows:

TARGET       := $(shell grep " FC_TARGET_MCU" $(CONFIG_FILE) | awk '{print $$3}' )

The same approach can be taken for the VMA address.

e.g. in a target

#define EXST_ADJUST_VMA 0x97CE0000

then:

EXST_ADJUST_VMA       := $(shell grep " EXST_ADJUST_VMA" $(CONFIG_FILE) | awk '{print $$3}' )

The same approach can also be used to pick the right linker script based on the EXST and TARGET_FLASH_SIZE settings which could also be extracted from the #defines in config file.

@hydra
Copy link
Contributor

hydra commented Feb 10, 2023

One more question is there a PR somewhere to update the documentation on the targets repo? I ask as currently target maintainers have to publish changes both to master and the targets repo to keep everything in sync.

@blckmn
Copy link
Member Author

blckmn commented Feb 10, 2023

Note this approach is no different and fundamentally the same as board.h that already existed but I've simply generated all of the header files. There's only two differences, the first is no need for mk file to set the core target, and the second is merely the file locations.

The plan would be to restrict to one header file only to restrict a proliferation of complexity.

Regarding your EXST targets, they aren't a priority for the moment. The volume of downloads don't warrant it taking priority over other items, this includes spending time reviewing PRs. Once we've migrated and finished the majority I'm happy to revisit.

In the meantime I'm happy for you to build a hex and provide to me, and I'll update the cloud build api with a static hex for each version so the target can be downloaded similarly to how it works for 4.3.2 releases.

@blckmn
Copy link
Member Author

blckmn commented Feb 10, 2023

One more question is there a PR somewhere to update the documentation on the targets repo? I ask as currently target maintainers have to publish changes both to master and the targets repo to keep everything in sync.

Until the targets repo is decommissioned maintainers will only need to update the config files in that repo. We will periodically auto generate, similar to translation PRs.

I'll be updating the documentation around the changes dropping board.h shortly.

@hydra
Copy link
Contributor

hydra commented Feb 10, 2023

Regarding your EXST targets, they aren't a priority for the moment. The volume of downloads don't warrant it taking priority over other items, this includes spending time reviewing PRs. Once we've migrated and finished the majority I'm happy to revisit.

Any plan will be incomplete unless the EXST targets are included in the plan.

The VMA address and linker script settings are in the BASE H750/H730 targets .mk files will suffice for now, so no rush there however and the approach, given this PR is clear.

In the meantime I'm happy for you to build a hex and provide to me, and I'll update the cloud build api with a static hex for each version so the target can be downloaded similarly to how it works for 4.3.2 releases.

I can, but it doesn't feel like this is a good idea as people won't be able to re-create the .hex/.bin themselves from the source like they should be able to.

Please can you answer my question about if this is going to be backported to the 4.4-maintenance branch? I can assist if required.

@hydra
Copy link
Contributor

hydra commented Feb 10, 2023

Also, please can you answer my other question:

This is the first stage in the decommissioning process for unified targets and custom defaults.

can you outline the remaining stages, as it's not clear what's going to happen with the exsting cli commands that are in the target config in the unified targets repo.

@blckmn
Copy link
Member Author

blckmn commented Feb 10, 2023

I may consider back porting it. I won't be needing your assistance but thanks for the offer.

@blckmn
Copy link
Member Author

blckmn commented Feb 10, 2023

can you outline the remaining stages, as it's not clear what's going to happen with the exsting cli commands that are in the target config in the unified targets repo.

Working defines for each of them as required.

I figured it was a fairly clear answer in the statement 'there will be one header file'.

The next stages will simply be working through various approaches to achieve the single header file.

@hydra
Copy link
Contributor

hydra commented Feb 10, 2023

I figured it was a fairly clear answer in the statement 'there will be one header file'.

it wasn't clear, as I had previously heard mention of using .json files for target configs, but that would entail json parsing code in the makefiles, etc. can you confirm going forward that the plan is to keep the per-target header file (only) in this PR going forward?

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

5 participants