Keil uVision4 support

[micromint]

Description

MBED_DEPRECATED macro breaks builds using Keil uVision4. The 'deprecated' attribute does not allow arguments using ARMCC 5.03 on Keil 4.74. Changing #ifdef order reinstates support for Keil uVision4. Tested with Keil 4.74, Keil 5.21 and GCC ARM Embedded 5.4 2016q3.

Status

READY

Todos

[theotherjimmy]

How did you export this to test it?

[micromint]

We test the changes on our internal repository. General purpose changes are pushed to our public github (link below). A consistent build can be exported from from there.

https://github.com/micromint/mbed-lpc43xx

[theotherjimmy]

There is a large number of commits to accomplish such a simple change. Could you clean up the history some? it's hard to follow.

A consistent build can be exported from there.

I was asking specifically about how you got to uVision 4 because it has been removed

[micromint]

Only these two commits are relevant:

Change MBED_DEPRECATED def order to support Keil 4 f9a1678
platform/toolchain.h

Fix default polarity on LPC43XX PWM driver cd269f9
targets/TARGET_NXP/TARGET_LPC43XX/pwmout_api.c

The other ones are temporary changes that were eventually rolled back. Those have no net file changes.

We used to push temporary changes to our public github. Now we do temporary ones internally and don't push them to our public github until they become general purpose.

[theotherjimmy]

Thanks for the explanation.

Further, Could these be two different PR's? They don't seem related at all.

[micromint]

With the online compiler I can still export projects to Keil uVision4. Don't know if it is because I have the "beta" mode enabled.

Keil 4 can also be used when building the library offline by specifying the relevant paths in mbed_settings.py.

[theotherjimmy]

With the online compiler I can still export projects to Keil uVision4.

Ah. That makes sense.

Keil 4 can also be used when building the library offline by specifying the relevant paths in mbed_settings.py.

We generally call that the ARM compiler 5 when used in that way (through mbed compile ...)

[micromint]

It would have been better to do 2 PR's. After the update from the pre mbed repository to the current mbed-os one they got pushed together.

[theotherjimmy]

It would have been better to do 2 PR's.

There's nothing stopping you from doing that right now!

something like (assuming this repo is setup as the remote armmebd and your remote as origin):

git checkout armmbed/master -b armc5-deprecated-macro
git cherrypick f9a1678
git push --set-upstream origin
git checkout armmbed/master -b lpc43xx-pwm-polarity
git cherrypick cd269f9
git push --set-upstream origin

would give you the branches in git. Then it's just a bit github away.

[micromint]

The current MBED_DEPRECATED macro definition fails with ARMCC 5.03 on Keil MDK 4.74. With ARMCC 5.06 on Keil MDK 5.21 it compiles, but seems to ignore the argument. The fix changes the check to ARMCC first and GCC afterwards instead of the other way around so the 'deprecated' attribute does not use arguments.

[theotherjimmy]

Does your version work with both ARMCC 5.03 and 5.06? It looks like it's passing CI, so probably yes.

[micromint]

Yes, as stated on the description:

Tested with Keil 4.74, Keil 5.21 and GCC ARM Embedded 5.4 2016q3.

[theotherjimmy]

Awesome! Sorry that my reading comprehension was non-existent.

[micromint]

Thanks for pointing out the 'git cherrypick' procedure.

Would making 2 separate PR's be a requirement for acceptance? Only two files are affected and the changes are relatively minor.

For future changes, I'll checkout each change to a separate branch so pulls are granular.

[theotherjimmy]

Would making 2 separate PR's be a requirement for acceptance?

Nope. It might speed up acceptance though.

[theotherjimmy]

For future changes, I'll checkout each change to a separate branch so pulls are granular.

That sounds like a great plan! I'm also glad to hear that you're planning on contributing more!

[theotherjimmy]

I take it that you're planning on leaving this as one PR for now. Do you mind if I clean up the history for you? I would reduce the history to just the two commits needed.

[micromint]

Do you mind if I clean up the history for you?

No problem. Please go ahead.

[0xc0170]

I take it that you're planning on leaving this as one PR for now. Do you mind if I clean up the history for you? I would reduce the history to just the two commits needed.

I would split those two in to separate PR

[micromint]

I would split those two in to separate PR

Ok. Reverted the PWM change. This PR only has the change in MBED_DEPRECATED to support Keil uVision4. Will post the PWM change in another PR after we are done with a customer project.

[theotherjimmy]

@0xc0170 @micromint I'll get this for you. It's ~5 min for me.

[micromint]

Is one of the test servers down? The test report states a check was not successful. Tried checking details several times but get a message that host jenkins-internal.mbed.com is not responding.

[theotherjimmy]

Is one of the test servers down?

No. Maybe you can't see it.

[theotherjimmy]

One down. I don't have the commit from the other one, so I can't create that minimal PR.

[sg-]

Closing in favor of #3250 If there is another change needed, lets reopen in another PR.

[micromint]

@theotherjimmy Thanks!

The other commit is at:

Fix default polarity on LPC43XX PWM driver cd269f9
targets/TARGET_NXP/TARGET_LPC43XX/pwmout_api.c

If that commit is no longer accessible for PR's, I'll resubmit after we are done with another project.

[theotherjimmy]

That commit is no longer accessible from your repo! I suppose that will have to come later.

[micromint]

Ok. Cleared history on my repo and re-submitted the PWM change separately as #3253