-
Notifications
You must be signed in to change notification settings - Fork 3k
Dual Flash and Hex file support on NCS36510 #4813
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
Conversation
…the proposed changes by Mac. 7/25/17
targets/targets.json
Outdated
@@ -3132,7 +3132,7 @@ | |||
"device_name": "NUC472HI8AE", | |||
"bootloader_supported": true | |||
}, | |||
"NCS36510": { | |||
"36510": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is unintentional change. If we ever rename a target, it should be backward compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, that was unintentional and has been fixed. Thanks for catching it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. This looks like an unintentional change. It causes everything to break.
@jacobjohnson-ON Please push an additional commit on your branch that sets the target name back to "NCS36510".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maclobdell I have already pushed this, and on my end it shows up in the commits to this PR.
start, end = start_end_pairs[0] | ||
print("Memory start 0x%08X, end 0x%08X" % (start, end)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these debug logs? Should not print anything by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously accepted versions of this file have used print statements by default. Is that not allowed? I'm fine removing these print statements if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@0xc0170 Functionally, the print statements do not hurt anything, but for consistent user experience perhaps in the future they could be turned off by default. There can be a macro in the post build script that could enable / disable them.
Can this PR be merged as-is and then the issue of extra prints be addressed this later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jacobjohnson-ON I recommend using the toolchain notification functions instead. Let's do this: Get this merged with the prints in for now, and you submit a PR to clean them up later.
Optimally, you could use the "notify" method of the toolchains. This would look something like:
t_self.info("Merory stat 0x%08X, end 0x%08X" % (start, end))
This has two main benifits:
- It heeds the
-v
and-vv
flags. - It can print in the online compiler.
@maclobdell @c1728p9 @theotherjimmy Please review @jacobjohnson-ON Thanks for updating the dual flash support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tools changes look fine. I recommend improving the post-binary script in another PR.
@@ -177,7 +204,7 @@ def add_fib_at_start(arginput): | |||
output_hex_file[trim_area_start + 1] = (mac_addr_low >> 8) & 0xFF | |||
output_hex_file[trim_area_start + 2] = (mac_addr_low >> 16) & 0xFF | |||
output_hex_file[trim_area_start + 3] = (mac_addr_low >> 24) & 0xFF | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for getting this. I suppose it might be editor settings though 👍
@@ -29,17 +32,17 @@ def ranges(i): | |||
|
|||
|
|||
def add_fib_at_start(arginput): | |||
input_file = arginput + ".bin" | |||
file_name_hex = arginput + "_fib.hex" | |||
input_file = arginput + ".hex" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For best user experience, you should avoid assung the location of this file. This change can come with another PR.
If you change the code that calls this function (in tools/targets/init.py) to:
class NCS36510TargetCode:
@staticmethod
def ncs36510_addfib(t_self, resources, elf, binf):
from tools.targets.NCS import add_fib_at_start
add_fib_at_start(binf)
You could correspondingly change this function prototype (and first few lines) to:
def add_fib_at_start(input_file):
basename, ext = os.path.splitext(input_file)
assert(ext is ".hex", "Error: Post-bulid script input must be an intel-Hex format file")
file_name_hex = basename + ".hex"
file_name_hex = basename + ".bin"
This would be a bit safer, and a bit easier to understand when it fails.
For reference, I manually ran all the mbed-os tests with IAR, ARM, and GCC_ARM and they all passed on the fork submitted in this PR. Looks good to me. Thanks! |
Let's get this tested! /morph test |
/morph export-build |
CC @danish-iftikhar @danclement @maclobdell @jacobjohnson-ON Changes looks fine to me and all gcc_arm/arm/iar tests are passing on latest mbed OS as on yesterday plus this PR. |
@studavekar One non-reporter, one assembler bug. Could you retrigger after the release. |
@theotherjimmy @studavekar Please retest this, so it can be merged and part of the next release. |
/morph test |
/morph export-build |
Result: FAILUREYour command has finished executing! Here's what you wrote!
Outputmbed Build Number: 109 Exporter Build failed! |
I cannot access the "Details" link for the tests. Can someone explain the latest test failure? |
Result: SUCCESSYour command has finished executing! Here's what you wrote!
OutputAll builds and test passed! |
/morph export-build |
The failures were not related to this patch, I restarted the CI. |
Result: FAILUREYour command has finished executing! Here's what you wrote!
Outputmbed Build Number: 113 Exporter Build failed! |
/morph export-build |
@0xc0170 @theotherjimmy This PR is high priority. Thanks for helping get it in. |
/morph export-build |
Result: SUCCESSYour command has finished executing! Here's what you wrote!
Outputmbed Build Number: 118 All exports and builds passed! |
Description
Dual flash and Hex file support added. This is a separate pull request to replace #4397. The changes are the same as the latest revisions on the previous PR, including the changes requested by @maclobdell.
Status
READY
Related PRs
#4397
Deploy notes
Build output will be hex file. Post build script will insert FIB, User trim and trims gap.
Post script generates hex and binary file. Binary file required if user uses boot loader to download.
Steps to test or reproduce
All tests passed.
Also tested 640k operation by restoring the top level main.cpp file with standard blinky example, and adding the line:
const uint8_t array[12000] = {0};
This resulted in the compiled flash section being 327kB. Hex file was loaded to the device and it runs properly.
Also of note, the board is using the latest version of bootloader (V15) and the latest version of DAPLink (see links on original PR)