-
Notifications
You must be signed in to change notification settings - Fork 3k
[Exporters] Refactor Uvision and IAR and initial support of CMSIS projects #2708
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
"release_versions": ["2", "5"], | ||
"device_name": "NUC472HI8AE" | ||
}, | ||
"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.
It looks like there was an issue rebasing, there are two entries of NCS36510 now.
Awesome work, just a few comments. It should be clarified that this PR completely removes uVision 4 exporting, not just deprecates it, correct? |
/morph export-build |
Result: FAILUREYour command has finished executing! Here's what you wrote!
|
Cmsis pack is for MCU, not a target/platform?
I like this idea to get this info from the packs, not to scrape the project files. I would imagine to have a separate arm pack manage python module, that can be reused. For instance, progen would use it to eliminate progen def, as it's in here, or any other python module that requires some info from cmsis packs. Have you guys considered it? If not, someone else will :) What I don't like it's a duplication effort. If there can be a generic python module handling cmsis-packs, also another module for generating projects (in this case it would be progen or anything else you guys can write if you don't like what's out there), why to put all in here and not be able to reuse? I believe many of us would benefit from it (users, projects). That's my 5 cents. This is fine as it is ,it has been in this codebase. You eliminated custom beetle uvision project, was this tested it works with generic one? Because there were some settings for that target. Once reviewed, please clean the log - for instance "arm_pack_manager - ya gotta start somewhere" :-) You are missing |
Sorry, as per the pack meaning that the vendor comes from the pack, and we use that to set the debugger. |
@theotherjimmy and I made the decision to move armpackmanager into mbed, as it would eliminate the volley of version bumps when the module changes. The build system of mbed provides a lot of vital information. I think it is best to have exporters integrated with mbed tools, rather than existing as a separate, generic entity. I think this will make exporters more reliable, rather than trying to cater to cases outside of mbed. I appreciate that this is an opinion, and if mbed users somehow demand it, we should consider it. As it is, I think this will potentially provide a better experience for both users and developers. |
An interesting point that @sarahmarshy brings up, it may be good to keep the pack manager in mbed for now as changes are made to it. When it becomes more stable, it could always be brought out as a separate package. |
@bridadan that is exactly what I was thinking when I made the decision to include it in this PR. After watching the number of changes that happened within arm_pack_manager caused by the needs of this PR, I was unwilling to let it live on it's own just yet. |
I prefer to do it before review, so that review is done on the final commit series. When you say clean the log, do you mean to squash the arm_pack_manager commits, or do you want the commit messages edited to be more informative? |
@theotherjimmy : Mainly latter, but also squashing for some that should not be there. I would propose to send package manager separately, and this changeset on top of it. We are mixing again various logical changes here.
@sarahmarshy : Please check if a debugger is correct for some platforms (jlink - efm32 platforms, stlink, cmsis-dap). Otherwise this will cause a regression.
@theotherjimmy : That would be nice if that happens in the future, however I would consider it would be much better to start right from the beginning, as a separate module brings other challenges (proper versioning, self contained module - not using anything from this codebase, clean API as its user facing, this is not as tools are magic to a user).
@sarahmarshy : Regarding progen: To summarize: |
@0xc0170 package manager support in mbed is inextricably linked to this PR. It is necessary for using CMSIS packs as device information within Uvision and providing support for CMSIS projects. It's only function within mbed (for now) is as an exporter aid, so I think is this changeset is just for exporters |
9de3b87
to
b5bd0cf
Compare
@0xc0170 Can you clarify what those settings are? I don't have a beetle board to test, unfortunately. Looking at the diff starting here, I see that a generic ARM cortex target is used. This PR defaults to the generic CPU target if the device name is not present or is invalid. I've exported to Beetle, successfully set the target to Cortex-M3 and built, but I can't try flashing. I'll have to try to try to get ahold of a board. If you have one, could you try using this branch to export/flash in Uvision5? |
targetnames = TARGET_NAMES | ||
targetnames.sort() | ||
|
||
ide_list = ["iar", "uvision5"] |
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.
seems like a list of tests and ide should be in a json file for easy extension. Maybe another pr though
test: string name to index all_os_tests | ||
returns: tuple of test_name and source location of test, | ||
as given by find_tests""" | ||
all_os_tests = find_tests(ROOT, "K64F", "ARM") |
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.
Is this the only combination that is run?
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.
No, it will run the specified tests for all specified targets. This function unfortunately requires a target to return a dictionary of possible tests. However, most tests in the TESTS folder at the root of mbed-is should also support all other v5 boards (in addition to K64F). That is my understanding of that function thanks to @bridadan
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.
@sarahmarshy is right. She brings up a valid point thought that being able to supply None
or something to the the target/toolchain could maybe bring up tests that apply to all target toolchain/combinations. Then you wouldn't have to pick just one combination of target and toolchain to get a list of tests. This would be a separate PR though
/morph export-build |
Result: FAILUREYour command has finished executing! Here's what you wrote!
|
@sarahmarshy needs some conflicts resolved. |
0ee5b2a
to
b99996d
Compare
"S130", | ||
"TARGET_MCU_NRF51822" | ||
], | ||
"macros": ["NRF51", "TARGET_NRF51822", "BLE_STACK_SUPPORT_REQD", "SOFTDEVICE_PRESENT", "S130", "TARGET_MCU_NRF51822"], |
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.
Could revert this formatting change?
"lf_clock_src": { | ||
"value": "NRF_LF_SRC_XTAL", | ||
"macro_name": "MBED_CONF_NORDIC_NRF_LF_CLOCK_SRC" | ||
}, | ||
"macro_name": "MBED_CONF_NORDIC_NRF_LF_CLOCK_SRC"}, |
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.
Could the ending }
go on the next line?
"lf_clock_src": { | ||
"value": "NRF_LF_SRC_XTAL", | ||
"macro_name": "MBED_CONF_NORDIC_NRF_LF_CLOCK_SRC" | ||
}, | ||
"macro_name": "MBED_CONF_NORDIC_NRF_LF_CLOCK_SRC"}, |
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.
Same here about the }
"NRF52_PAN_62", | ||
"NRF52_PAN_63" | ||
], | ||
"macros_add": ["BOARD_PCA10040", "NRF52_PAN_12", "NRF52_PAN_15", "NRF52_PAN_58", "NRF52_PAN_55", "NRF52_PAN_54", "NRF52_PAN_31", "NRF52_PAN_30", "NRF52_PAN_51", "NRF52_PAN_36", "NRF52_PAN_53", "S132", "CONFIG_GPIO_AS_PINRESET", "BLE_STACK_SUPPORT_REQD", "SWI_DISABLE0", "NRF52_PAN_20", "NRF52_PAN_64", "NRF52_PAN_62", "NRF52_PAN_63"], |
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.
Can you change the formatting here?
Nevermind, seems to be inconsistent across a lot of the targets. Looks fine!
0b06f5e
to
d764d7d
Compare
d764d7d
to
2a8c9ef
Compare
/morph export-build |
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 @sarahmarshy for putting up with all of my complaints! Changes LGTM, let's see what CI says after going through a final run
Result: ABORTEDYour command has finished executing! Here's what you wrote!
|
/morph export-build |
Result: ABORTEDYour command has finished executing! Here's what you wrote!
|
/morph export-build |
Result: SUCCESSYour command has finished executing! Here's what you wrote!
|
@sarahmarshy Is there a documentation for iar how to add a new MCU? and how it works for uvision (cmsis packs)? I could not locate any readme in the exporters. Is it somewhere else? |
@0xc0170 Wouldn't it be better to leave that documentation to the IDEs? |
Description
Exporters
This PR removes the dependencies on progen and progen definitions. Instead, target information necessary for uvision project files is retrieved from CMSIS packs using the device name (which has been added to targets.json) and @theotherjimmy's arm pack manager. Arm pack manager has been moved to mbed tools, and you can view the history in the commits of this PR.
IAR continues to require magic strings, but they are now stored in iar_definitions.json in tools/export/iar. The definitions are indexed by the same device name found in targets.json.
Preferred debugger is set per vendor, as found in the CMSIS pack.
Uvision4 support has been removed.
In addition, the ability to export to a .cpdsc CMSIS project has been added (http://arm-software.github.io/CMSIS_5/Pack/html/cpdsc_pg.html). You can open this file in Uvision to see a project. but it will not build as Keil has not implemented specification of macros and scatter files. This is under active development.
Build tests
Export-build test now exports projects in parallel. It is also capable of testing the export of mbed-os tests. They can be found in the mbed-os/TESTS. The targets and tests are by default those from the latest release, though you can specify previous releases.
Status
READY
Todos
Steps to test or reproduce
To export
mbed export -m k64f -i uvision5
mbed export -m k64f -i iar
mbed export -m k64f -i cmsis
To run build tests
Please add UV4.exe and IarBuild.exe to your path.
On Windows:
UV4 is usually found in:
C:\Keil_v5\UV4\UV4.exe
IarBuild is usually found in:
C:\Program Files (x86)\IAR Systems\Embedded Workbench 7.x\common\bin\IarBuild.exe
python tools/test/export/build_test.py --release 2 -n MBED_BLINKY
(blinky is default for release 2)python tools/test/export/build_test.py --release 5 -os-tests tests-mbedmicro-rtos-mbed-basic
(though 5 is default release, and this basic test is the default test for release 5)@sg- @theotherjimmy @0xc0170 @screamerbg