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

Add support for IAR version 8.x #4938

Merged
merged 10 commits into from Sep 11, 2017

Conversation

Projects
None yet
10 participants
@deepikabhavnani
Contributor

deepikabhavnani commented Aug 18, 2017

Added support to IAR 8.x, with the reference of release document

__aeabi_read_tp - Weak empty function is added to support the build. It should be implemented with TLS support in IAR Dlib.

@deepikabhavnani deepikabhavnani force-pushed the deepikabhavnani:IAR_fixes branch Aug 18, 2017

@0xc0170

What about testing? M0/M3/M4 with IAR all OK with these fixes (also backward compatible if possible. I assume yes as we are now using version < 8 macro in some changes here).

rtos/mbed_boot.c Outdated
@@ -574,9 +576,14 @@ void pre_main(void)
singleton_mutex_attr.cb_mem = &singleton_mutex_obj;
singleton_mutex_id = osMutexNew(&singleton_mutex_attr);
#if (__IAR_SYSTEMS_ICC__ >= 8)
__iar_Initlocks();

This comment has been minimized.

@0xc0170

0xc0170 Aug 21, 2017

Member

we are having exit function here defined as well? As the docs say, we should invoke it (as initlocks in multithreaded env, added here.

Info: http://netstorage.iar.com/SuppDB/Public/UPDINFO/012120/arm/doc/infocenter/iccarm.ENU.html

(This info is useful to know, could be part of the commit message to include details what this initlocks is fixing).

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Aug 21, 2017

Contributor

As per the release document, void __iar_clearlocks(void); should be called to destroy the locks. It should be called after atexit and _Close_all.
In mbed applications, we never do close all we just wait for system to die (in case of issue). Other option was to call it on main thread exit, but than timer thread and background thread will still be running.

platform/mbed_retarget.cpp Outdated
@@ -880,6 +880,9 @@ extern "C" WEAK void __iar_file_Mtxinit(__iar_Rmtx *mutex) {}
extern "C" WEAK void __iar_file_Mtxdst(__iar_Rmtx *mutex) {}
extern "C" WEAK void __iar_file_Mtxlock(__iar_Rmtx *mutex) {}
extern "C" WEAK void __iar_file_Mtxunlock(__iar_Rmtx *mutex) {}
#if (__IAR_SYSTEMS_ICC__ >= 8)
extern "C" WEAK void *__aeabi_read_tp (void) {}

This comment has been minimized.

@0xc0170

0xc0170 Aug 21, 2017

Member

why is this suddenly required, and what about the other requirements? As for any thread, there is more to implement than just read_tp ? See http://netstorage.iar.com/SuppDB/Public/UPDINFO/012120/arm/doc/infocenter/iccarm.ENU.html

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Aug 21, 2017

Contributor

Thread Local Storage requirement has changed in this version of IAR, and requires RTOS to allocate memory for secondary threads as part of TLS support. read_tp() is to point to the TLS section.

We do not have support of TLS in current RTOS, hence weak function is added to assist build. Actual support will be added in future with changes in RTOS.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 21, 2017

nit: should the title be "Add support for IAR 8"? or better yet: "Update IAR to version 8".

@theotherjimmy theotherjimmy changed the title from Add support to IAR 8.x to Add support for IAR 8.x Aug 21, 2017

@deepikabhavnani deepikabhavnani changed the title from Add support for IAR 8.x to Update IAR to version 8 Aug 21, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 22, 2017

@studavekar Are we still waiting for IAR 8.x update in CI?

@deepikabhavnani - all these changes are compatible with the exporter for IAR (templates we have are for v7.x and are forward compatible , all builds with 8.x) ? What version have you been testing with?

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Aug 22, 2017

@0xc0170 - I have verified exporter and build for K64F and Cortex-M23 with IAR 7.8/8.x @studavekar will verify few devices with IAR 8.x in single CI machine. We can label it to "needs: CI" but that would be manually triggered.

We have not updated the template files to version 3. Version 2 is compatible, we will be missing few advantages of version 3, till we support new template version.

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Aug 22, 2017

@deepikabhavnani @0xc0170 have setup the CI machines with IAR 8.11.x

Triggered a build for current PR

00:01:04.984 [DEBUG] Output:    IAR ELF Linker V8.11.2.13589/W32 for ARM
00:01:04.984 [DEBUG] Output:    Copyright 2007-2017 IAR Systems AB.

00:01:04.984 [DEBUG] Output:    IAR ELF Tool V10.1.5.191 [BUILT at IAR]
00:01:04.984 [DEBUG] Output:    Copyright 2007-2017 IAR Systems AB.

Build : http://mbed-ci-master-2.austin.arm.com:8081/view/All/job/build_matrix_stage_iar_8/2/

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Aug 22, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 22, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1081

Build failed!

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Aug 23, 2017

/morph test

1 similar comment
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 23, 2017

/morph test

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Aug 23, 2017

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 24, 2017

Result: ABORTED

Your command has finished executing! Here's what you wrote!

/morph test

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Aug 24, 2017

/morph test

1 similar comment
@studavekar

This comment has been minimized.

Collaborator

studavekar commented Aug 24, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 24, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1107

Build failed!

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Aug 25, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 25, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1115

Example Build failed!

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Aug 28, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 28, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1126

All builds and test passed!

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 29, 2017

@studavekar Could you trigger this manually with IAR8?

@deepikabhavnani deepikabhavnani force-pushed the deepikabhavnani:IAR_fixes branch Aug 29, 2017

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Aug 30, 2017

This would run with IAR 8

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 30, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1140

Build failed!

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Sep 10, 2017

@geky @sarahmarshy : Please review the fix in cellular example.

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Sep 10, 2017

@0xc0170 IAR build failure : http://mbed-ci-master-2.austin.arm.com:8081/job/iar_8_temp_pr_pipeline/

With the fix added , triggering the test again

/morph test

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Sep 10, 2017

/morph test

@0xc0170 0xc0170 added needs: CI and removed ready for merge labels Sep 10, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 10, 2017

@geky @sarahmarshy : Please review the fix in cellular example.

Looks fine to me. I had a look earlier this morning, syntax looked correct to me. Not sure why IAR 8 complained.

@hasnainvirk To be aware of this change, the latest commit with CTX update.

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 10, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1271

All builds and test passed!

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Sep 11, 2017

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Sep 11, 2017

@0xc0170 @deepikabhavnani Are we updating now to C++14 ? What about other compilers , are they going to be switched to C++14 as well ?
Regarding change in cellular example: Before C++11, string literals were handled separated by tokens and if there was a macro being combined with a string literal, it was considered as yet another token and hence the code like "my"MACRO compiled. Since C++11, all tokens must be separated by a space , so the proper fix to cellular example is to add a space between string literal and macro.

@@ -110,7 +110,7 @@ static bool set_CNMI(ATCmdParser *at)
// New SMS indication configuration
// set AT+CMTI=[mode, index] , 0 PDU mode, 1 text mode
// Multiple URCs for SMS, i.e., CMT, CMTI, UCMT, CBMI, CDSI as DTE could be following any of these SMS formats
bool success = at->send("AT+CNMI=2,"CTX) && at->recv("OK");
bool success = at->send("AT+CNMI=2,""1") && at->recv("OK");

This comment has been minimized.

@hasnainvirk

hasnainvirk Sep 11, 2017

Contributor

Add a space string literal and macro, it will work. Like:

bool success = at->send("AT+CNMI=2," CTX) && at->recv("OK");
@@ -141,7 +141,7 @@ static void CMT_URC(ATCmdParser *at)
static bool set_atd(ATCmdParser *at)
{
bool success = at->send("ATD*99***"CTX"#") && at->recv("CONNECT");
bool success = at->send("ATD*99***""1""#") && at->recv("CONNECT");

This comment has been minimized.

@hasnainvirk

hasnainvirk Sep 11, 2017

Contributor

same like above

@@ -469,7 +469,7 @@ nsapi_error_t PPPCellularInterface::setup_context_and_credentials()
#endif
success = _at->send("AT"
"+FCLASS=0;" // set to connection (ATD) to data mode
"+CGDCONT="CTX",\"%s\",\"%s%s\"",
"+CGDCONT=""1"",\"%s\",\"%s%s\"",

This comment has been minimized.

@hasnainvirk

hasnainvirk Sep 11, 2017

Contributor

same like above

Remove string literal values and revert back to using CTX macro .
The previous fix to replace CTX with string literals was the wrong
solution. All that was actually required was to insert a space before
the macro.
@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Sep 11, 2017

Results for IAR 8 : http://mbedosci.cloudapp.net/results/pr/4938/6/

@0xc0170 - LP ticker and sleep manager tests are failing for all three toolchains. Is it expected for all 4 devices? NCS36510  | NRF51_DK  | NRF52_DK | NUCLEO_F429ZI |

@adbridge

This comment has been minimized.

Contributor

adbridge commented Sep 11, 2017

@hasnainvirk I've pushed your change suggestions to Deepika's branch .Does this look better ?

@adbridge

This comment has been minimized.

Contributor

adbridge commented Sep 11, 2017

/morph test

@adbridge

This comment has been minimized.

Contributor

adbridge commented Sep 11, 2017

@studavekar Can we re-run with IAR 8 please and paste results once done?

@adbridge

This comment has been minimized.

Contributor

adbridge commented Sep 11, 2017

@deepikabhavnani yeah we are waiting on #5063 going in

@adbridge

This comment has been minimized.

Contributor

adbridge commented Sep 11, 2017

@SeppoTakalo @mikaleppanen @hasnainvirk pr_head has failed for some reason, any ideas ?
"ERROR: ARMmbed pipeline builds » mbed-os-cliapp » master #6025 completed with status ABORTED (propagate: false to ignore)
Finished: FAILURE"

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Sep 11, 2017

@adbridge It seems CI timed out and couldn't complete jobs for many targets. Too many jobs in parallel pipe line.

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Sep 11, 2017

ARMmbed pipeline builds » mbed-os-cliapp » master is area of Systest team. @marhil01 Who is responsible of maintaining mbed-os-cliapp?

I have no better knowledge, so how about restarting the testcases?

@adbridge

This comment has been minimized.

Contributor

adbridge commented Sep 11, 2017

I've restarted pr-head, hopefully it won't fail in the same way again

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 11, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1277

All builds and test passed!

@@ -217,7 +217,7 @@ emac_interface_t *wlan_emac_init_interface()
{
if (_emac == NULL) {
_emac = new emac_interface_t();
_emac = (emac_interface_t*) malloc(sizeof(emac_interface_t));

This comment has been minimized.

@sg-

sg- Sep 11, 2017

Member

This should use new or error in the case it fails, not print

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Sep 11, 2017

Contributor

@sg- Everywhere in file print statements are used instead error, I can add that as bug and get it fix in new PR

This comment has been minimized.

@sg-

sg- Sep 11, 2017

Member

new would have failed as assert but malloc won't. That is the change in behavior.

This comment has been minimized.

@geky

geky Sep 11, 2017

Member

Oh, @deepikabhavnani, this was the issue with const members right?

@sg-, the emac_interface_t as written can't be default constructed in c++ (here). The const member (ops) must be constructed but the emac_interface_t class doesn't have a constructor to initialize the member. Since the emac_api header is also included by C files, we can't add a constructor. C supports struct initializers for this case, but C++ relies on constructors.

We can either remove the const or allocate with malloc. We could change the error print to assert.

@adbridge adbridge added ready for merge and removed needs: CI labels Sep 11, 2017

@adbridge adbridge merged commit cab660d into ARMmbed:master Sep 11, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@deepikabhavnani deepikabhavnani changed the title from Update IAR to version 8 to Add support for IAR version 8.x Jul 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment