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

Added support for Unified Target configurations with manufacturer_id in the name. #1692

Merged
merged 1 commit into from Sep 28, 2019

Conversation

mikeller
Copy link
Member

@mikeller mikeller commented Sep 26, 2019

This adds support for loading / processing Unified Target configurations with the following naming schema: <manufacturer id>-<board name>.config
This is a prerequisite to allowing manufacturers to manage their own targets, as there would be the potential for naming clashes in the Unified Target configuration files otherwise.
Additionally, the manufacturer id is displayed in the target information page.

image

@mikeller mikeller added this to the 10.6.0 milestone Sep 26, 2019
targetName = targetParts[2];
if (targetParts[1]) {
$('div.release_info #manufacturerInfo').show();
$('div.release_info .manufacturer').text(targetParts[1]);
Copy link
Contributor

Choose a reason for hiding this comment

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

your span with the class of manufacturer is inside the #manufacturerInfo.

I think you should use id for both.

                <div id="manufacturerSection">
                    <strong i18n="firmwareFlasherReleaseManufacturer"></strong>
                    <span id="manufacturerInfo"></span>
                    <br />
                </div>

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point - was copy / pasta. I suspect the original author of a lot of this was allergic to ids.

optionText = target;
}

var select_e = $("<option value='{0}'>{1}</option>".format(item.targetName, optionText));
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get legacy targets listed when I run this PR
image
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Ha, I was wondering why we were adding the releases list twice - now this makes sense. Will check to see how we can make this a bit more intuitive.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, some more rewriting needed to make this work with the added complexity. Done now, please test again.

@mikeller
Copy link
Member Author

For extra credit: Found the bug that the available versions for Unified Target configurations were tied to their respective non-unified targets - this would have broken as soon as we were to pull the builds / availability for non-unified targets that have a confirmed working Unified Target configuration.

// Chicken and egg problem: We need to know what Unified Target this configuration uses before reading the configuration.
// Solving this by assuming that all Unified Targets have the same availability for now.
const DEFAULT_UNIFIED_TARGET_NAME = "STM32F405";
releases[targetName] = builds[DEFAULT_UNIFIED_TARGET_NAME];
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that needed? for a unified target I thought it was

  1. grab the file, parse the UNIFIED_TARGET_NAME
  2. Populate versions with those releases, eg releases[STM32F411]. Otherwise the unified only targets would have shown issues.

} else {
console.log('We have the config cached for', target);
var data = storageObj.data;
bareBoard = grabBuildNameFromConfig(data);
TABS.firmware_flasher.bareBoard = bareBoard;
setUnifiedConfig(target, data, bareBoard);
populateVersions(versions_e, TABS.firmware_flasher.releases[bareBoard], target);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Something that yields the available versions is needed.
The current way of using the non-unified .hex for this is not viable, as one goal of this whole exercise is to stop building / releasing them as soon as the Unified Target configurations are confirmed working for users, so the available versions for the non-unified targets will stop for some 4.1.x.
To get the actually used Unified Target .hex from the Unified Target config requires loading the file. If done when generating the list this will trigger GitHub's API request limiting (and be inefficient). If done when selecting a Unified Target configuration will introduce a delay and still trigger the request limit if a user is 'browsing'. So this is a pragmatic solution that will work for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Grabbing the individual config files isn't doing API calls. So I didn't worry about how many requests I do.

We're doing an API call to get the contents of configs/default (list of files) and another one for the releases. In the API call for releases, we get the last 30 releases, which is chock full of data.

If you did a release with only unified targets, users would get the proper versions, its just that the (Legacy) option wouldn't include a latest version.

Copy link
Member Author

Choose a reason for hiding this comment

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

We're doing an API call to get the contents of configs/default (list of files) and another one for the releases. In the API call for releases, we get the last 30 releases, which is chock full of data.

But we get them on a per target basis. And if we use a target that has been replaced by a Unified Target configuration and isn't released any more to populate the Unified Target configuration's releases, there won't be any releases showing for after we've pulled the non-unified target from release.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you download the releases, and remove the 4.1.0 RC builds for one target it looks like this:
image
This PR
image
Master
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, that is what I am going for - the aim is to phase out the non-unified targets in releases going forward (i.e. only Unified Targets and 'exotic' non-unified targets should be included in later 4.1.x releases).
For now, having access to the legacy targets provides a fallback for users if there are problems with Unified Targets, but in a future release we'll probably want to merge them into the 'normal' target selection, and give users non-unified targets for releases before 4.1.0-RC1, and Unified Targets after.
At this point we should also look into switching to using releases for the actual Unified Target .hex for every target - right now I am just not sure how best to do this. Speculatively loading all 140 (for now) Unified Target configurations seems to be a non-ideal way. Maybe we can generate a file containing a list of all Unified Target configurations with their required Unified Target .hex as part of the CI process in the unified-targets repository?

Copy link
Contributor

Choose a reason for hiding this comment

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

Generating a file sounds good, if we do it as part of the CI process, where does it go? Oh we have options for that. My vote is github pages https://docs.travis-ci.com/user/deployment/pages/

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking of a future CI process that would push any changes to master to something that is more suited as a content delivery network as well.
But even without this, or having CI modify the repository in any form, adding a script that re-generates the index file, requiring target maintainers to update the index file (by script or manually) as part of every pull request, and using CI to fail pull requests that fail to do so should work just fine as a first incarnation.

@mikeller mikeller merged commit 7cb8635 into betaflight:master Sep 28, 2019
@mikeller mikeller deleted the add_manufacturer_id_support branch September 28, 2019 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants