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

initial go at flashing unified target with a config #1587

Merged
merged 4 commits into from
Sep 5, 2019

Conversation

Docteh
Copy link
Contributor

@Docteh Docteh commented Aug 19, 2019

This adds a method to flash firmware and preload a unified target configuration directly from internet resources.

This builds on #1585 Applying the configuration automatically will be handled later.
image
In the above screencut MATEKF411 will use flash with the STM32F411 target and have the MATEKF411.config attached. selection of MATEKF411 (Legacy) will use the MATEKF411 target.

To test this PR look at this list of configuration files, and pick one of them. After flashing take a look at defaults show or just punch in defaults and see if the board comes back with a working ACC

show_development_releases selected_build_type are left in config.storage.local as there is some execution order issue that results in the builds list being downloaded twice on the tab load. Any change to those would need to be tested on both ChromeAPP and NWJS builds.

Doubling up on lines like this is a giveaway:
Using cached release information for firmware releases.
Using cached release information for firmware releases.

Todo

  • Version and target choice need to show loading when loading.
  • UI Changes. Need to show an indicator if a user loads a .config locally, and have a way to throw it out.
  • investigate why data is being saved to the options in the version <select> field with jQuery. I might have to eat my words but it seems like a dumb idea. Get: .data('summary') Set: .data('summary', object)
  • move the URL for unified configs to a better location.
  • localization pass (read through code and add strings where appropriate)
  • Amount of versions to show
  • Amount of (Legacy) to show

@mikeller
Copy link
Member

Nice job @Docteh! And even though we doubled up I think we both solved different parts of the problem, so we can probably combine our solutions.

My thoughts:

  • while I appreciate the approach to use the nrf-intel-hex package (I am surprised this comes as web ready), considering that we already have our own implementation of a parser / memory structure, which is tied into the programmer, I think we should go with extending this for now, and not transition in / out of the in-memory representation used by nrf-intel-hex. In the longer term, it would be nice to replace the proprietary implementation entirely with nrf-intel-hex though;
  • in my opinion, Unified Targets should work transparently to the user - we have done our job right if the user does not notice it when they flash a Unified Target for the first time. As part of this, my idea would be to, instead of having a 'Unified' channel, take the list of supported Unified Targets, and merge it into the list of supported targets, potentially changing the non-unified target of the same name to ' (legacy)' or similar. When being flashed, the target will then grab the required Unified Target .hex from the same firmware channel;
  • at a later stage, this could be pivoted, and the available targets shown in the top level drop down, and then later on the channels / versions available for the selected target;
  • n.b. that I am planning to move the Unified Target configurations into a separate repository, with the idea to set up an automated approval process to allow manufacturers to self-manage their Unified Target configurations.

@Docteh
Copy link
Contributor Author

Docteh commented Aug 20, 2019

  • For the config files, I'll move the URL higher in the file. The current 66 files add up to 173,519 bytes. The API call to get a list of stuff in the unified_targets/configs directory is currently 71,431 bytes. The API returns 1000 files at a time. I'm hoping we can have only one version of unified target configurations, but if not we can deal.

  • I'll stick with my approach for now until I can just use your implementation. Looks like I just need to load the config file into var targetConfig. I tested it with STM32F405 STM32F411 and OMNIBUSF4.

@Docteh
Copy link
Contributor Author

Docteh commented Aug 21, 2019

image

@mikeller
Copy link
Member

@Docteh: #1585 is merged now, you can rebase and use the ConfigInserter.

@Docteh
Copy link
Contributor Author

Docteh commented Aug 22, 2019

Do we want to do anything here? I'm already hiding the unified options when there is no STM32F411 build (3.3 maintenance)
image

@Docteh Docteh marked this pull request as ready for review August 23, 2019 23:40
@Docteh
Copy link
Contributor Author

Docteh commented Aug 24, 2019

We might want to translate the (Legacy) part.

@McGiverGim
Copy link
Member

We might want to translate the (Legacy) part

For sure...

@Docteh
Copy link
Contributor Author

Docteh commented Aug 24, 2019

Another question, should all the old style ones be labeled Legacy, or just the ones where we have a unified target? In this screenshot we only have a config for MATEKF411.
image

Or do we want this second choice?
image

@mikeller
Copy link
Member

mikeller commented Aug 24, 2019

@Docteh: The idea is to only mark the non-unified targets for which there is a Unified Target as legacy - for the rest, the non-unified target is still current.
For now we'll need to keep the 'legacy' targets around to give users a way to access older versions of the firmware, but I think we should consider pivoting the order of the drop downs, so that users first select the target, and then select what channel they want, which will allow us to go back to only showing one entry per target.

P.S.: I like 'ye olde', we should keep it. 😀

Copy link
Member

@mikeller mikeller left a comment

Choose a reason for hiding this comment

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

This is starting to look real good - the usability is getting to the point where the user won't notice.
Some points:

  • I think we should be removing the target name from the 'Select firmware' drop-down entries. They did not add much in the past, and now with Unified Targets they are only adding confusion, if anything:
    image
  • we need to add a minimum version check, and display available firmware versions for Unified Targets only if they are 4.1.0 or better - currently flashing a pre-4.1.0 build with a Unified Target will happily flash the Unified Target only, sans configuration, which I don't think is something we want:
    image
  • tying into the above, we should probably also fail instead of flashing if we try to insert the configuration, and find that the loaded .hex does not support custom defaults.

src/js/tabs/firmware_flasher.js Outdated Show resolved Hide resolved
var unifiedConfig; // Unified target configuration loaded from the menu, used when throwing out a local config

// Is there a list of these elsewhere?, used in parseUnifiedBuilds and ... look for unifiedTargets[0]
var unifiedTargets = [ 'STM32F411', 'STM32F405', 'STM32F745', 'STM32F7X2' ];
Copy link
Member

Choose a reason for hiding this comment

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

Not sure we even need a list of Unified Targets worst case we load a non-unified target, discover from the .hex that it does not support custom defaults, then fail.

return targetConfig;
}
var unifiedSource = 'https://api.github.com/repos/betaflight/unified-targets/contents/configs';
//unifiedSource = 'https://api.github.com/repos/betaflight/betaflight/contents/unified_targets/configs';
Copy link
Member

Choose a reason for hiding this comment

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

Please remove commented out code, or add a comment indicating the reason it is left in.

src/js/tabs/firmware_flasher.js Outdated Show resolved Hide resolved
@@ -288,7 +370,7 @@ TABS.firmware_flasher.initialize = function (callback) {

var expertMode_e = $('.tab-firmware_flasher input.expert_mode');
expertMode_e.prop('checked', globalExpertMode_e.is(':checked'));
$('input.show_development_releases').change(showOrHideBuildTypes).change();
$('input.show_development_releases').change(showOrHideBuildTypes); //.change();
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make a note to check, I either broke the relevant toggle, or I didn't.

chrome.storage.local.get(storageTag, function (result) {
let storageObj = result[storageTag];
let bareBoard = null;
//bareBoard = 'STM32F405';
Copy link
Member

Choose a reason for hiding this comment

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

Commented out code.

@mikeller mikeller added this to the 10.6.0 milestone Aug 25, 2019
Copy link
Member

@McGiverGim McGiverGim left a comment

Choose a reason for hiding this comment

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

Nice job is being done here. I want something usable to give a real start to the unified targets.

I have commented about some spaces missing in the older code, but now that you have moved it, maybe we can fix it.

src/js/tabs/firmware_flasher.js Outdated Show resolved Hide resolved
src/js/tabs/firmware_flasher.js Outdated Show resolved Hide resolved
src/js/tabs/firmware_flasher.js Outdated Show resolved Hide resolved
return;
}
var date = new Date(release.published_at);
var formattedDate = ("0" + date.getDate()).slice(-2) + "-" + ("0"+(date.getMonth()+1)).slice(-2) + "-" + date.getFullYear() + " " + ("0" + date.getHours()).slice(-2) + ":" + ("0" + date.getMinutes()).slice(-2);
Copy link
Member

Choose a reason for hiding this comment

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

Again, this is an old code, but if you can add spaces around + operator the code left will be better than the original.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wow that is horrible looking code

Copy link
Member

@mikeller mikeller left a comment

Choose a reason for hiding this comment

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

We need to move along with this - this is now ready to go in.

One more thing we need to do is prevent flashing of Unified Targets with configurations earlier than 4.1.0, as these targets do not support custom defaults.

@McGiverGim
Copy link
Member

Ok, we can merge this and add the remainings things at a later PR. We must start with something :)

@Docteh
Copy link
Contributor Author

Docteh commented Sep 5, 2019

Hi, sounds good to me.

@mikeller mikeller merged commit 7bd1f16 into betaflight:master Sep 5, 2019
@Docteh Docteh deleted the flashunification branch September 11, 2019 00:51
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.

3 participants