Initial support for flashing EXST .hex files to External Flash. #1565

hydra · 2019-08-08T00:10:34Z

Tested with hex files generated firmware from betaflight/betaflight#8679

All existing .hex files generated by the build are not suitable for flashing using the configurator as they don't have the zero fill. This issue wasn't known about until the code in this PR was written and tested.

Instructions for use:

Put the FC into flash bootloader mode. (not ROM bootloader mode, e.g. using cli command "bl flash") For H7EXTREME/NANO/ZERO there are instructions here if "bl flash" doesn't work for you - SPRacingH7EXTREME/H7NANO/H7ZERO - Firmware spracing/betaflight#1
Load suitable .hex file
Flash as normal.

Notes:

"Full chip erase" will erase the entire external flash, blackbox log, config, firmware and badblock sectors.
When "Full chip erase" is NOT selected, the config is NOT erased. See bootloader instructions if you need to erase your config.

Video here:

https://youtu.be/jyFupDg1biQ

Parsing `0800` was ok, but parsing `97CE` was not. The sign was not correctly handled, this uses `>>> 0` to force unsigned result. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Bitwise_Operators#%3E%3E%3E_(Zero-fill_right_shift)

mikeller · 2019-08-08T11:03:02Z

as they don't have the zero fill.

Not sure why you would need to have a zero fill when using .hex files to flash - .hex files allow for the target address of non-contiguos blocks to be explicitly set, so unless you need the space in between two blocks to be 0, just specifying the first block, beginning at its start address, and then the second block, also beginning at its start address, will work with .hex files.

mikeller · 2019-08-08T11:06:55Z

src/js/protocols/stm32usbdfu.js

-                            console.log('Using transfer size: ' + self.transferSize);
-                            self.clearStatus(function () {
-                                self.upload_procedure(1);
+                    if (typeof chipInfo.internal_flash != "undefined") {


Should be a non-coercing not equals.

mikeller · 2019-08-08T11:07:55Z

src/js/protocols/stm32usbdfu.js

                            });
-                        });
+                        }
+                    } else if (typeof chipInfo.external_flash != "undefined") {


Should be a non-coercing not equals.

hydra · 2019-08-08T11:16:54Z

as they don't have the zero fill.

Not sure why you would need to have a zero fill when using .hex files to flash - .hex files allow for the target address of non-contiguos blocks to be explicitly set, so unless you need the space in between two blocks to be 0, just specifying the first block, beginning at its start address, and then the second block, also beginning at its start address, will work with .hex files.

yes, the zero fill is needed because the 0's are used for exst hash calculation.

I didnt see any other command line arguments to objcopy that allow the gap to be set, it would also require additional code in the hex parser anyway, iirc.

mikeller · 2019-08-08T11:40:16Z

it would also require additional code in the hex parser anyway, iirc

The .hex file parser in the Betaflight configurator can already handle non-contiguos blocks - that's how it is possible to update the firmware without resetting the configuration.

hydra · 2019-08-08T12:18:13Z

@mikeller yes I know it does non-continguous blocks. I had a good look at the code and step-debugged through a lot of it to see how it dealt with hex data sections, etc. However that is not the issue here.

The issue is that the EXST hash is computed on the firmware BINARY, minus the last 64 byte block, this binary contains 0's as defined by the linker script. However, when you use objcopy to generate a hex file from the patched .elf file it does not output the area between the end of firmware and the start of the hash block.

Since the configurator will erase pages as needed, based on the addresses in the hex file and sector boundaries of the flash, and these erased pages on NAND flash have a default value of 0xFF when read, the hash computed from the content on flash won't match the hash at the end of the firmware block.

To me the simplest solution is to generate a .hex file have the same binary content as the .bin file.

hydra · 2019-08-08T12:29:26Z

@mikeller with regards to use of "typeof" it seems kinda weird that in the same file, stm32usbdfu.js there are now two ways to check if something is defined, in some cases negating the comparison. e.g.

                if (resultCode != 0 || typeof chipInfo === "undefined") {
                    ...
                } else {
                    if (chipInfo.internal_flash != undefined) {
                   ...

Initially the way I had it was to use typeof for consistency, can you shed some light on why you requested the non-coercing expression? My JS is a bit rusty at the moment so maybe I'm missing something here?

mikeller · 2019-08-08T21:11:04Z

@hydra: You misunderstood me. All I meant you to do was to change the != in the expression typeof chipInfo.external_flash != "undefined" to a !==.
In my opinion, typeof x !== "undefined" is a wordy way to check x !== undefined, but it is correct.
But == and != should only ever be used in JavaScript if one wants to specifically check 'is there a way to cast x into y?'. In every other case === and !== should be used.
Coming from C, this is a major pitfall in JavaScript for me.

Docteh · 2019-08-08T21:17:01Z

Looking at the use of undefined in quotes might be worth a look.

I'm the sort of guy that has to go and look up what coercing means as far as javascript goes, but I've noticed that we've got some code that has been unmodified since baseflight-configurator

We're using undefined outside of quotes a lot more than inside of quotes

Looks like all the "undefined" in stm32usbdfu.js are from one commit: 20b6d65

src/js/main.js:        CliAutoComplete.setEnabled(typeof result.cliAutoComplete == 'undefined' || result.cliAutoComplete); // On by default
src/js/protocols/stm32usbdfu.js:    if (typeof _timeout === "undefined") {
src/js/protocols/stm32usbdfu.js:                            if(typeof abort === "undefined" || abort) {
src/js/protocols/stm32usbdfu.js:                if (resultCode != 0 || typeof chipInfo === "undefined") {
src/js/protocols/stm32usbdfu.js:                    if (typeof chipInfo.internal_flash === "undefined") {
src/js/protocols/stm32usbdfu.js:                if (typeof self.chipInfo.option_bytes === "undefined") {
src/js/tabs/failsafe.js:        if (typeof RSSI_CONFIG.channel !== 'undefined')  {
src/js/serial_backend.js:        if (result.auto_connect === 'undefined' || result.auto_connect) {

mikeller · 2019-08-08T21:37:00Z

@Docteh:

There's the difference:

undefined is a constant that is assigned to any variable / property on declaration (potentially implicit);
"undefined" is the type of undefined.

The type system in JavaScript, and the typeof operator (evaluating to a string) are things that were done in a hurry in JavaScript, and they are ugly and clunky.

Type checking with typeof makes sense for most types, but in the case of "undefined" it is pretty much redundant, since this type will only ever be returned by undefined, making typeof(x) === "undefined" equivalent to x === undefined.

If you want to get into the nitty gritty of JavaScript, I can recommend Douglas Crockford's presentations on it: https://www.youtube.com/watch?v=JxAXlJEmNMg

hydra · 2019-08-08T23:46:57Z

will fix this up in the morning. thanks for feedback and info.

In this case, it's invalid if the HEX file doesn't contain any data in the address range of the chip to flash.

hydra · 2019-08-09T11:18:26Z

@hydra: You misunderstood me. All I meant you to do was to change the != in the expression typeof chipInfo.external_flash != "undefined" to a !==.

ok done.

In my opinion, typeof x !== "undefined" is a wordy way to check x !== undefined, but it is correct.

agreed!

LGTM!

McGiverGim · 2019-08-09T11:42:59Z

In my opinion, typeof x !== "undefined" is a wordy way to check x !== undefined, but it is correct.

Now I'm working on a PR that uses this, and both are different.

https://github.com/McGiverGim/betaflight-configurator/blob/af423ddb8b82605ffb4608d59bafbb35b5c704b3/src/js/tabs/vtx.js#L22-L32

If I remove the typeof the result is different. This is an ugly hack to execute a MSP command in a loop to retrieve an array. The MSP is async so I needed this kind of things to make it working.

hydra · 2019-08-09T20:52:32Z

If I remove the typeof the result is different. This is an ugly hack to execute a MSP command in a loop to retrieve an array. The MSP is async so I needed this kind of things to make it working.

you must use undefined (keyword) rather than "undefined" (string literal) if you also remove the typeof.

Since this PR checks the same object for two properties and results in false for the first expression and true for the second one when flashing an EXST target we can be sure that all versions of the expression work OK. i.e. For this PR, both the original expression, the second version and the current expression all work fine.

I'm happy to use any of the expressions so far, but like @mikeller I often err on the side of strictness when it comes to languages that do type coercion so that the reader of the code can clearly see the intent.

mikeller · 2019-08-10T00:40:02Z

src/js/protocols/stm32usbdfu.js

                  }
                }
+
+                if (erase_pages.length == 0) {


Same deal here, should be ===.

mikeller · 2019-08-10T00:45:31Z

src/js/workers/hex_parser.js

            case 0x04: // extended linear address record
-                extended_linear_address = (parseInt(content.substr(0, 2), 16) << 24) | parseInt(content.substr(2, 2), 16) << 16;
+                // input address is UNSIGNED
+                extended_linear_address = ((parseInt(content.substr(0, 2), 16) << 24) | parseInt(content.substr(2, 2), 16) << 16) >>> 0;


Not sure why you are adding the >>> 0 here?

Byte overflow? not sure on the right term. I checked with the binary he has posted:

Did console.log of extended_linear_address when it was set. Hex conversion is what windows calculator says. Old way: -1748107264 FFFF FFFF 97CE 0000 New way: 2546860032 97CE 0000

I got suggested this when I looked for 'uint32 javascript' so it seems reasonable.
https://stackoverflow.com/questions/22335853/hack-to-convert-javascript-number-to-uint32

yes, it's because all bitwise operators in JS are 32 bit signed. The >>> 0 forces the result to UNSIGNED.

Heh, signed bitwise operators, who wouldn't want those.

hydra · 2019-08-11T10:37:29Z

ready for merge now?

mikeller · 2019-08-11T14:40:48Z

src/js/workers/hex_parser.js

            case 0x04: // extended linear address record
-                extended_linear_address = (parseInt(content.substr(0, 2), 16) << 24) | parseInt(content.substr(2, 2), 16) << 16;
+                // input address is UNSIGNED
+                extended_linear_address = ((parseInt(content.substr(0, 2), 16) << 24) | parseInt(content.substr(2, 2), 16) << 16) >>> 0;


Heh, signed bitwise operators, who wouldn't want those.

mikeller requested changes Aug 8, 2019

View reviewed changes

hydra requested a review from mikeller August 8, 2019 15:43

hydra added 2 commits August 9, 2019 13:14

Support EXST flashing.

2cfc173

Display error to user when HEX file is invalid.

5cd4b2f

In this case, it's invalid if the HEX file doesn't contain any data in the address range of the chip to flash.

mikeller requested changes Aug 10, 2019

View reviewed changes

Use strict operator when checking for flash pages to erase.

4ff2a11

mikeller approved these changes Aug 11, 2019

View reviewed changes

mikeller added this to the 10.6.0 milestone Aug 11, 2019

mikeller added the RN: IMPROVEMENT label Aug 11, 2019

mikeller merged commit 7189363 into betaflight:master Aug 11, 2019

Uh oh!

Initial support for flashing EXST .hex files to External Flash. #1565

Initial support for flashing EXST .hex files to External Flash. #1565

Uh oh!

Conversation

hydra commented Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikeller commented Aug 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hydra commented Aug 8, 2019

Uh oh!

mikeller commented Aug 8, 2019

Uh oh!

hydra commented Aug 8, 2019

Uh oh!

hydra commented Aug 8, 2019

Uh oh!

mikeller commented Aug 8, 2019

Uh oh!

Docteh commented Aug 8, 2019

Uh oh!

mikeller commented Aug 8, 2019

Uh oh!

hydra commented Aug 8, 2019

Uh oh!

hydra commented Aug 9, 2019

Uh oh!

McGiverGim commented Aug 9, 2019

Uh oh!

hydra commented Aug 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hydra Aug 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hydra commented Aug 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hydra commented Aug 8, 2019 •

edited

Loading

hydra commented Aug 9, 2019 •

edited

Loading

hydra Aug 11, 2019 •

edited

Loading